Repository: miekg/dns
Pull request #1215 (2021-02-01 02:07:31)

Validate Rdlength and off in UnpackRRWithHeader
This was split out of #1214.

I don't think this can be hit from `UnpackRR` as `unpackHeader` does validate it doesn't exceed `msg`, but it can definitely be hit by calling `UnpackRRWithHeader` directly.

tmthrgd: The `off` check doesn't seem to be done anywhere else `off` is taken so I'm not sure whether it should be introduced here or not, happy to back that out.

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/msg.go b/msg.go
index 7001f6da..1728a98b 100644
--- a/msg.go
+++ b/msg.go
@@ -624,11 +624,18 @@ func UnpackRRWithHeader(h RR_Header, msg []byte, off int) (rr RR, off1 int, err
 		rr = &RFC3597{Hdr: h}
 	}
 
-	if noRdata(h) {
-		return rr, off, nil
+	if off < 0 || off > len(msg) {
+		return &h, off, &Error{err: "bad off"}
 	}
 
 	end := off + int(h.Rdlength)
+	if end < off || end > len(msg) {
+		return &h, end, &Error{err: "bad rdlength"}
+	}
+
+	if noRdata(h) {
+		return rr, off, nil
+	}
 
 	off, err = rr.unpack(msg, off)
 	if err != 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/parse_test.go b/parse_test.go
index 886f46cc..1895506e 100644
--- a/parse_test.go
+++ b/parse_test.go
@@ -1884,3 +1884,41 @@ func TestParseAPLErrors(t *testing.T) {
 		})
 	}
 }
+
+func TestUnpackRRWithHeaderInvalidLengths(t *testing.T) {
+	rr, err := NewRR("test.example.org. 300 IN SSHFP 1 2 BC6533CDC95A79078A39A56EA7635984ED655318ADA9B6159E30723665DA95BB")
+	if err != nil {
+		t.Fatalf("failed to parse SSHFP record: %v", err)
+	}
+
+	buf := make([]byte, Len(rr))
+	headerEnd, end, err := packRR(rr, buf, 0, compressionMap{}, false)
+	if err != nil {
+		t.Fatalf("failed to pack A record: %v", err)
+	}
+
+	rr.Header().Rdlength = uint16(end - headerEnd)
+	for _, off := range []int{
+		-1,
+		end + 1,
+		1<<16 - 1,
+	} {
+		_, _, err := UnpackRRWithHeader(*rr.Header(), buf, off)
+		if de, ok := err.(*Error); !ok || de.err != "bad off" {
+			t.Errorf("UnpackRRWithHeader with bad offset (%d) returned wrong or no error: %v", off, err)
+		}
+	}
+
+	for _, rdlength := range []uint16{
+		uint16(end - headerEnd + 1),
+		uint16(end),
+		1<<16 - 1,
+	} {
+		rr.Header().Rdlength = rdlength
+
+		_, _, err := UnpackRRWithHeader(*rr.Header(), buf, headerEnd)
+		if de, ok := err.(*Error); !ok || de.err != "bad rdlength" {
+			t.Errorf("UnpackRRWithHeader with bad rdlength (%d) returned wrong or no error: %v", rdlength, err)
+		}
+	}
+}
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)
 		}
 	})
 }