From 7c45f7b88c05d93115a2e68382bef9bf2ef210ba Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 16 May 2022 11:17:30 +0200 Subject: [PATCH] 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 https://github.com/bassosimone/websteps-illustrated/commit/9249d14f80b4c685234da8e344b36c8904ee69e9 See https://github.com/ooni/probe/issues/2096. --- internal/model/netx.go | 3 ++ internal/netxlite/dnsdecoder.go | 26 ++++++++++++---- internal/netxlite/dnsdecoder_test.go | 44 ++++++++++++++++++++++++++-- 3 files changed, 66 insertions(+), 7 deletions(-) diff --git a/internal/model/netx.go b/internal/model/netx.go index 93c6907..a25575b 100644 --- a/internal/model/netx.go +++ b/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; diff --git a/internal/netxlite/dnsdecoder.go b/internal/netxlite/dnsdecoder.go index b37fd9f..cf6590d 100644 --- a/internal/netxlite/dnsdecoder.go +++ b/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 } diff --git a/internal/netxlite/dnsdecoder_test.go b/internal/netxlite/dnsdecoder_test.go index ed716f5..67fbd9d 100644 --- a/internal/netxlite/dnsdecoder_test.go +++ b/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 (