plugin/file: guard against cname loops (#4387)

Automatically submitted.

8 files changed, 84 insertions, 4 deletions

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)
+
+	}
+}