From 6c71e5b85103f2c491b3eb6a9e15604bd1a683b7 Mon Sep 17 00:00:00 2001 From: Dmytro Shynkevych Date: Thu, 20 Aug 2020 18:54:18 -0400 Subject: [PATCH] tsdns: copy name when loewrcasing. The previous approach modifies name in-place in the request slice to avoid an allocation. This is incorrect: the question section of a DNS request must be copied verbatim, without any such modification. Software may rely on it (we rely on other resolvers doing it it in tsdns/forwarder). Signed-off-by: Dmytro Shynkevych --- wgengine/tsdns/tsdns.go | 62 +++++++++++++++++++----------------- wgengine/tsdns/tsdns_test.go | 41 ++++++++++++++++-------- 2 files changed, 61 insertions(+), 42 deletions(-) diff --git a/wgengine/tsdns/tsdns.go b/wgengine/tsdns/tsdns.go index 6aa637ac5..0216b8d90 100644 --- a/wgengine/tsdns/tsdns.go +++ b/wgengine/tsdns/tsdns.go @@ -7,10 +7,10 @@ package tsdns import ( - "bytes" "encoding/hex" "errors" "net" + "strings" "sync" "time" @@ -59,7 +59,7 @@ type Packet struct { type Resolver struct { logf logger.Logf // rootDomain is in ... - rootDomain []byte + rootDomain string // forwarder is forwarder *forwarder @@ -100,7 +100,7 @@ func NewResolver(config ResolverConfig) *Resolver { responses: make(chan Packet), errors: make(chan error), closed: make(chan struct{}), - rootDomain: []byte(config.RootDomain), + rootDomain: config.RootDomain, } if config.Forward { @@ -293,14 +293,6 @@ func (r *Resolver) parseQuery(query []byte, resp *response) error { return err } - // Lowercase the name: DOMAIN.COM. should resolve the same as domain.com. - name := resp.Question.Name.Data[:resp.Question.Name.Length] - for i, b := range name { - if 'A' <= b && b <= 'Z' { - name[i] = b - 'A' + 'a' - } - } - return nil } @@ -402,19 +394,34 @@ func marshalResponse(resp *response) ([]byte, error) { return builder.Finish() } -var ( - rdnsv4Suffix = []byte(".in-addr.arpa.") - rdnsv6Suffix = []byte(".ip6.arpa.") +const ( + rdnsv4Suffix = ".in-addr.arpa." + rdnsv6Suffix = ".ip6.arpa." ) +// rawNameToLower converts a raw DNS name to a string, lowercasing it. +func rawNameToLower(name []byte) string { + var sb strings.Builder + sb.Grow(len(name)) + + for _, b := range name { + if 'A' <= b && b <= 'Z' { + b = b - 'A' + 'a' + } + sb.WriteByte(b) + } + + return sb.String() +} + // ptrNameToIPv4 transforms a PTR name representing an IPv4 address to said address. // Such names are IPv4 labels in reverse order followed by .in-addr.arpa. // For example, // 4.3.2.1.in-addr.arpa // is transformed to // 1.2.3.4 -func rdnsNameToIPv4(name []byte) (ip netaddr.IP, ok bool) { - name = bytes.TrimSuffix(name, rdnsv4Suffix) +func rdnsNameToIPv4(name string) (ip netaddr.IP, ok bool) { + name = strings.TrimSuffix(name, rdnsv4Suffix) ip, err := netaddr.ParseIP(string(name)) if err != nil { return netaddr.IP{}, false @@ -432,11 +439,11 @@ func rdnsNameToIPv4(name []byte) (ip netaddr.IP, ok bool) { // b.a.9.8.7.6.5.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. // is transformed to // 2001:db8::567:89ab -func rdnsNameToIPv6(name []byte) (ip netaddr.IP, ok bool) { +func rdnsNameToIPv6(name string) (ip netaddr.IP, ok bool) { var b [32]byte var ipb [16]byte - name = bytes.TrimSuffix(name, rdnsv6Suffix) + name = strings.TrimSuffix(name, rdnsv6Suffix) // 32 nibbles and 31 dots between them. if len(name) != 63 { return netaddr.IP{}, false @@ -470,16 +477,14 @@ func rdnsNameToIPv6(name []byte) (ip netaddr.IP, ok bool) { // respondReverse returns a DNS response to a PTR query. // It is assumed that resp.Question is populated by respond before this is called. -func (r *Resolver) respondReverse(query []byte, resp *response) ([]byte, error) { - name := resp.Question.Name.Data[:resp.Question.Name.Length] - +func (r *Resolver) respondReverse(query []byte, name string, resp *response) ([]byte, error) { var ip netaddr.IP var ok bool var err error switch { - case bytes.HasSuffix(name, rdnsv4Suffix): + case strings.HasSuffix(name, rdnsv4Suffix): ip, ok = rdnsNameToIPv4(name) - case bytes.HasSuffix(name, rdnsv6Suffix): + case strings.HasSuffix(name, rdnsv6Suffix): ip, ok = rdnsNameToIPv6(name) default: return nil, errNotOurName @@ -489,7 +494,7 @@ func (r *Resolver) respondReverse(query []byte, resp *response) ([]byte, error) // To avoid frustrating users, just log and delegate. if !ok { // Without this conversion, escape analysis rules that resp escapes. - r.logf("parsing rdns: malformed name: %s", resp.Question.Name.String()) + r.logf("parsing rdns: malformed name: %s", name) return nil, errNotOurName } @@ -518,24 +523,23 @@ func (r *Resolver) respond(query []byte) ([]byte, error) { resp.Header.RCode = dns.RCodeFormatError return marshalResponse(resp) } + rawName := resp.Question.Name.Data[:resp.Question.Name.Length] + name := rawNameToLower(rawName) // Always try to handle reverse lookups; delegate inside when not found. // This way, queries for exitent nodes do not leak, // but we behave gracefully if non-Tailscale nodes exist in CGNATRange. if resp.Question.Type == dns.TypePTR { - return r.respondReverse(query, resp) + return r.respondReverse(query, name, resp) } // Delegate forward lookups when not a subdomain of rootDomain. - // We do this on bytes because Name.String() allocates. - rawName := resp.Question.Name.Data[:resp.Question.Name.Length] - if !bytes.HasSuffix(rawName, r.rootDomain) { + if !strings.HasSuffix(name, r.rootDomain) { return nil, errNotOurName } switch resp.Question.Type { case dns.TypeA, dns.TypeAAAA, dns.TypeALL: - name := resp.Question.Name.String() resp.IP, resp.Header.RCode, err = r.Resolve(name) default: resp.Header.RCode = dns.RCodeNotImplemented diff --git a/wgengine/tsdns/tsdns_test.go b/wgengine/tsdns/tsdns_test.go index 4cca1a108..668b846f5 100644 --- a/wgengine/tsdns/tsdns_test.go +++ b/wgengine/tsdns/tsdns_test.go @@ -122,8 +122,7 @@ func TestRDNSNameToIPv4(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - name := []byte(tt.input) - ip, ok := rdnsNameToIPv4(name) + ip, ok := rdnsNameToIPv4(tt.input) if ok != tt.wantOK { t.Errorf("ok = %v; want %v", ok, tt.wantOK) } else if ok && ip != tt.wantIP { @@ -168,8 +167,7 @@ func TestRDNSNameToIPv6(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - name := []byte(tt.input) - ip, ok := rdnsNameToIPv6(name) + ip, ok := rdnsNameToIPv6(tt.input) if ok != tt.wantOK { t.Errorf("ok = %v; want %v", ok, tt.wantOK) } else if ok && ip != tt.wantIP { @@ -467,7 +465,7 @@ func TestConcurrentSetUpstreams(t *testing.T) { wg.Wait() } -var validIPv4Response = []byte{ +var ipv4Response = []byte{ 0x00, 0x00, // transaction id: 0 0x84, 0x00, // flags: response, authoritative, no error 0x00, 0x01, // one question @@ -484,7 +482,7 @@ var validIPv4Response = []byte{ 0x01, 0x02, 0x03, 0x04, // A: 1.2.3.4 } -var validIPv6Response = []byte{ +var ipv6Response = []byte{ 0x00, 0x00, // transaction id: 0 0x84, 0x00, // flags: response, authoritative, no error 0x00, 0x01, // one question @@ -502,7 +500,24 @@ var validIPv6Response = []byte{ 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0xb, 0xc, 0xd, 0xe, 0xf, } -var validPTRResponse = []byte{ +var ipv4UppercaseResponse = []byte{ + 0x00, 0x00, // transaction id: 0 + 0x84, 0x00, // flags: response, authoritative, no error + 0x00, 0x01, // one question + 0x00, 0x01, // one answer + 0x00, 0x00, 0x00, 0x00, // no authority or additional RRs + // Question: + 0x05, 0x54, 0x45, 0x53, 0x54, 0x31, 0x03, 0x49, 0x50, 0x4e, 0x03, 0x44, 0x45, 0x56, 0x00, // name + 0x00, 0x01, 0x00, 0x01, // type A, class IN + // Answer: + 0x05, 0x54, 0x45, 0x53, 0x54, 0x31, 0x03, 0x49, 0x50, 0x4e, 0x03, 0x44, 0x45, 0x56, 0x00, // name + 0x00, 0x01, 0x00, 0x01, // type A, class IN + 0x00, 0x00, 0x02, 0x58, // TTL: 600 + 0x00, 0x04, // length: 4 bytes + 0x01, 0x02, 0x03, 0x04, // A: 1.2.3.4 +} + +var ptrResponse = []byte{ 0x00, 0x00, // transaction id: 0 0x84, 0x00, // flags: response, authoritative, no error 0x00, 0x01, // one question @@ -548,10 +563,10 @@ func TestFull(t *testing.T) { request []byte response []byte }{ - {"ipv4", dnspacket("test1.ipn.dev.", dns.TypeA), validIPv4Response}, - {"ipv6", dnspacket("test2.ipn.dev.", dns.TypeAAAA), validIPv6Response}, - {"upper", dnspacket("TEST1.IPN.DEV.", dns.TypeA), validIPv4Response}, - {"ptr", dnspacket("4.3.2.1.in-addr.arpa.", dns.TypePTR), validPTRResponse}, + {"ipv4", dnspacket("test1.ipn.dev.", dns.TypeA), ipv4Response}, + {"ipv6", dnspacket("test2.ipn.dev.", dns.TypeAAAA), ipv6Response}, + {"upper", dnspacket("TEST1.IPN.DEV.", dns.TypeA), ipv4UppercaseResponse}, + {"ptr", dnspacket("4.3.2.1.in-addr.arpa.", dns.TypePTR), ptrResponse}, {"error", dnspacket("test3.ipn.dev.", dns.TypeA), nxdomainResponse}, } @@ -584,8 +599,8 @@ func TestAllocs(t *testing.T) { query []byte want int }{ - // The only alloc is the slice created by dns.NewBuilder. - {"forward", dnspacket("test1.ipn.dev.", dns.TypeA), 1}, + // Name lowercasing and response slice created by dns.NewBuilder. + {"forward", dnspacket("test1.ipn.dev.", dns.TypeA), 2}, // 3 extra allocs in rdnsNameToIPv4 and one in marshalPTRRecord (dns.NewName). {"reverse", dnspacket("4.3.2.1.in-addr.arpa.", dns.TypePTR), 5}, }