Repository: miekg/dns
Pull request #1214 (2021-01-31 14:47:18)

Fix Rdlength related parsing bug in RFC3597 records
`(*RFC3597).fromRFC3597` failed to set `Rdlength` on the RR header. This is used (since yesterday) when parsing records from a zone file in the RFC 3597 "Unknown DNS Resource Record Type" format.

Also while we're here, validate the length doesn't exceed the maximum RDATA length when parsing in that format and harden `UnpackRRWithHeader`.

Updates #1211 (my bad)

/cc @catenacyber

tmthrgd: It's late here so if people could give it a good looking over, that would be appreciated. 
miekg: Hi @tmthrgd thanks for working on this. I think the most minimal fix is just 1 line, left couple of other comments as well.

The tests are def. /lgtm

Patch:
diff --git a/dns.go b/dns.go
index 33c93b2b..a88484b0 100644
--- a/dns.go
+++ b/dns.go
@@ -134,9 +134,14 @@ func (rr *RFC3597) ToRFC3597(r RR) error {
 
 // fromRFC3597 converts an unknown RR representation from RFC 3597 to the known RR type.
 func (rr *RFC3597) fromRFC3597(r RR) error {
-	*r.Header() = rr.Hdr
+	hdr := r.Header()
+	*hdr = rr.Hdr
 
-	if len(rr.Rdata) == 0 {
+	// Can't overflow uint16 as the length of Rdata is validated in (*RFC3597).parse.
+	// We can only get here when rr was constructed with that method.
+	hdr.Rdlength = uint16(hex.DecodedLen(len(rr.Rdata)))
+
+	if noRdata(*hdr) {
 		// Dynamic update.
 		return nil
 	}
diff --git a/scan_rr.go b/scan_rr.go
index 69f10052..23b4043b 100644
--- a/scan_rr.go
+++ b/scan_rr.go
@@ -1387,7 +1387,7 @@ func (rr *RFC3597) parse(c *zlexer, o string) *ParseError {
 
 	c.Next() // zBlank
 	l, _ = c.Next()
-	rdlength, e := strconv.Atoi(l.token)
+	rdlength, e := strconv.ParseUint(l.token, 10, 16)
 	if e != nil || l.err {
 		return &ParseError{"", "bad RFC3597 Rdata ", l}
 	}
@@ -1396,7 +1396,7 @@ func (rr *RFC3597) parse(c *zlexer, o string) *ParseError {
 	if e1 != nil {
 		return e1
 	}
-	if rdlength*2 != len(s) {
+	if int(rdlength)*2 != len(s) {
 		return &ParseError{"", "bad RFC3597 Rdata", l}
 	}
 	rr.Rdata = s


Test patch:
diff --git a/scan_test.go b/scan_test.go
index 08424e09..cc70171b 100644
--- a/scan_test.go
+++ b/scan_test.go
@@ -229,8 +229,23 @@ example.com. 60 PX (
 	}
 }
 
+func TestParseRFC3597InvalidLength(t *testing.T) {
+	// We need to space separate the 00s otherwise it will exceed the maximum token size
+	// of the zone lexer.
+	_, err := NewRR("example. 3600 CLASS1 TYPE1 \\# 65536 " + strings.Repeat("00 ", 65536))
+	if err == nil {
+		t.Error("should not have parsed excessively long RFC3579 record")
+	}
+}
+
 func TestParseKnownRRAsRFC3597(t *testing.T) {
 	t.Run("with RDATA", func(t *testing.T) {
+		// This was found by oss-fuzz.
+		_, err := NewRR("example. 3600 tYpe44 \\# 03 75  0100")
+		if err != nil {
+			t.Errorf("failed to parse RFC3579 format: %v", err)
+		}
+
 		rr, err := NewRR("example. 3600 CLASS1 TYPE1 \\# 4 7f000001")
 		if err != nil {
 			t.Fatalf("failed to parse RFC3579 format: %v", err)
@@ -247,7 +262,7 @@ func TestParseKnownRRAsRFC3597(t *testing.T) {
 
 		localhost := net.IPv4(127, 0, 0, 1)
 		if !a.A.Equal(localhost) {
-			t.Fatalf("expected A with IP %v, but got %v", localhost, a.A)
+			t.Errorf("expected A with IP %v, but got %v", localhost, a.A)
 		}
 	})
 	t.Run("without RDATA", func(t *testing.T) {
@@ -266,7 +281,7 @@ func TestParseKnownRRAsRFC3597(t *testing.T) {
 		}
 
 		if len(a.A) != 0 {
-			t.Fatalf("expected A with empty IP, but got %v", a.A)
+			t.Errorf("expected A with empty IP, but got %v", a.A)
 		}
 	})
 }