Repository: miekg/dns
Pull request #1568 (2024-06-01 15:34:27)

Add a hook to catch invalid messages
Currently there are hooks for reading messages off the wire (DecorateReader), checking if they comply with policy (MsgAcceptFunc), and generating responses (Handler).  However, there is no hook that notifies the server when a message is dropped or rejected due to a syntax error.  That makes it hard to monitor these packets without repeating the parsing process.

This PR adds a hook for notifications about invalid packets.


Patch:
diff --git a/go.mod b/go.mod
index bd188208..2f8e569b 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.22.0
-	golang.org/x/sync v0.6.0
-	golang.org/x/sys v0.18.0
-	golang.org/x/tools v0.19.0
+	golang.org/x/net v0.26.0
+	golang.org/x/sync v0.7.0
+	golang.org/x/sys v0.21.0
+	golang.org/x/tools v0.22.0
 )
 
-require golang.org/x/mod v0.16.0 // indirect
+require golang.org/x/mod v0.18.0 // indirect
diff --git a/go.sum b/go.sum
index d0f87415..1e3f4ba0 100644
--- a/go.sum
+++ b/go.sum
@@ -1,10 +1,10 @@
-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.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=
+golang.org/x/mod v0.18.0 h1:5+9lSbEzPSdWkH32vYPBwEpX8KwDbM52Ud9xBUvNlb0=
+golang.org/x/mod v0.18.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
+golang.org/x/net v0.26.0 h1:soB7SVo0PWrY4vPW/+ay0jKDNScG2X9wFeYlXIvJsOQ=
+golang.org/x/net v0.26.0/go.mod h1:5YKkiSynbBIh3p6iOc/vibscux0x38BZDkn8sCUPxHE=
+golang.org/x/sync v0.7.0 h1:YsImfSBoP9QPYL0xyKJPq0gcaJdG3rInoqxTWbfQu9M=
+golang.org/x/sync v0.7.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
+golang.org/x/sys v0.21.0 h1:rF+pYz3DAGSQAxAu1CbC7catZg4ebC4UIeIhKxBZvws=
+golang.org/x/sys v0.21.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
+golang.org/x/tools v0.22.0 h1:gqSGLZqv+AI9lIQzniJ0nZDRG5GBPsSi+DRNHWNz6yA=
+golang.org/x/tools v0.22.0/go.mod h1:aCwcsjqvq7Yqt6TNyX7QMU2enbQ/Gt0bo6krSeEri+c=
diff --git a/scan_rr.go b/scan_rr.go
index 7d1ade7d..c1a76995 100644
--- a/scan_rr.go
+++ b/scan_rr.go
@@ -55,7 +55,10 @@ func endingToTxtSlice(c *zlexer, errstr string) ([]string, *ParseError) {
 			sx := []string{}
 			p := 0
 			for {
-				i := escapedStringOffset(l.token[p:], 255)
+				i, ok := escapedStringOffset(l.token[p:], 255)
+				if !ok {
+					return nil, &ParseError{err: errstr, lex: l}
+				}
 				if i != -1 && p+i != len(l.token) {
 					sx = append(sx, l.token[p:p+i])
 				} else {
@@ -1919,29 +1922,36 @@ func (rr *APL) parse(c *zlexer, o string) *ParseError {
 
 // 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
+// out of bounds, -1 is returned (which is *not* considered an error).
+func escapedStringOffset(s string, desiredByteOffset int) (int, bool) {
+	if desiredByteOffset == 0 {
+		return 0, true
 	}
 
-	offset := 0
-	for i := 0; i < len(s); i++ {
-		offset += 1
+	currentByteOffset, i := 0, 0
+
+	for i < len(s) {
+		currentByteOffset += 1
 
 		// Skip escape sequences
 		if s[i] != '\\' {
-			// Not an escape sequence; nothing to do.
+			// Single plain byte, not an escape sequence.
+			i++
 		} else if isDDD(s[i+1:]) {
-			i += 3
+			// Skip backslash and DDD.
+			i += 4
+		} else if len(s[i+1:]) < 1 {
+			// No character following the backslash; that's an error.
+			return 0, false
 		} else {
-			i++
+			// Skip backslash and following byte.
+			i += 2
 		}
 
-		if offset >= byteOffset {
-			return i + 1
+		if currentByteOffset >= desiredByteOffset {
+			return i, true
 		}
 	}
 
-	return -1
+	return -1, true
 }
diff --git a/server.go b/server.go
index 0207d6da..2f765564 100644
--- a/server.go
+++ b/server.go
@@ -188,6 +188,14 @@ type DecorateReader func(Reader) Reader
 // Implementations should never return a nil Writer.
 type DecorateWriter func(Writer) Writer
 
+// InvalidMsgFunc is a listener hook for observing incoming messages that were discarded
+// because they could not be parsed.
+// Every message that is read by a Reader will eventually be provided to the Handler,
+// rejected (or ignored) by the MsgAcceptFunc, or passed to this function.
+type InvalidMsgFunc func(m []byte, err error)
+
+func DefaultMsgInvalidFunc(m []byte, err error) {}
+
 // A Server defines parameters for running an DNS server.
 type Server struct {
 	// Address to listen on, ":dns" if empty.
@@ -233,6 +241,8 @@ type Server struct {
 	// AcceptMsgFunc will check the incoming message and will reject it early in the process.
 	// By default DefaultMsgAcceptFunc will be used.
 	MsgAcceptFunc MsgAcceptFunc
+	// MsgInvalidFunc is optional, will be called if a message is received but cannot be parsed.
+	MsgInvalidFunc InvalidMsgFunc
 
 	// Shutdown handling
 	lock     sync.RWMutex
@@ -277,6 +287,9 @@ func (srv *Server) init() {
 	if srv.MsgAcceptFunc == nil {
 		srv.MsgAcceptFunc = DefaultMsgAcceptFunc
 	}
+	if srv.MsgInvalidFunc == nil {
+		srv.MsgInvalidFunc = DefaultMsgInvalidFunc
+	}
 	if srv.Handler == nil {
 		srv.Handler = DefaultServeMux
 	}
@@ -531,6 +544,7 @@ func (srv *Server) serveUDP(l net.PacketConn) error {
 			if cap(m) == srv.UDPSize {
 				srv.udpPool.Put(m[:srv.UDPSize])
 			}
+			srv.MsgInvalidFunc(m, ErrShortRead)
 			continue
 		}
 		wg.Add(1)
@@ -611,6 +625,7 @@ func (srv *Server) serveUDPPacket(wg *sync.WaitGroup, m []byte, u net.PacketConn
 func (srv *Server) serveDNS(m []byte, w *response) {
 	dh, off, err := unpackMsgHdr(m, 0)
 	if err != nil {
+		srv.MsgInvalidFunc(m, err)
 		// Let client hang, they are sending crap; any reply can be used to amplify.
 		return
 	}
@@ -620,10 +635,12 @@ func (srv *Server) serveDNS(m []byte, w *response) {
 
 	switch action := srv.MsgAcceptFunc(dh); action {
 	case MsgAccept:
-		if req.unpack(dh, m, off) == nil {
+		err := req.unpack(dh, m, off)
+		if err == nil {
 			break
 		}
 
+		srv.MsgInvalidFunc(m, err)
 		fallthrough
 	case MsgReject, MsgRejectNotImplemented:
 		opcode := req.Opcode
diff --git a/xfr.go b/xfr.go
index 2187c456..5cfbb516 100644
--- a/xfr.go
+++ b/xfr.go
@@ -209,6 +209,7 @@ func (t *Transfer) inIxfr(q *Msg, c chan *Envelope) {
 //	ch := make(chan *dns.Envelope)
 //	tr := new(dns.Transfer)
 //	var wg sync.WaitGroup
+//	wg.Add(1)
 //	go func() {
 //		tr.Out(w, r, ch)
 //		wg.Done()


Test patch:
diff --git a/acceptfunc_test.go b/acceptfunc_test.go
index d40d4e4c..868154d4 100644
--- a/acceptfunc_test.go
+++ b/acceptfunc_test.go
@@ -1,6 +1,8 @@
 package dns
 
 import (
+	"encoding/binary"
+	"net"
 	"testing"
 )
 
@@ -33,3 +35,86 @@ func handleNotify(w ResponseWriter, req *Msg) {
 	m.SetReply(req)
 	w.WriteMsg(m)
 }
+
+func TestInvalidMsg(t *testing.T) {
+	HandleFunc("example.org.", func(ResponseWriter, *Msg) {
+		t.Fatal("the handler must not be called in any of these tests")
+	})
+	s, addrstr, _, err := RunLocalTCPServer(":0")
+	if err != nil {
+		t.Fatalf("unable to run test server: %v", err)
+	}
+	defer s.Shutdown()
+
+	s.MsgAcceptFunc = func(dh Header) MsgAcceptAction {
+		switch dh.Id {
+		case 0x0001:
+			return MsgAccept
+		case 0x0002:
+			return MsgReject
+		case 0x0003:
+			return MsgIgnore
+		case 0x0004:
+			return MsgRejectNotImplemented
+		default:
+			t.Errorf("unexpected ID %x", dh.Id)
+			return -1
+		}
+	}
+
+	invalidErrors := make(chan error)
+	s.MsgInvalidFunc = func(m []byte, err error) {
+		invalidErrors <- err
+	}
+
+	c, err := net.Dial("tcp", addrstr)
+	if err != nil {
+		t.Fatalf("cannot connect to test server: %v", err)
+	}
+
+	write := func(m []byte) {
+		var length [2]byte
+		binary.BigEndian.PutUint16(length[:], uint16(len(m)))
+		_, err := c.Write(length[:])
+		if err != nil {
+			t.Fatalf("length write failed: %v", err)
+		}
+		_, err = c.Write(m)
+		if err != nil {
+			t.Fatalf("content write failed: %v", err)
+		}
+	}
+
+	/* Message is too short, so there is no header to accept or reject. */
+
+	tooShortMessage := make([]byte, 11)
+	tooShortMessage[1] = 0x3 // ID = 3, would be ignored if it were parsable.
+
+	write(tooShortMessage)
+	// Expect an error to be reported.
+	<-invalidErrors
+
+	/* Message is accepted but is actually invalid. */
+
+	badMessage := make([]byte, 13)
+	badMessage[1] = 0x1 // ID = 1, Accept.
+	badMessage[5] = 1   // QDCOUNT = 1
+	badMessage[12] = 99 // Bad question section.  Invalid!
+
+	write(badMessage)
+	// Expect an error to be reported.
+	<-invalidErrors
+
+	/* Message is rejected before it can be determined to be invalid. */
+
+	close(invalidErrors) // A call to InvalidMsgFunc would panic due to the closed chan.
+
+	badMessage[1] = 0x2 // ID = 2, Reject
+	write(badMessage)
+
+	badMessage[1] = 0x3 // ID = 3, Ignore
+	write(badMessage)
+
+	badMessage[1] = 0x4 // ID = 4, RejectNotImplemented
+	write(badMessage)
+}
diff --git a/scan_test.go b/scan_test.go
index 580236ea..c4f7e7f4 100644
--- a/scan_test.go
+++ b/scan_test.go
@@ -433,25 +433,37 @@ func TestEscapedStringOffset(t *testing.T) {
 		input          string
 		inputOffset    int
 		expectedOffset int
+		expectedOK     bool
 	}{
-		{"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},
+		{"simple string with no escape sequences", 20, 20, true},
+		{"simple string with no escape sequences", 500, -1, true},
+		{`\;\088\\\;\120\\`, 0, 0, true},
+		{`\;\088\\\;\120\\`, 1, 2, true},
+		{`\;\088\\\;\120\\`, 2, 6, true},
+		{`\;\088\\\;\120\\`, 3, 8, true},
+		{`\;\088\\\;\120\\`, 4, 10, true},
+		{`\;\088\\\;\120\\`, 5, 14, true},
+		{`\;\088\\\;\120\\`, 6, 16, true},
+		{`\;\088\\\;\120\\`, 7, -1, true},
+		{`\`, 3, 0, false},
+		{`a\`, 3, 0, false},
+		{`aa\`, 3, 0, false},
+		{`aaa\`, 3, 3, true},
+		{`aaaa\`, 3, 3, true},
 	}
 	for i, test := range cases {
-		outputOffset := escapedStringOffset(test.input, test.inputOffset)
+		outputOffset, outputOK := 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,
 			)
 		}
+		if outputOK != test.expectedOK {
+			t.Errorf(
+				"Test %d (input %#q offset %d) returned ok=%t but expected %t",
+				i, test.input, test.inputOffset, outputOK, test.expectedOK,
+			)
+		}
 	}
 }