net/dns/resolver: use forwarded dns txid directlyAadi/speedtest-tailscaled

Previously, we hashed the question and combined it with the original
txid which was useful when concurrent queries were multiplexed on a
single local source port. We encountered some situations where the DNS
server canonicalizes the question in the response (uppercase converted
to lowercase in this case), which resulted in responses that we couldn't
match to the original request due to hash mismatches. This includes a
new test to cover that situation.

Fixes #2597

Signed-off-by: Adrian Dewhurst <adrian@tailscale.com>

3 files changed, 77 insertions, 39 deletions

diff --git a/net/dns/resolver/forwarder.go b/net/dns/resolver/forwarder.go
index 5d1904468..68b84b557 100644
--- a/net/dns/resolver/forwarder.go
+++ b/net/dns/resolver/forwarder.go
@@ -10,7 +10,6 @@ import (
 	"encoding/binary"
 	"errors"
 	"fmt"
-	"hash/crc32"
 	"io"
 	"io/ioutil"
 	"math/rand"
@@ -65,44 +64,13 @@ func getTxID(packet []byte) txid {
 	}
 
 	dnsid := binary.BigEndian.Uint16(packet[0:2])
-	qcount := binary.BigEndian.Uint16(packet[4:6])
-	if qcount == 0 {
-		return txid(dnsid)
-	}
-
-	offset := headerBytes
-	for i := uint16(0); i < qcount; i++ {
-		// Note: this relies on the fact that names are not compressed in questions,
-		// so they are guaranteed to end with a NUL byte.
-		//
-		// Justification:
-		// RFC 1035 doesn't seem to explicitly prohibit compressing names in questions,
-		// but this is exceedingly unlikely to be done in practice. A DNS request
-		// with multiple questions is ill-defined (which questions do the header flags apply to?)
-		// and a single question would have to contain a pointer to an *answer*,
-		// which would be excessively smart, pointless (an answer can just as well refer to the question)
-		// and perhaps even prohibited: a draft RFC (draft-ietf-dnsind-local-compression-05) states:
-		//
-		// > It is important that these pointers always point backwards.
-		//
-		// This is said in summarizing RFC 1035, although that phrase does not appear in the original RFC.
-		// Additionally, (https://cr.yp.to/djbdns/notes.html) states:
-		//
-		// > The precise rule is that a name can be compressed if it is a response owner name,
-		// > the name in NS data, the name in CNAME data, the name in PTR data, the name in MX data,
-		// > or one of the names in SOA data.
-		namebytes := bytes.IndexByte(packet[offset:], 0)
-		// ... | name | NUL | type | class
-		//        ??     1      2      2
-		offset = offset + namebytes + 5
-		if len(packet) < offset {
-			// Corrupt packet; don't crash.
-			return txid(dnsid)
-		}
-	}
-
-	hash := crc32.ChecksumIEEE(packet[headerBytes:offset])
-	return (txid(hash) << 32) | txid(dnsid)
+	// Previously, we hashed the question and combined it with the original txid
+	// which was useful when concurrent queries were multiplexed on a single
+	// local source port. We encountered some situations where the DNS server
+	// canonicalizes the question in the response (uppercase converted to
+	// lowercase in this case), which resulted in responses that we couldn't
+	// match to the original request due to hash mismatches.
+	return txid(dnsid)
 }
 
 // clampEDNSSize attempts to limit the maximum EDNS response size. This is not
diff --git a/net/dns/resolver/tsdns_server_test.go b/net/dns/resolver/tsdns_server_test.go
index e0d8c40c5..675892a6e 100644
--- a/net/dns/resolver/tsdns_server_test.go
+++ b/net/dns/resolver/tsdns_server_test.go
@@ -6,6 +6,7 @@ package resolver
 
 import (
 	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/miekg/dns"
@@ -66,6 +67,58 @@ func resolveToIP(ipv4, ipv6 netaddr.IP, ns string) dns.HandlerFunc {
 	}
 }
 
+// resolveToIPLowercase returns a handler function which canonicalizes responses
+// by lowercasing the question and answer names, and responds
+// to queries of type A it receives with an A record containing ipv4,
+// to queries of type AAAA with an AAAA record containing ipv6,
+// to queries of type NS with an NS record containg name.
+func resolveToIPLowercase(ipv4, ipv6 netaddr.IP, ns string) dns.HandlerFunc {
+	return func(w dns.ResponseWriter, req *dns.Msg) {
+		m := new(dns.Msg)
+		m.SetReply(req)
+
+		if len(req.Question) != 1 {
+			panic("not a single-question request")
+		}
+		m.Question[0].Name = strings.ToLower(m.Question[0].Name)
+		question := req.Question[0]
+
+		var ans dns.RR
+		switch question.Qtype {
+		case dns.TypeA:
+			ans = &dns.A{
+				Hdr: dns.RR_Header{
+					Name:   question.Name,
+					Rrtype: dns.TypeA,
+					Class:  dns.ClassINET,
+				},
+				A: ipv4.IPAddr().IP,
+			}
+		case dns.TypeAAAA:
+			ans = &dns.AAAA{
+				Hdr: dns.RR_Header{
+					Name:   question.Name,
+					Rrtype: dns.TypeAAAA,
+					Class:  dns.ClassINET,
+				},
+				AAAA: ipv6.IPAddr().IP,
+			}
+		case dns.TypeNS:
+			ans = &dns.NS{
+				Hdr: dns.RR_Header{
+					Name:   question.Name,
+					Rrtype: dns.TypeNS,
+					Class:  dns.ClassINET,
+				},
+				Ns: ns,
+			}
+		}
+
+		m.Answer = append(m.Answer, ans)
+		w.WriteMsg(m)
+	}
+}
+
 // resolveToTXT returns a handler function which responds to queries of type TXT
 // it receives with the strings in txts.
 func resolveToTXT(txts []string, ednsMaxSize uint16) dns.HandlerFunc {
diff --git a/net/dns/resolver/tsdns_test.go b/net/dns/resolver/tsdns_test.go
index 94f3eb88e..d3f0ca55c 100644
--- a/net/dns/resolver/tsdns_test.go
+++ b/net/dns/resolver/tsdns_test.go
@@ -440,6 +440,8 @@ func TestDelegate(t *testing.T) {
 	records := []interface{}{
 		"test.site.",
 		resolveToIP(testipv4, testipv6, "dns.test.site."),
+		"LCtesT.SiTe.",
+		resolveToIPLowercase(testipv4, testipv6, "dns.test.site."),
 		"nxdomain.site.", resolveToNXDOMAIN,
 		"small.txt.", resolveToTXT(smallTXT, noEdns),
 		"smalledns.txt.", resolveToTXT(smallTXT, 512),
@@ -486,6 +488,21 @@ func TestDelegate(t *testing.T) {
 			dnsResponse{name: "dns.test.site.", rcode: dns.RCodeSuccess},
 		},
 		{
+			"ipv4",
+			dnspacket("LCtesT.SiTe.", dns.TypeA, noEdns),
+			dnsResponse{ip: testipv4, rcode: dns.RCodeSuccess},
+		},
+		{
+			"ipv6",
+			dnspacket("LCtesT.SiTe.", dns.TypeAAAA, noEdns),
+			dnsResponse{ip: testipv6, rcode: dns.RCodeSuccess},
+		},
+		{
+			"ns",
+			dnspacket("LCtesT.SiTe.", dns.TypeNS, noEdns),
+			dnsResponse{name: "dns.test.site.", rcode: dns.RCodeSuccess},
+		},
+		{
 			"nxdomain",
 			dnspacket("nxdomain.site.", dns.TypeA, noEdns),
 			dnsResponse{rcode: dns.RCodeNameError},