fix(netxlite): reject replies with wrong queryID (#732)

This diff has been extracted from bassosimone/ooni-2022-04-websteps-illustrated@c2f7cca

See ooni/probe#2096

While there, export DecodeReply to decode a raw reply without
interpreting the Rcode or parsing the results, which seems a
nice extra feature to have to more flexibly parse DNS replies
in other parts of the codebase.

internal/model/mocks/dnsdecoder.go

@@ -1,20 +1,30 @@
 package mocks
 
-import "github.com/ooni/probe-cli/v3/internal/model"
+import (
+	"github.com/miekg/dns"
+	"github.com/ooni/probe-cli/v3/internal/model"
+)
 
 // DNSDecoder allows mocking dnsx.DNSDecoder.
 type DNSDecoder struct {
-	MockDecodeLookupHost func(qtype uint16, reply []byte) ([]string, error)
+	MockDecodeLookupHost func(qtype uint16, reply []byte, queryID uint16) ([]string, error)
 
-	MockDecodeHTTPS func(reply []byte) (*model.HTTPSSvc, error)
+	MockDecodeHTTPS func(reply []byte, queryID uint16) (*model.HTTPSSvc, error)
+
+	MockDecodeReply func(reply []byte) (*dns.Msg, error)
 }
 
 // DecodeLookupHost calls MockDecodeLookupHost.
-func (e *DNSDecoder) DecodeLookupHost(qtype uint16, reply []byte) ([]string, error) {
-	return e.MockDecodeLookupHost(qtype, reply)
+func (e *DNSDecoder) DecodeLookupHost(qtype uint16, reply []byte, queryID uint16) ([]string, error) {
+	return e.MockDecodeLookupHost(qtype, reply, queryID)
 }
 
 // DecodeHTTPS calls MockDecodeHTTPS.
-func (e *DNSDecoder) DecodeHTTPS(reply []byte) (*model.HTTPSSvc, error) {
-	return e.MockDecodeHTTPS(reply)
+func (e *DNSDecoder) DecodeHTTPS(reply []byte, queryID uint16) (*model.HTTPSSvc, error) {
+	return e.MockDecodeHTTPS(reply, queryID)
+}
+
+// DecodeReply calls MockDecodeReply.
+func (e *DNSDecoder) DecodeReply(reply []byte) (*dns.Msg, error) {
+	return e.MockDecodeReply(reply)
 }

internal/model/mocks/dnsdecoder_test.go

@@ -12,11 +12,11 @@ func TestDNSDecoder(t *testing.T) {
 	t.Run("DecodeLookupHost", func(t *testing.T) {
 		expected := errors.New("mocked error")
 		e := &DNSDecoder{
-			MockDecodeLookupHost: func(qtype uint16, reply []byte) ([]string, error) {
+			MockDecodeLookupHost: func(qtype uint16, reply []byte, queryID uint16) ([]string, error) {
 				return nil, expected
 			},
 		}
-		out, err := e.DecodeLookupHost(dns.TypeA, make([]byte, 17))
+		out, err := e.DecodeLookupHost(dns.TypeA, make([]byte, 17), dns.Id())
 		if !errors.Is(err, expected) {
 			t.Fatal("unexpected err", err)
 		}
@@ -28,11 +28,27 @@ func TestDNSDecoder(t *testing.T) {
 	t.Run("DecodeHTTPS", func(t *testing.T) {
 		expected := errors.New("mocked error")
 		e := &DNSDecoder{
-			MockDecodeHTTPS: func(reply []byte) (*model.HTTPSSvc, error) {
+			MockDecodeHTTPS: func(reply []byte, queryID uint16) (*model.HTTPSSvc, error) {
 				return nil, expected
 			},
 		}
-		out, err := e.DecodeHTTPS(make([]byte, 17))
+		out, err := e.DecodeHTTPS(make([]byte, 17), dns.Id())
+		if !errors.Is(err, expected) {
+			t.Fatal("unexpected err", err)
+		}
+		if out != nil {
+			t.Fatal("unexpected out")
+		}
+	})
+
+	t.Run("DecodeReply", func(t *testing.T) {
+		expected := errors.New("mocked error")
+		e := &DNSDecoder{
+			MockDecodeReply: func(reply []byte) (*dns.Msg, error) {
+				return nil, expected
+			},
+		}
+		out, err := e.DecodeReply(make([]byte, 17))
 		if !errors.Is(err, expected) {
 			t.Fatal("unexpected err", err)
 		}

internal/model/mocks/dnsencoder.go

@@ -2,10 +2,10 @@ package mocks
 
 // DNSEncoder allows mocking dnsx.DNSEncoder.
 type DNSEncoder struct {
-	MockEncode func(domain string, qtype uint16, padding bool) ([]byte, error)
+	MockEncode func(domain string, qtype uint16, padding bool) ([]byte, uint16, error)
 }
 
 // Encode calls MockEncode.
-func (e *DNSEncoder) Encode(domain string, qtype uint16, padding bool) ([]byte, error) {
+func (e *DNSEncoder) Encode(domain string, qtype uint16, padding bool) ([]byte, uint16, error) {
 	return e.MockEncode(domain, qtype, padding)
 }

internal/model/mocks/dnsencoder_test.go

@@ -11,16 +11,19 @@ func TestDNSEncoder(t *testing.T) {
 	t.Run("Encode", func(t *testing.T) {
 		expected := errors.New("mocked error")
 		e := &DNSEncoder{
-			MockEncode: func(domain string, qtype uint16, padding bool) ([]byte, error) {
-				return nil, expected
+			MockEncode: func(domain string, qtype uint16, padding bool) ([]byte, uint16, error) {
+				return nil, 0, expected
 			},
 		}
-		out, err := e.Encode("dns.google", dns.TypeA, true)
+		out, queryID, err := e.Encode("dns.google", dns.TypeA, true)
 		if !errors.Is(err, expected) {
 			t.Fatal("unexpected err", err)
 		}
 		if out != nil {
 			t.Fatal("unexpected out")
 		}
+		if queryID != 0 {
+			t.Fatal("unexpected queryID")
+		}
 	})
 }

internal/model/mocks/http_test.go

@@ -9,6 +9,18 @@ import (
 )
 
 func TestHTTPTransport(t *testing.T) {
+	t.Run("Network", func(t *testing.T) {
+		expected := "quic"
+		txp := &HTTPTransport{
+			MockNetwork: func() string {
+				return expected
+			},
+		}
+		if txp.Network() != expected {
+			t.Fatal("unexpected network value")
+		}
+	})
+
 	t.Run("RoundTrip", func(t *testing.T) {
 		expected := errors.New("mocked error")
 		txp := &HTTPTransport{

internal/model/netx.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/lucas-clemente/quic-go"
+	"github.com/miekg/dns"
 )
 
 //
@@ -25,6 +26,8 @@ type DNSDecoder interface {
 	//
 	// - data contains the reply bytes read from a DNSTransport
 	//
+	// - queryID is the original query ID
+	//
 	// Returns:
 	//
 	// - on success, a list of IP addrs inside the reply and a nil error
@@ -33,9 +36,9 @@ type DNSDecoder interface {
 	//
 	// Note that this function will return an error if there is no
 	// IP address inside of the reply.
-	DecodeLookupHost(qtype uint16, data []byte) ([]string, error)
+	DecodeLookupHost(qtype uint16, data []byte, queryID uint16) ([]string, error)
 
-	// DecodeHTTPS decodes an HTTPS reply.
+	// DecodeHTTPS is like DecodeLookupHost but decodes an HTTPS reply.
 	//
 	// The argument is the reply as read by the DNSTransport.
 	//
@@ -46,7 +49,22 @@ type DNSDecoder interface {
 	// This function will return an error if the HTTPS reply does not
 	// contain at least a valid ALPN entry. It will not return
 	// an error, though, when there are no IPv4/IPv6 hints in the reply.
-	DecodeHTTPS(data []byte) (*HTTPSSvc, error)
+	DecodeHTTPS(data []byte, queryID uint16) (*HTTPSSvc, error)
+
+	// DecodeReply decodes a DNS reply message.
+	//
+	// Arguments:
+	//
+	// - data is the raw reply
+	//
+	// If you use this function, remember that:
+	//
+	// 1. the Rcode MAY be nonzero;
+	//
+	// 2. the replyID MAY NOT match the original query ID.
+	//
+	// That is, this is a very basic parsing method.
+	DecodeReply(data []byte) (*dns.Msg, error)
 }
 
 // The DNSEncoder encodes DNS queries to bytes
@@ -61,9 +79,10 @@ type DNSEncoder interface {
 	//
 	// - padding is whether to add padding to the query.
 	//
-	// On success, this function returns a valid byte array and
-	// a nil error. On failure, we have an error and the byte array is nil.
-	Encode(domain string, qtype uint16, padding bool) ([]byte, error)
+	// On success, this function returns a valid byte array, the queryID, and
+	// a nil error. On failure, we have a non-nil error, a nil arrary and a zero
+	// query ID.
+	Encode(domain string, qtype uint16, padding bool) ([]byte, uint16, error)
 }
 
 // DNSTransport represents an abstract DNS transport.

internal/netxlite/classify.go

@@ -287,6 +287,9 @@ func classifyResolverError(err error) string {
 	if errors.Is(err, ErrOODNSServfail) {
 		return FailureDNSServfailError
 	}
+	if errors.Is(err, ErrDNSReplyWithWrongQueryID) {
+		return FailureDNSReplyWithWrongQueryID
+	}
 	return classifyGenericError(err)
 }
 

internal/netxlite/classify_test.go

@@ -275,6 +275,12 @@ func TestClassifyResolverError(t *testing.T) {
 		}
 	})
 
+	t.Run("for dns reply with wrong queryID", func(t *testing.T) {
+		if classifyResolverError(ErrDNSReplyWithWrongQueryID) != FailureDNSReplyWithWrongQueryID {
+			t.Fatal("unexpected result")
+		}
+	})
+
 	t.Run("for another kind of error", func(t *testing.T) {
 		if classifyResolverError(io.EOF) != FailureEOFError {
 			t.Fatal("unexpected result")

internal/netxlite/dnsdecoder.go

@@ -1,18 +1,35 @@
 package netxlite
 
 import (
+	"errors"
+
 	"github.com/miekg/dns"
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
 // DNSDecoderMiekg uses github.com/miekg/dns to implement the Decoder.
 type DNSDecoderMiekg struct{}
 
-func (d *DNSDecoderMiekg) parseReply(data []byte) (*dns.Msg, error) {
+// ErrDNSReplyWithWrongQueryID indicates we have got a DNS reply with the wrong queryID.
+var ErrDNSReplyWithWrongQueryID = errors.New(FailureDNSReplyWithWrongQueryID)
+
+// DecodeReply implements model.DNSDecoder.DecodeReply
+func (d *DNSDecoderMiekg) DecodeReply(data []byte) (*dns.Msg, error) {
 	reply := new(dns.Msg)
 	if err := reply.Unpack(data); err != nil {
 		return nil, err
 	}
+	return reply, nil
+}
+
+func (d *DNSDecoderMiekg) parseReply(data []byte, queryID uint16) (*dns.Msg, error) {
+	reply, err := d.DecodeReply(data)
+	if err != nil {
+		return nil, err
+	}
+	if reply.Id != queryID {
+		return nil, ErrDNSReplyWithWrongQueryID
+	}
 	// TODO(bassosimone): map more errors to net.DNSError names
 	// TODO(bassosimone): add support for lame referral.
 	switch reply.Rcode {
@@ -29,8 +46,8 @@ func (d *DNSDecoderMiekg) parseReply(data []byte) (*dns.Msg, error) {
 	}
 }
 
-func (d *DNSDecoderMiekg) DecodeHTTPS(data []byte) (*model.HTTPSSvc, error) {
-	reply, err := d.parseReply(data)
+func (d *DNSDecoderMiekg) DecodeHTTPS(data []byte, queryID uint16) (*model.HTTPSSvc, error) {
+	reply, err := d.parseReply(data, queryID)
 	if err != nil {
 		return nil, err
 	}
@@ -64,8 +81,8 @@ func (d *DNSDecoderMiekg) DecodeHTTPS(data []byte) (*model.HTTPSSvc, error) {
 	return out, nil
 }
 
-func (d *DNSDecoderMiekg) DecodeLookupHost(qtype uint16, data []byte) ([]string, error) {
-	reply, err := d.parseReply(data)
+func (d *DNSDecoderMiekg) DecodeLookupHost(qtype uint16, data []byte, queryID uint16) ([]string, error) {
+	reply, err := d.parseReply(data, queryID)
 	if err != nil {
 		return nil, err
 	}