From c1bdeb82b2ab425fc56b2f50ece8c7f5faa10b6c Mon Sep 17 00:00:00 2001 From: Richard Gibson Date: Wed, 29 Mar 2017 15:43:02 -0400 Subject: [PATCH] Domain name limits (#478) * limiting domain names to 255/63 octets/labels (#463) (cherry picked from commit 0b729df06cb29cf7a3ff59b876e9a2ecb7fac2b1) * account for \ and \xxx in presentation format (cherry picked from commit a094f774892fb4305051d185c2488cb43200c4d9) * go fmt * Add tests for UnpackDomainName Domain names must not exceed 255 octets in wire format. Ref gh-463 Ref gh-469 * Fix UnpackDomainName * Introduce a long-domain sentinel error A typed error would be better, but inconsistent with this library. cf. https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully --- msg.go | 20 ++++++++- msg_test.go | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 msg_test.go diff --git a/msg.go b/msg.go index 57262a10..a348f7c9 100644 --- a/msg.go +++ b/msg.go @@ -14,13 +14,17 @@ package dns import ( crand "crypto/rand" "encoding/binary" + "fmt" "math/big" "math/rand" "strconv" "sync" ) -const maxCompressionOffset = 2 << 13 // We have 14 bits for the compression pointer +const ( + maxCompressionOffset = 2 << 13 // We have 14 bits for the compression pointer + maxDomainNameWireOctets = 255 // See RFC 1035 section 2.3.4 +) var ( ErrAlg error = &Error{err: "bad algorithm"} // ErrAlg indicates an error with the (DNSSEC) algorithm. @@ -33,6 +37,7 @@ var ( ErrKeyAlg error = &Error{err: "bad key algorithm"} // ErrKeyAlg indicates that the algorithm in the key is not valid. ErrKey error = &Error{err: "bad key"} ErrKeySize error = &Error{err: "bad key size"} + ErrLongDomain error = &Error{err: fmt.Sprintf("domain name exceeded %d wire-format octets", maxDomainNameWireOctets)} ErrNoSig error = &Error{err: "no signature found"} ErrPrivKey error = &Error{err: "bad private key"} ErrRcode error = &Error{err: "bad rcode"} @@ -329,6 +334,7 @@ func UnpackDomainName(msg []byte, off int) (string, int, error) { s := make([]byte, 0, 64) off1 := 0 lenmsg := len(msg) + maxLen := maxDomainNameWireOctets ptr := 0 // number of pointers followed Loop: for { @@ -353,8 +359,10 @@ Loop: fallthrough case '"', '\\': s = append(s, '\\', b) + // presentation-format \X escapes add an extra byte + maxLen += 1 default: - if b < 32 || b >= 127 { // unprintable use \DDD + if b < 32 || b >= 127 { // unprintable, use \DDD var buf [3]byte bufs := strconv.AppendInt(buf[:0], int64(b), 10) s = append(s, '\\') @@ -364,6 +372,8 @@ Loop: for _, r := range bufs { s = append(s, r) } + // presentation-format \DDD escapes add 3 extra bytes + maxLen += 3 } else { s = append(s, b) } @@ -388,6 +398,9 @@ Loop: if ptr++; ptr > 10 { return "", lenmsg, &Error{err: "too many compression pointers"} } + // pointer should guarantee that it advances and points forwards at least + // but the condition on previous three lines guarantees that it's + // at least loop-free off = (c^0xC0)<<8 | int(c1) default: // 0x80 and 0x40 are reserved @@ -399,6 +412,9 @@ Loop: } 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 string(s), off1, nil } diff --git a/msg_test.go b/msg_test.go new file mode 100644 index 00000000..2dbef626 --- /dev/null +++ b/msg_test.go @@ -0,0 +1,124 @@ +package dns + +import ( + "fmt" + "regexp" + "strconv" + "strings" + "testing" +) + +const ( + maxPrintableLabel = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789x" + tooLongLabel = maxPrintableLabel + "x" +) + +var ( + longDomain = maxPrintableLabel[:53] + strings.TrimSuffix( + strings.Join([]string{".", ".", ".", ".", "."}, maxPrintableLabel[:49]), ".") + reChar = regexp.MustCompile(`.`) + i = -1 + maxUnprintableLabel = reChar.ReplaceAllStringFunc(maxPrintableLabel, func(ch string) string { + if i++; i >= 32 { + i = 0 + } + return fmt.Sprintf("\\%03d", i) + }) +) + +func TestUnpackDomainName(t *testing.T) { + var cases = []struct { + label string + input string + expectedOutput string + expectedError string + }{ + {"empty domain", + "\x00", + ".", + ""}, + {"long label", + string(63) + maxPrintableLabel + "\x00", + maxPrintableLabel + ".", + ""}, + {"unprintable label", + string(63) + regexp.MustCompile(`\\[0-9]+`).ReplaceAllStringFunc(maxUnprintableLabel, + func(escape string) string { + n, _ := strconv.ParseInt(escape[1:], 10, 8) + return string(n) + }) + "\x00", + maxUnprintableLabel + ".", + ""}, + {"long domain", + string(53) + strings.Replace(longDomain, ".", string(49), -1) + "\x00", + longDomain + ".", + ""}, + {"compression pointer", + // an unrealistic but functional test referencing an offset _inside_ a label + "\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 + string([]byte{ + // 11 length values, first to last + 40, 37, 34, 31, 28, 25, 22, 19, 16, 13, 0, + // 12 filler values + 120, 120, 120, 120, 120, 120, 120, 120, 120, 120, 120, 120, + // 10 pointers, last to first + 192, 10, 192, 9, 192, 8, 192, 7, 192, 6, 192, 5, 192, 4, 192, 3, 192, 2, 192, 1, + }), + "", + ErrLongDomain.Error()}, + {"long by pointer", + // a matryoshka doll name _not_ exceeding 255 octets after expansion + string([]byte{ + // 11 length values, first to last + 37, 34, 31, 28, 25, 22, 19, 16, 13, 10, 0, + // 9 filler values + 120, 120, 120, 120, 120, 120, 120, 120, 120, + // 10 pointers, last to first + 192, 10, 192, 9, 192, 8, 192, 7, 192, 6, 192, 5, 192, 4, 192, 3, 192, 2, 192, 1, + }), + "" + + (`\"\031\028\025\022\019\016\013\010\000xxxxxxxxx` + + `\192\010\192\009\192\008\192\007\192\006\192\005\192\004\192\003\192\002.`) + + (`\031\028\025\022\019\016\013\010\000xxxxxxxxx` + + `\192\010\192\009\192\008\192\007\192\006\192\005\192\004\192\003.`) + + (`\028\025\022\019\016\013\010\000xxxxxxxxx` + + `\192\010\192\009\192\008\192\007\192\006\192\005\192\004.`) + + (`\025\022\019\016\013\010\000xxxxxxxxx` + + `\192\010\192\009\192\008\192\007\192\006\192\005.`) + + `\022\019\016\013\010\000xxxxxxxxx\192\010\192\009\192\008\192\007\192\006.` + + `\019\016\013\010\000xxxxxxxxx\192\010\192\009\192\008\192\007.` + + `\016\013\010\000xxxxxxxxx\192\010\192\009\192\008.` + + `\013\010\000xxxxxxxxx\192\010\192\009.` + + `\010\000xxxxxxxxx\192\010.` + + `\000xxxxxxxxx.`, + ""}, + {"truncated name", "\x07example\x03", "", "dns: buffer size too small"}, + {"non-absolute name", "\x07example\x03com", "", "dns: buffer size too small"}, + {"compression pointer cycle", + "\x03foo" + "\x03bar" + "\x07example" + "\xC0\x04", + "", + "dns: too many compression pointers"}, + {"reserved compression pointer 0b10", "\x07example\x80", "", "dns: bad rdata"}, + {"reserved compression pointer 0b01", "\x07example\x40", "", "dns: bad rdata"}, + } + for _, test := range cases { + output, idx, err := UnpackDomainName([]byte(test.input), 0) + if test.expectedOutput != "" && output != test.expectedOutput { + t.Errorf("%s: expected %s, got %s", test.label, test.expectedOutput, output) + } + if test.expectedError == "" && err != nil { + t.Errorf("%s: expected no error, got %d %v", test.label, idx, err) + } else if test.expectedError != "" && (err == nil || err.Error() != test.expectedError) { + t.Errorf("%s: expected error %s, got %d %v", test.label, test.expectedError, idx, err) + } + } +}