fix(netxlite): ensure we only accept DNS responses (#735)

Previously, the DNS decoder did not check whether it was parsing
a DNS query or a DNS response, which was wrong.

As a side note, it seems I am using "reply" in the codebase instead
of "response". The latter seems correct DNS terminology.

This diff has been extracted from 9249d14f80

See https://github.com/ooni/probe/issues/2096.

internal/model/netx.go

@@ -60,6 +60,9 @@ type DNSDecoder interface {
 	//
 	// - data is the raw reply
 	//
+	// This function fails if we cannot parse data as a DNS
+	// message or the message is not a reply.
+	//
 	// If you use this function, remember that:
 	//
 	// 1. the Rcode MAY be nonzero;

internal/netxlite/dnsdecoder.go

@@ -18,16 +18,32 @@ type DNSDecoderMiekg struct{}
 // ErrDNSReplyWithWrongQueryID indicates we have got a DNS reply with the wrong queryID.
 var ErrDNSReplyWithWrongQueryID = errors.New(FailureDNSReplyWithWrongQueryID)
 
+// ErrDNSIsQuery indicates that we were passed a DNS query.
+var ErrDNSIsQuery = errors.New("ooresolver: expected response but received query")
+
 // DecodeReply implements model.DNSDecoder.DecodeReply
 func (d *DNSDecoderMiekg) DecodeReply(data []byte) (*dns.Msg, error) {
-	reply := new(dns.Msg)
+	reply := &dns.Msg{}
 	if err := reply.Unpack(data); err != nil {
 		return nil, err
 	}
+	if !reply.Response {
+		return nil, ErrDNSIsQuery
+	}
 	return reply, nil
 }
 
-func (d *DNSDecoderMiekg) parseReply(data []byte, queryID uint16) (*dns.Msg, error) {
+// decodeSuccessfulReply decodes the bytes in data as a successful reply for the
+// given queryID. This function returns an error if:
+//
+// 1. we cannot decode data
+//
+// 2. the decoded message is not a reply
+//
+// 3. the query ID does not match
+//
+// 4. the Rcode is not zero.
+func (d *DNSDecoderMiekg) decodeSuccessfulReply(data []byte, queryID uint16) (*dns.Msg, error) {
 	reply, err := d.DecodeReply(data)
 	if err != nil {
 		return nil, err
@@ -52,7 +68,7 @@ func (d *DNSDecoderMiekg) parseReply(data []byte, queryID uint16) (*dns.Msg, err
 }
 
 func (d *DNSDecoderMiekg) DecodeHTTPS(data []byte, queryID uint16) (*model.HTTPSSvc, error) {
-	reply, err := d.parseReply(data, queryID)
+	reply, err := d.decodeSuccessfulReply(data, queryID)
 	if err != nil {
 		return nil, err
 	}
@@ -87,7 +103,7 @@ func (d *DNSDecoderMiekg) DecodeHTTPS(data []byte, queryID uint16) (*model.HTTPS
 }
 
 func (d *DNSDecoderMiekg) DecodeLookupHost(qtype uint16, data []byte, queryID uint16) ([]string, error) {
-	reply, err := d.parseReply(data, queryID)
+	reply, err := d.decodeSuccessfulReply(data, queryID)
 	if err != nil {
 		return nil, err
 	}
@@ -113,7 +129,7 @@ func (d *DNSDecoderMiekg) DecodeLookupHost(qtype uint16, data []byte, queryID ui
 }
 
 func (d *DNSDecoderMiekg) DecodeNS(data []byte, queryID uint16) ([]*net.NS, error) {
-	reply, err := d.parseReply(data, queryID)
+	reply, err := d.decodeSuccessfulReply(data, queryID)
 	if err != nil {
 		return nil, err
 	}

internal/netxlite/dnsdecoder_test.go

@@ -24,6 +24,19 @@ func TestDNSDecoder(t *testing.T) {
 			}
 		})
 
+		t.Run("with bytes containing a query", func(t *testing.T) {
+			d := &DNSDecoderMiekg{}
+			queryID := dns.Id()
+			rawQuery := dnsGenQuery(dns.TypeA, queryID)
+			addrs, err := d.DecodeLookupHost(dns.TypeA, rawQuery, queryID)
+			if !errors.Is(err, ErrDNSIsQuery) {
+				t.Fatal("unexpected err", err)
+			}
+			if len(addrs) > 0 {
+				t.Fatal("expected no addrs")
+			}
+		})
+
 		t.Run("wrong query ID", func(t *testing.T) {
 			d := &DNSDecoderMiekg{}
 			const (
@@ -157,15 +170,16 @@ func TestDNSDecoder(t *testing.T) {
 		})
 	})
 
-	t.Run("parseReply", func(t *testing.T) {
+	t.Run("decodeSuccessfulReply", func(t *testing.T) {
 		d := &DNSDecoderMiekg{}
 		msg := &dns.Msg{}
 		msg.Rcode = dns.RcodeFormatError // an rcode we don't handle
+		msg.Response = true
 		data, err := msg.Pack()
 		if err != nil {
 			t.Fatal(err)
 		}
-		reply, err := d.parseReply(data, 0)
+		reply, err := d.decodeSuccessfulReply(data, 0)
 		if !errors.Is(err, ErrOODNSMisbehaving) { // catch all error
 			t.Fatal("not the error we expected", err)
 		}
@@ -186,6 +200,19 @@ func TestDNSDecoder(t *testing.T) {
 			}
 		})
 
+		t.Run("with bytes containing a query", func(t *testing.T) {
+			d := &DNSDecoderMiekg{}
+			queryID := dns.Id()
+			rawQuery := dnsGenQuery(dns.TypeHTTPS, queryID)
+			https, err := d.DecodeHTTPS(rawQuery, queryID)
+			if !errors.Is(err, ErrDNSIsQuery) {
+				t.Fatal("unexpected err", err)
+			}
+			if https != nil {
+				t.Fatal("expected nil https")
+			}
+		})
+
 		t.Run("wrong query ID", func(t *testing.T) {
 			d := &DNSDecoderMiekg{}
 			const (
@@ -252,6 +279,19 @@ func TestDNSDecoder(t *testing.T) {
 			}
 		})
 
+		t.Run("with bytes containing a query", func(t *testing.T) {
+			d := &DNSDecoderMiekg{}
+			queryID := dns.Id()
+			rawQuery := dnsGenQuery(dns.TypeNS, queryID)
+			ns, err := d.DecodeNS(rawQuery, queryID)
+			if !errors.Is(err, ErrDNSIsQuery) {
+				t.Fatal("unexpected err", err)
+			}
+			if len(ns) > 0 {
+				t.Fatal("expected no result")
+			}
+		})
+
 		t.Run("wrong query ID", func(t *testing.T) {
 			d := &DNSDecoderMiekg{}
 			const (