From 342eae9b4b1791da8fb92d36b3968839f3f38b94 Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Fri, 15 Jan 2021 19:26:04 +0100 Subject: [PATCH] plugin/file: guard against cname loops (#4387) Automatically submitted. --- core/dnsserver/server.go | 11 ++++++-- core/dnsserver/server_grpc.go | 1 + core/dnsserver/server_https.go | 1 + core/dnsserver/server_tls.go | 1 + plugin.md | 9 ++++++ plugin/file/lookup.go | 15 +++++++++- plugin/pkg/upstream/upstream.go | 1 - test/file_loop_test.go | 49 +++++++++++++++++++++++++++++++++ 8 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 test/file_loop_test.go diff --git a/core/dnsserver/server.go b/core/dnsserver/server.go index eb23346e0..c7304d763 100644 --- a/core/dnsserver/server.go +++ b/core/dnsserver/server.go @@ -110,6 +110,7 @@ func (s *Server) Serve(l net.Listener) error { s.m.Lock() s.server[tcp] = &dns.Server{Listener: l, Net: "tcp", Handler: dns.HandlerFunc(func(w dns.ResponseWriter, r *dns.Msg) { ctx := context.WithValue(context.Background(), Key{}, s) + ctx = context.WithValue(ctx, LoopKey{}, 0) s.ServeDNS(ctx, w, r) })} s.m.Unlock() @@ -123,6 +124,7 @@ func (s *Server) ServePacket(p net.PacketConn) error { s.m.Lock() s.server[udp] = &dns.Server{PacketConn: p, Net: "udp", Handler: dns.HandlerFunc(func(w dns.ResponseWriter, r *dns.Msg) { ctx := context.WithValue(context.Background(), Key{}, s) + ctx = context.WithValue(ctx, LoopKey{}, 0) s.ServeDNS(ctx, w, r) })} s.m.Unlock() @@ -347,8 +349,13 @@ const ( udp = 1 ) -// Key is the context key for the current server added to the context. -type Key struct{} +type ( + // Key is the context key for the current server added to the context. + Key struct{} + + // LoopKey is the context key to detect server wide loops. + LoopKey struct{} +) // EnableChaos is a map with plugin names for which we should open CH class queries as we block these by default. var EnableChaos = map[string]struct{}{ diff --git a/core/dnsserver/server_grpc.go b/core/dnsserver/server_grpc.go index 7873a47ad..37cc237b7 100644 --- a/core/dnsserver/server_grpc.go +++ b/core/dnsserver/server_grpc.go @@ -134,6 +134,7 @@ func (s *ServergRPC) Query(ctx context.Context, in *pb.DnsPacket) (*pb.DnsPacket w := &gRPCresponse{localAddr: s.listenAddr, remoteAddr: a, Msg: msg} dnsCtx := context.WithValue(ctx, Key{}, s.Server) + dnsCtx = context.WithValue(dnsCtx, LoopKey{}, 0) s.ServeDNS(dnsCtx, w, msg) packed, err := w.Msg.Pack() diff --git a/core/dnsserver/server_https.go b/core/dnsserver/server_https.go index 057dac49c..7292311e8 100644 --- a/core/dnsserver/server_https.go +++ b/core/dnsserver/server_https.go @@ -145,6 +145,7 @@ func (s *ServerHTTPS) ServeHTTP(w http.ResponseWriter, r *http.Request) { // We just call the normal chain handler - all error handling is done there. // We should expect a packet to be returned that we can send to the client. ctx := context.WithValue(context.Background(), Key{}, s.Server) + ctx = context.WithValue(ctx, LoopKey{}, 0) s.ServeDNS(ctx, dw, msg) // See section 4.2.1 of RFC 8484. diff --git a/core/dnsserver/server_tls.go b/core/dnsserver/server_tls.go index 3f45e1568..1c53c4e3c 100644 --- a/core/dnsserver/server_tls.go +++ b/core/dnsserver/server_tls.go @@ -50,6 +50,7 @@ func (s *ServerTLS) Serve(l net.Listener) error { // Only fill out the TCP server for this one. s.server[tcp] = &dns.Server{Listener: l, Net: "tcp-tls", Handler: dns.HandlerFunc(func(w dns.ResponseWriter, r *dns.Msg) { ctx := context.WithValue(context.Background(), Key{}, s.Server) + ctx = context.WithValue(ctx, LoopKey{}, 0) s.ServeDNS(ctx, w, r) })} s.m.Unlock() diff --git a/plugin.md b/plugin.md index 68ab10e50..f7158651c 100644 --- a/plugin.md +++ b/plugin.md @@ -69,6 +69,15 @@ works, and implement the `ready.Readiness` interface. See the plugin/pkg/reuseport for `Listen` and `ListenPacket` functions. Using these functions makes your plugin handle reload events better. +## Context + +Every request get a context.Context these are pre-filled with 2 values: + +* `Key`: holds a pointer to the current server, this can be useful for logging or metrics. It is + infact used in the *metrics* plugin to tie a request to a specific (internal) server. +* `LoopKey`: holds an integer to detect loops within the current context. The *file* plugin uses + this to detect loops when resolving CNAMEs. + ## Documentation Each plugin should have a README.md explaining what the plugin does and how it is configured. The diff --git a/plugin/file/lookup.go b/plugin/file/lookup.go index e036d809a..6eeb4c397 100644 --- a/plugin/file/lookup.go +++ b/plugin/file/lookup.go @@ -3,6 +3,7 @@ package file import ( "context" + "github.com/coredns/coredns/core/dnsserver" "github.com/coredns/coredns/plugin/file/rrutil" "github.com/coredns/coredns/plugin/file/tree" "github.com/coredns/coredns/request" @@ -29,7 +30,6 @@ const ( // Lookup looks up qname and qtype in the zone. When do is true DNSSEC records are included. // Three sets of records are returned, one for the answer, one for authority and one for the additional section. func (z *Zone) Lookup(ctx context.Context, state request.Request, qname string) ([]dns.RR, []dns.RR, []dns.RR, Result) { - qtype := state.QType() do := state.Do() @@ -62,6 +62,16 @@ func (z *Zone) Lookup(ctx context.Context, state request.Request, qname string) elem, wildElem *tree.Elem ) + loop, _ := ctx.Value(dnsserver.LoopKey{}).(int) + if loop > 8 { + // We're back here for the 9th time; we have a loop and need to bail out. + // Note the answer we're returning will be incomplete (more cnames to be followed) or + // illegal (wildcard cname with multiple identical records). For now it's more important + // to protect ourselves then to give the client a valid answer. We return with an error + // to let the server handle what to do. + return nil, nil, nil, ServerFailure + } + // Lookup: // * Per label from the right, look if it exists. We do this to find potential // delegation records. @@ -105,6 +115,7 @@ func (z *Zone) Lookup(ctx context.Context, state request.Request, qname string) // Only one DNAME is allowed per name. We just pick the first one to synthesize from. dname := dnamerrs[0] if cname := synthesizeCNAME(state.Name(), dname.(*dns.DNAME)); cname != nil { + ctx = context.WithValue(ctx, dnsserver.LoopKey{}, loop+1) answer, ns, extra, rcode := z.externalLookup(ctx, state, elem, []dns.RR{cname}) if do { @@ -156,6 +167,7 @@ func (z *Zone) Lookup(ctx context.Context, state request.Request, qname string) if found && shot { if rrs := elem.Type(dns.TypeCNAME); len(rrs) > 0 && qtype != dns.TypeCNAME { + ctx = context.WithValue(ctx, dnsserver.LoopKey{}, loop+1) return z.externalLookup(ctx, state, elem, rrs) } @@ -192,6 +204,7 @@ func (z *Zone) Lookup(ctx context.Context, state request.Request, qname string) auth := ap.ns(do) if rrs := wildElem.TypeForWildcard(dns.TypeCNAME, qname); len(rrs) > 0 { + ctx = context.WithValue(ctx, dnsserver.LoopKey{}, loop+1) return z.externalLookup(ctx, state, wildElem, rrs) } diff --git a/plugin/pkg/upstream/upstream.go b/plugin/pkg/upstream/upstream.go index eacb9b4d5..9c2973e41 100644 --- a/plugin/pkg/upstream/upstream.go +++ b/plugin/pkg/upstream/upstream.go @@ -32,7 +32,6 @@ func (u *Upstream) Lookup(ctx context.Context, state request.Request, name strin req.SetEdns0(uint16(size), do) nw := nonwriter.New(state.W) - server.ServeDNS(ctx, nw, req) return nw.Msg, nil diff --git a/test/file_loop_test.go b/test/file_loop_test.go new file mode 100644 index 000000000..daebb7088 --- /dev/null +++ b/test/file_loop_test.go @@ -0,0 +1,49 @@ +package test + +import ( + "testing" + + "github.com/coredns/coredns/plugin/test" + + "github.com/miekg/dns" +) + +const loopDB = `example.com. 500 IN SOA ns1.outside.com. root.example.com. 3 604800 86400 2419200 604800 +example.com. 500 IN NS ns1.outside.com. +a.example.com. 500 IN CNAME b.example.com. +*.foo.example.com. 500 IN CNAME bar.foo.example.com.` + +func TestFileLoop(t *testing.T) { + name, rm, err := test.TempFile(".", loopDB) + if err != nil { + t.Fatalf("Failed to create zone: %s", err) + } + defer rm() + + // Corefile with for example without proxy section. + corefile := `example.com:0 { + file ` + name + ` + }` + + i, udp, _, err := CoreDNSServerAndPorts(corefile) + if err != nil { + t.Fatalf("Could not get CoreDNS serving instance: %s", err) + } + defer i.Stop() + + m := new(dns.Msg) + m.SetQuestion("something.foo.example.com.", dns.TypeA) + + r, err := dns.Exchange(m, udp) + if err != nil { + t.Fatalf("Could not exchange msg: %s", err) + } + + // This should not loop, don't really care about the correctness of the answer. + // Currently we return servfail in the file lookup.go file. + // For now: document current behavior in this test. + if r.Rcode != dns.RcodeServerFailure { + t.Errorf("Rcode should be dns.RcodeServerFailure: %d", r.Rcode) + + } +}