Repository: miekg/dns
Pull request #1540 (2024-03-01 12:57:59)

Fix counting of escape sequences when splitting TXT strings
`endingToTxtSlice`, used by TXT, SPF and a few other types, parses a string such as `"hello world"` from an RR's content in a zone file. These strings are limited to 255 characters, and `endingToTxtSlice` automatically splits them if they're longer than that. However, it didn't count the length correctly: escape sequences such as `\\` or `\123` were counted as multiple characters (2 and 4 respectively in these examples), but they should only count as one character because they represent a single byte in wire format (which is where this 255 character limit comes from). This commit fixes that.

janik-cloudflare: A more detailed explanation of the problem: https://github.com/miekg/dns/issues/1384#issuecomment-1946017184
janik-cloudflare: Apologies, found a bug! Fix and more tests coming today.

Patch:
diff --git a/README.md b/README.md
index bca1a55a..58275db3 100644
--- a/README.md
+++ b/README.md
@@ -84,6 +84,7 @@ A not-so-up-to-date-list-that-may-be-actually-current:
 * https://github.com/semihalev/sdns
 * https://github.com/wintbiit/NineDNS
 * https://linuxcontainers.org/incus/
+* https://ifconfig.es
 
 
 Send pull request if you want to be listed here.
diff --git a/go.mod b/go.mod
index 77a744c5..bd188208 100644
--- a/go.mod
+++ b/go.mod
@@ -3,10 +3,10 @@ module github.com/miekg/dns
 go 1.19
 
 require (
-	golang.org/x/net v0.20.0
+	golang.org/x/net v0.22.0
 	golang.org/x/sync v0.6.0
-	golang.org/x/sys v0.16.0
-	golang.org/x/tools v0.17.0
+	golang.org/x/sys v0.18.0
+	golang.org/x/tools v0.19.0
 )
 
-require golang.org/x/mod v0.14.0 // indirect
+require golang.org/x/mod v0.16.0 // indirect
diff --git a/go.sum b/go.sum
index 5bad0575..d0f87415 100644
--- a/go.sum
+++ b/go.sum
@@ -1,10 +1,10 @@
-golang.org/x/mod v0.14.0 h1:dGoOF9QVLYng8IHTm7BAyWqCqSheQ5pYWGhzW00YJr0=
-golang.org/x/mod v0.14.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
-golang.org/x/net v0.20.0 h1:aCL9BSgETF1k+blQaYUBx9hJ9LOGP3gAVemcZlf1Kpo=
-golang.org/x/net v0.20.0/go.mod h1:z8BVo6PvndSri0LbOE3hAn0apkU+1YvI6E70E9jsnvY=
+golang.org/x/mod v0.16.0 h1:QX4fJ0Rr5cPQCF7O9lh9Se4pmwfwskqZfq5moyldzic=
+golang.org/x/mod v0.16.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
+golang.org/x/net v0.22.0 h1:9sGLhx7iRIHEiX0oAJ3MRZMUCElJgy7Br1nO+AMN3Tc=
+golang.org/x/net v0.22.0/go.mod h1:JKghWKKOSdJwpW2GEx0Ja7fmaKnMsbu+MWVZTokSYmg=
 golang.org/x/sync v0.6.0 h1:5BMeUDZ7vkXGfEr1x9B4bRcTH4lpkTkpdh0T/J+qjbQ=
 golang.org/x/sync v0.6.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
-golang.org/x/sys v0.16.0 h1:xWw16ngr6ZMtmxDyKyIgsE93KNKz5HKmMa3b8ALHidU=
-golang.org/x/sys v0.16.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
-golang.org/x/tools v0.17.0 h1:FvmRgNOcs3kOa+T20R1uhfP9F6HgG2mfxDv1vrx1Htc=
-golang.org/x/tools v0.17.0/go.mod h1:xsh6VxdV005rRVaS6SSAf9oiAqljS7UZUacMZ8Bnsps=
+golang.org/x/sys v0.18.0 h1:DBdB3niSjOA/O0blCZBqDefyWNYveAYMNF1Wum0DYQ4=
+golang.org/x/sys v0.18.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
+golang.org/x/tools v0.19.0 h1:tfGCXNR1OsFG+sVdLAitlpjAvD/I6dHDKnYrpEZUHkw=
+golang.org/x/tools v0.19.0/go.mod h1:qoJWxmGSIBmAeriMx19ogtrEPrGtDbPK634QFIcLAhc=
diff --git a/msg.go b/msg.go
index 8294d039..5fa7f9e8 100644
--- a/msg.go
+++ b/msg.go
@@ -714,7 +714,7 @@ func (h *MsgHdr) String() string {
 	return s
 }
 
-// Pack packs a Msg: it is converted to to wire format.
+// Pack packs a Msg: it is converted to wire format.
 // If the dns.Compress is true the message will be in compressed wire format.
 func (dns *Msg) Pack() (msg []byte, err error) {
 	return dns.PackBuffer(nil)
diff --git a/scan.go b/scan.go
index 315bf8a6..e26e8027 100644
--- a/scan.go
+++ b/scan.go
@@ -1283,7 +1283,7 @@ func stringToCm(token string) (e, m uint8, ok bool) {
 			cmeters *= 10
 		}
 	}
-	// This slighly ugly condition will allow omitting the 'meter' part, like .01 (meaning 0.01m = 1cm).
+	// This slightly ugly condition will allow omitting the 'meter' part, like .01 (meaning 0.01m = 1cm).
 	if !hasCM || mStr != "" {
 		meters, err = strconv.Atoi(mStr)
 		// RFC1876 states the max value is 90000000.00.  The latter two conditions enforce it.
diff --git a/scan_rr.go b/scan_rr.go
index 1a90c61f..7d1ade7d 100644
--- a/scan_rr.go
+++ b/scan_rr.go
@@ -51,25 +51,21 @@ func endingToTxtSlice(c *zlexer, errstr string) ([]string, *ParseError) {
 		switch l.value {
 		case zString:
 			empty = false
-			if len(l.token) > 255 {
-				// split up tokens that are larger than 255 into 255-chunks
-				sx := []string{}
-				p, i := 0, 255
-				for {
-					if i <= len(l.token) {
-						sx = append(sx, l.token[p:i])
-					} else {
-						sx = append(sx, l.token[p:])
-						break
-
-					}
-					p, i = p+255, i+255
+			// split up tokens that are larger than 255 into 255-chunks
+			sx := []string{}
+			p := 0
+			for {
+				i := escapedStringOffset(l.token[p:], 255)
+				if i != -1 && p+i != len(l.token) {
+					sx = append(sx, l.token[p:p+i])
+				} else {
+					sx = append(sx, l.token[p:])
+					break
+
 				}
-				s = append(s, sx...)
-				break
+				p += i
 			}
-
-			s = append(s, l.token)
+			s = append(s, sx...)
 		case zBlank:
 			if quote {
 				// zBlank can only be seen in between txt parts.
@@ -1920,3 +1916,32 @@ func (rr *APL) parse(c *zlexer, o string) *ParseError {
 	rr.Prefixes = prefixes
 	return nil
 }
+
+// escapedStringOffset finds the offset within a string (which may contain escape
+// sequences) that corresponds to a certain byte offset. If the input offset is
+// out of bounds, -1 is returned.
+func escapedStringOffset(s string, byteOffset int) int {
+	if byteOffset == 0 {
+		return 0
+	}
+
+	offset := 0
+	for i := 0; i < len(s); i++ {
+		offset += 1
+
+		// Skip escape sequences
+		if s[i] != '\\' {
+			// Not an escape sequence; nothing to do.
+		} else if isDDD(s[i+1:]) {
+			i += 3
+		} else {
+			i++
+		}
+
+		if offset >= byteOffset {
+			return i + 1
+		}
+	}
+
+	return -1
+}
diff --git a/xfr.go b/xfr.go
index 91080109..2187c456 100644
--- a/xfr.go
+++ b/xfr.go
@@ -188,7 +188,7 @@ func (t *Transfer) inIxfr(q *Msg, c chan *Envelope) {
 			if v, ok := rr.(*SOA); ok {
 				if v.Serial == serial {
 					n++
-					// quit if it's a full axfr or the the servers' SOA is repeated the third time
+					// quit if it's a full axfr or the servers' SOA is repeated the third time
 					if axfr && n == 2 || n == 3 {
 						c <- &Envelope{in.Answer, nil}
 						return


Test patch:
diff --git a/parse_test.go b/parse_test.go
index 1b8f5db4..da94cc38 100644
--- a/parse_test.go
+++ b/parse_test.go
@@ -1098,18 +1098,41 @@ func TestTXT(t *testing.T) {
 		}
 	}
 
-	// Test TXT record with chunk larger than 255 bytes, they should be split up, by the parser
-	s := ""
-	for i := 0; i < 255; i++ {
-		s += "a"
+	// Test TXT record with string larger than 255 bytes that should be split
+	// up by the parser. Add some escape sequences too to ensure their length
+	// is counted correctly.
+	s := `"\;\\\120` + strings.Repeat("a", 255) + `b"`
+	rr, err = NewRR(`test.local. 60 IN TXT ` + s)
+	if err != nil {
+		t.Error("failed to parse empty-string TXT record", err)
+	}
+	if rr.(*TXT).Txt[1] != "aaab" {
+		t.Errorf("Txt should have two strings, last one must be 'aaab', but is %s", rr.(*TXT).Txt[1])
 	}
-	s += "b"
-	rr, err = NewRR(`test.local. 60 IN TXT "` + s + `"`)
+	rrContent := strings.Replace(rr.String(), rr.Header().String(), "", 1)
+	expectedRRContent := `";\\x` + strings.Repeat("a", 252) + `" "aaab"`
+	if expectedRRContent != rrContent {
+		t.Errorf("Expected TXT RR content to be %#q but got %#q", expectedRRContent, rrContent)
+	}
+
+	// Test TXT record that is already split up into strings of len <= 255.
+	s = fmt.Sprintf(
+		"%q %q %q %q %q %q",
+		strings.Repeat(`a`, 255),
+		strings.Repeat("b", 255),
+		strings.Repeat("c", 255),
+		strings.Repeat("d", 0),
+		strings.Repeat("e", 1),
+		strings.Repeat("f", 123),
+	)
+	rr, err = NewRR(`test.local. 60 IN TXT ` + s)
 	if err != nil {
 		t.Error("failed to parse empty-string TXT record", err)
 	}
-	if rr.(*TXT).Txt[1] != "b" {
-		t.Errorf("Txt should have two chunk, last one my be 'b', but is %s", rr.(*TXT).Txt[1])
+	rrContent = strings.Replace(rr.String(), rr.Header().String(), "", 1)
+	expectedRRContent = s // same as input
+	if expectedRRContent != rrContent {
+		t.Errorf("Expected TXT RR content to be %#q but got %#q", expectedRRContent, rrContent)
 	}
 }
 
diff --git a/scan_test.go b/scan_test.go
index 207748b6..580236ea 100644
--- a/scan_test.go
+++ b/scan_test.go
@@ -427,3 +427,31 @@ func BenchmarkZoneParser(b *testing.B) {
 		}
 	}
 }
+
+func TestEscapedStringOffset(t *testing.T) {
+	var cases = []struct {
+		input          string
+		inputOffset    int
+		expectedOffset int
+	}{
+		{"simple string with no escape sequences", 20, 20},
+		{"simple string with no escape sequences", 500, -1},
+		{`\;\088\\\;\120\\`, 0, 0},
+		{`\;\088\\\;\120\\`, 1, 2},
+		{`\;\088\\\;\120\\`, 2, 6},
+		{`\;\088\\\;\120\\`, 3, 8},
+		{`\;\088\\\;\120\\`, 4, 10},
+		{`\;\088\\\;\120\\`, 5, 14},
+		{`\;\088\\\;\120\\`, 6, 16},
+		{`\;\088\\\;\120\\`, 7, -1},
+	}
+	for i, test := range cases {
+		outputOffset := escapedStringOffset(test.input, test.inputOffset)
+		if outputOffset != test.expectedOffset {
+			t.Errorf(
+				"Test %d (input %#q offset %d) returned offset %d but expected %d",
+				i, test.input, test.inputOffset, outputOffset, test.expectedOffset,
+			)
+		}
+	}
+}