Bail early from UnpackDomainName when name is too long (#839)

* Simplify maxDomainNameWireOctets checking in UnpackDomainName

* Don't return too long name in UnpackDomainName

* Simplify root domain return in UnpackDomainName

* Bail early from UnpackDomainName when name is too long

This drastically reduces the amount of garbage created
in UnpackDomainName for certain malicious names.

The wire formatted name
 "\x3Faaabbbcccdddeeefffggghhhiiijjjkkklllmmmnnnooopppqqqrrrssstttuuu\xC0\x00"
would previously generate 1936B of garbage (36112B since maxCompressionPointers
was raised) before returning the "too many compression pointers" error, while
it now generates just 384B of garbage.

* Change +1 budget comment to reflect spec

This better reflects what maxDomainNameWireOctets is actually measuring.

* Remove budget check from after loop in UnpackDomainName

This can never be tripped as budget is always checked immediately after
subtracting inside the loop.

* Improve UnpackDomainName documentation

msg.go

@@ -375,12 +375,16 @@ func isRootLabel(s string, bs []byte, off, end int) bool {
 // In theory, the pointers are only allowed to jump backward.
 // We let them jump anywhere and stop jumping after a while.
 
-// UnpackDomainName unpacks a domain name into a string.
+// UnpackDomainName unpacks a domain name into a string. It returns
+// the name, the new offset into msg and any error that occurred.
+//
+// When an error is encountered, the unpacked name will be discarded
+// and len(msg) will be returned as the offset.
 func UnpackDomainName(msg []byte, off int) (string, int, error) {
 	s := make([]byte, 0, 64)
 	off1 := 0
 	lenmsg := len(msg)
-	maxLen := maxDomainNameWireOctets
+	budget := maxDomainNameWireOctets
 	ptr := 0 // number of pointers followed
 Loop:
 	for {
@@ -399,14 +403,16 @@ Loop:
 			if off+c > lenmsg {
 				return "", lenmsg, ErrBuf
 			}
+			budget -= c + 1 // +1 for the label separator
+			if budget <= 0 {
+				return "", lenmsg, ErrLongDomain
+			}
 			for j := off; j < off+c; j++ {
 				switch b := msg[j]; b {
 				case '.', '(', ')', ';', ' ', '@':
 					fallthrough
 				case '"', '\\':
 					s = append(s, '\\', b)
-					// presentation-format \X escapes add an extra byte
-					maxLen++
 				default:
 					if b < 32 || b >= 127 { // unprintable, use \DDD
 						var buf [3]byte
@@ -416,8 +422,6 @@ Loop:
 							s = append(s, '0')
 						}
 						s = append(s, bufs...)
-						// presentation-format \DDD escapes add 3 extra bytes
-						maxLen += 3
 					} else {
 						s = append(s, b)
 					}
@@ -455,10 +459,7 @@ Loop:
 		off1 = off
 	}
 	if len(s) == 0 {
-		s = []byte(".")
-	} else if len(s) >= maxLen {
-		// error if the name is too long, but don't throw it away
-		return string(s), lenmsg, ErrLongDomain
+		return ".", off1, nil
 	}
 	return string(s), off1, nil
 }

msg_test.go

@@ -148,10 +148,9 @@ func TestUnpackDomainName(t *testing.T) {
 			"\x03foo" + "\x05\x03com\x00" + "\x07example" + "\xC0\x05",
 			"foo.\\003com\\000.example.com.",
 			""},
-
 		{"too long domain",
 			string(54) + "x" + strings.Replace(longDomain, ".", string(49), -1) + "\x00",
-			"x" + longDomain + ".",
+			"",
 			ErrLongDomain.Error()},
 		{"too long by pointer",
 			// a matryoshka doll name to get over 255 octets after expansion via internal pointers
@@ -193,10 +192,11 @@ func TestUnpackDomainName(t *testing.T) {
 			""},
 		{"truncated name", "\x07example\x03", "", "dns: buffer size too small"},
 		{"non-absolute name", "\x07example\x03com", "", "dns: buffer size too small"},
-		{"compression pointer cycle",
+		{"compression pointer cycle (too many)", "\xC0\x00", "", "dns: too many compression pointers"},
+		{"compression pointer cycle (too long)",
 			"\x03foo" + "\x03bar" + "\x07example" + "\xC0\x04",
 			"",
-			"dns: too many compression pointers"},
+			ErrLongDomain.Error()},
 		{"reserved compression pointer 0b10", "\x07example\x80", "", "dns: bad rdata"},
 		{"reserved compression pointer 0b01", "\x07example\x40", "", "dns: bad rdata"},
 	}