Use golang.org/x/crypto/cryptobyte to unpack DNS records

client.go

@@ -10,6 +10,8 @@ import (
 	"net"
 	"strings"
 	"time"
+
+	"golang.org/x/crypto/cryptobyte"
 )
 
 const (
@@ -274,7 +276,6 @@ func (co *Conn) ReadMsgHeader(hdr *Header) ([]byte, error) {
 		n   int
 		err error
 	)
-
 	if isPacketConn(co.Conn) {
 		if co.UDPSize > MinMsgSize {
 			p = make([]byte, co.UDPSize)
@@ -291,22 +292,23 @@ func (co *Conn) ReadMsgHeader(hdr *Header) ([]byte, error) {
 		p = make([]byte, length)
 		n, err = io.ReadFull(co.Conn, p)
 	}
+	p = p[:n]
 
 	if err != nil {
 		return nil, err
-	} else if n < headerSize {
+	} else if len(p) < headerSize {
 		return nil, ErrShortRead
 	}
 
-	p = p[:n]
-	if hdr != nil {
-		dh, _, err := unpackMsgHdr(p, 0)
-		if err != nil {
-			return nil, err
-		}
-		*hdr = dh
+	s := cryptobyte.String(p)
+	if hdr != nil && !hdr.unpack(&s) {
+		// unpack only ever reports false if the String is too short, we
+		// check exactly that by comparing against headerSize above. This
+		// panic can never happen.
+		panic("dns: internal error: failed to unpack message header")
 	}
-	return p, err
+
+	return p, nil
 }
 
 // Read implements the net.Conn read method.

client_test.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"crypto/tls"
 	"errors"
-	"fmt"
 	"net"
 	"strconv"
 	"strings"
@@ -507,138 +506,6 @@ func TestClientConnWriteSinglePacket(t *testing.T) {
 	}
 }
 
-func TestTruncatedMsg(t *testing.T) {
-	m := new(Msg)
-	m.SetQuestion("miek.nl.", TypeSRV)
-	cnt := 10
-	for i := 0; i < cnt; i++ {
-		r := &SRV{
-			Hdr:    RR_Header{Name: m.Question[0].Name, Rrtype: TypeSRV, Class: ClassINET, Ttl: 0},
-			Port:   uint16(i + 8000),
-			Target: "target.miek.nl.",
-		}
-		m.Answer = append(m.Answer, r)
-
-		re := &A{
-			Hdr: RR_Header{Name: m.Question[0].Name, Rrtype: TypeA, Class: ClassINET, Ttl: 0},
-			A:   net.ParseIP(fmt.Sprintf("127.0.0.%d", i)).To4(),
-		}
-		m.Extra = append(m.Extra, re)
-	}
-	buf, err := m.Pack()
-	if err != nil {
-		t.Errorf("failed to pack: %v", err)
-	}
-
-	r := new(Msg)
-	if err = r.Unpack(buf); err != nil {
-		t.Errorf("unable to unpack message: %v", err)
-	}
-	if len(r.Answer) != cnt {
-		t.Errorf("answer count after regular unpack doesn't match: %d", len(r.Answer))
-	}
-	if len(r.Extra) != cnt {
-		t.Errorf("extra count after regular unpack doesn't match: %d", len(r.Extra))
-	}
-
-	m.Truncated = true
-	buf, err = m.Pack()
-	if err != nil {
-		t.Errorf("failed to pack truncated message: %v", err)
-	}
-
-	r = new(Msg)
-	if err = r.Unpack(buf); err != nil {
-		t.Errorf("failed to unpack truncated message: %v", err)
-	}
-	if !r.Truncated {
-		t.Errorf("truncated message wasn't unpacked as truncated")
-	}
-	if len(r.Answer) != cnt {
-		t.Errorf("answer count after truncated unpack doesn't match: %d", len(r.Answer))
-	}
-	if len(r.Extra) != cnt {
-		t.Errorf("extra count after truncated unpack doesn't match: %d", len(r.Extra))
-	}
-
-	// Now we want to remove almost all of the extra records
-	// We're going to loop over the extra to get the count of the size of all
-	// of them
-	off := 0
-	buf1 := make([]byte, m.Len())
-	for i := 0; i < len(m.Extra); i++ {
-		off, err = PackRR(m.Extra[i], buf1, off, nil, m.Compress)
-		if err != nil {
-			t.Errorf("failed to pack extra: %v", err)
-		}
-	}
-
-	// Remove all of the extra bytes but 10 bytes from the end of buf
-	off -= 10
-	buf1 = buf[:len(buf)-off]
-
-	r = new(Msg)
-	if err = r.Unpack(buf1); err == nil {
-		t.Error("cutoff message should have failed to unpack")
-	}
-	// r's header might be still usable.
-	if !r.Truncated {
-		t.Error("truncated cutoff message wasn't unpacked as truncated")
-	}
-	if len(r.Answer) != cnt {
-		t.Errorf("answer count after cutoff unpack doesn't match: %d", len(r.Answer))
-	}
-	if len(r.Extra) != 0 {
-		t.Errorf("extra count after cutoff unpack is not zero: %d", len(r.Extra))
-	}
-
-	// Now we want to remove almost all of the answer records too
-	buf1 = make([]byte, m.Len())
-	as := 0
-	for i := 0; i < len(m.Extra); i++ {
-		off1 := off
-		off, err = PackRR(m.Extra[i], buf1, off, nil, m.Compress)
-		as = off - off1
-		if err != nil {
-			t.Errorf("failed to pack extra: %v", err)
-		}
-	}
-
-	// Keep exactly one answer left
-	// This should still cause Answer to be nil
-	off -= as
-	buf1 = buf[:len(buf)-off]
-
-	r = new(Msg)
-	if err = r.Unpack(buf1); err == nil {
-		t.Error("cutoff message should have failed to unpack")
-	}
-	if !r.Truncated {
-		t.Error("truncated cutoff message wasn't unpacked as truncated")
-	}
-	if len(r.Answer) != 0 {
-		t.Errorf("answer count after second cutoff unpack is not zero: %d", len(r.Answer))
-	}
-
-	// Now leave only 1 byte of the question
-	// Since the header is always 12 bytes, we just need to keep 13
-	buf1 = buf[:13]
-
-	r = new(Msg)
-	err = r.Unpack(buf1)
-	if err == nil {
-		t.Errorf("error should be nil after question cutoff unpack: %v", err)
-	}
-
-	// Finally, if we only have the header, we don't return an error.
-	buf1 = buf[:12]
-
-	r = new(Msg)
-	if err = r.Unpack(buf1); err != nil {
-		t.Errorf("from header-only unpack should not return an error: %v", err)
-	}
-}
-
 func TestTimeout(t *testing.T) {
 	// Set up a dummy UDP server that won't respond
 	addr, err := net.ResolveUDPAddr("udp", ":0")

dns.go

@@ -52,7 +52,7 @@ type RR interface {
 	//
 	// This will only be called on a new and empty RR type with only the header populated. It
 	// will only be called if the record's RDATA is non-empty.
-	unpack(msg []byte, off int) (off1 int, err error)
+	unpack(data, msgBuf []byte) error
 
 	// parse parses an RR from zone file format.
 	//
@@ -104,7 +104,7 @@ func (h *RR_Header) pack(msg []byte, off int, compression compressionMap, compre
 	return off, nil
 }
 
-func (h *RR_Header) unpack(msg []byte, off int) (int, error) {
+func (h *RR_Header) unpack(data, msgBuf []byte) error {
 	panic("dns: internal error: unpack should never be called on RR_Header")
 }
 
@@ -124,12 +124,11 @@ func (rr *RFC3597) ToRFC3597(r RR) error {
 	*rr = RFC3597{Hdr: *r.Header()}
 	rr.Hdr.Rdlength = uint16(off - headerEnd)
 
-	if noRdata(rr.Hdr) {
+	if rr.Hdr.Rdlength == 0 {
 		return nil
 	}
 
-	_, err = rr.unpack(buf, headerEnd)
-	return err
+	return rr.unpack(buf[headerEnd:], buf)
 }
 
 // fromRFC3597 converts an unknown RR representation from RFC 3597 to the known RR type.
@@ -141,7 +140,7 @@ func (rr *RFC3597) fromRFC3597(r RR) error {
 	// We can only get here when rr was constructed with that method.
 	hdr.Rdlength = uint16(hex.DecodedLen(len(rr.Rdata)))
 
-	if noRdata(*hdr) {
+	if hdr.Rdlength == 0 {
 		// Dynamic update.
 		return nil
 	}
@@ -153,6 +152,5 @@ func (rr *RFC3597) fromRFC3597(r RR) error {
 		return err
 	}
 
-	_, err = r.unpack(msg, 0)
-	return err
+	return r.unpack(msg, msg)
 }

dns_bench_test.go

@@ -167,6 +167,20 @@ func BenchmarkUnpackDomainNameLongestUnprintable(b *testing.B) {
 	}
 }
 
+func BenchmarkUnpackDomainNamePointers(b *testing.B) {
+	msg := []byte("\x07example\x03com\x00" + "\x04test\xC0\x00" + "\x09benchmark\xC0\x0D" + "\x10UnpackDomainName\xC0\x14")
+	expect := "UnpackDomainName.benchmark.test.example.com."
+
+	for n := 0; n < b.N; n++ {
+		name, _, err := UnpackDomainName(msg, 32)
+		if err != nil {
+			b.Fatalf("UnpackDomainName failed: %v", err)
+		} else if name != expect {
+			b.Fatalf("UnpackDomainName produced wrong value; wanted %s, got %s", expect, name)
+		}
+	}
+}
+
 func BenchmarkCopy(b *testing.B) {
 	b.ReportAllocs()
 	m := new(Msg)

dns_test.go

@@ -156,6 +156,8 @@ func TestToRFC3597(t *testing.T) {
 }
 
 func TestNoRdataPack(t *testing.T) {
+	// TODO(tmthrgd): This test isn't correct. See TestNoRdataUnpack.
+
 	data := make([]byte, 1024)
 	for typ, fn := range TypeToRR {
 		r := fn()
@@ -168,6 +170,16 @@ func TestNoRdataPack(t *testing.T) {
 }
 
 func TestNoRdataUnpack(t *testing.T) {
+	// TODO(tmthrgd): This test isn't correct and we sometimes don't correctly
+	// pack the messages. It currently succeeds as we explicitly handle certain
+	// broken records in their unpack method, but we shouldn't be. See the TODO
+	// in msg_generate.go. In fact we don't actually produce RRs with an
+	// RDLENGTH of zero, but rather one that just happens to contain mostly
+	// nothing. Further this isn't even how we ourselves construct dynamic
+	// update messages, in update.go we use ANY for when producing an update
+	// message. This correctly produces an record with a type, class, TTL, etc.
+	// but with an RDLENGTH of zero.
+
 	data := make([]byte, 1024)
 	for typ, fn := range TypeToRR {
 		if typ == TypeSOA || typ == TypeTSIG || typ == TypeTKEY {
@@ -254,8 +266,7 @@ func TestMsgPackBuffer(t *testing.T) {
 		input, _ := hex.DecodeString(hexData)
 		m := new(Msg)
 		if err := m.Unpack(input); err != nil {
-			t.Errorf("packet %d failed to unpack", i)
-			continue
+			t.Errorf("packet %d failed to unpack: %v", i, err)
 		}
 	}
 }