Fix dns-01-003 (#1634)

* plugin/{cache,forward,proxy}: don't allow responses that are bogus

Responses that are not matching what we've been querying for should be
dropped. They are converted into FormErrs by forward and proxy; as a 2nd
backstop cache will also not cache these.

* plug

* add explicit test

plugin/cache/README.md

@@ -57,6 +57,7 @@ If monitoring is enabled (via the *prometheus* directive) then the following met
 * `coredns_cache_capacity{type}` - Total capacity of the cache by cache type.
 * `coredns_cache_hits_total{type}` - Counter of cache hits by cache type.
 * `coredns_cache_misses_total{}` - Counter of cache misses.
+* `coredns_cache_drops_total{}` - Counter of dropped messages.
 
 Cache types are either "denial" or "success".
 

plugin/cache/cache.go

@@ -10,6 +10,7 @@ import (
 	"github.com/coredns/coredns/plugin"
 	"github.com/coredns/coredns/plugin/pkg/cache"
 	"github.com/coredns/coredns/plugin/pkg/response"
+	"github.com/coredns/coredns/request"
 
 	"github.com/miekg/dns"
 )
@@ -102,6 +103,7 @@ func hash(qname string, qtype uint16, do bool) uint32 {
 type ResponseWriter struct {
 	dns.ResponseWriter
 	*Cache
+	state request.Request
 
 	prefetch bool // When true write nothing back to the client.
 }
@@ -128,10 +130,15 @@ func (w *ResponseWriter) WriteMsg(res *dns.Msg) error {
 	}
 
 	if key != -1 && duration > 0 {
-		w.set(res, key, mt, duration)
 
-		cacheSize.WithLabelValues(Success).Set(float64(w.pcache.Len()))
-		cacheSize.WithLabelValues(Denial).Set(float64(w.ncache.Len()))
+		if w.state.Match(res) {
+			w.set(res, key, mt, duration)
+			cacheSize.WithLabelValues(Success).Set(float64(w.pcache.Len()))
+			cacheSize.WithLabelValues(Denial).Set(float64(w.ncache.Len()))
+		} else {
+			// Don't log it, but increment counter
+			cacheDrops.Inc()
+		}
 	}
 
 	if w.prefetch {

plugin/cache/handler.go

@@ -46,7 +46,7 @@ func (c *Cache) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg)
 					// When prefetching we loose the item i, and with it the frequency
 					// that we've gathered sofar. See we copy the frequencies info back
 					// into the new item that was stored in the cache.
-					prr := &ResponseWriter{ResponseWriter: w, Cache: c, prefetch: true}
+					prr := &ResponseWriter{ResponseWriter: w, Cache: c, prefetch: true, state: state}
 					plugin.NextOrFailure(c.Name(), c.Next, ctx, prr, r)
 
 					if i1 := c.exists(qname, qtype, do); i1 != nil {
@@ -58,7 +58,7 @@ func (c *Cache) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg)
 		return dns.RcodeSuccess, nil
 	}
 
-	crr := &ResponseWriter{ResponseWriter: w, Cache: c}
+	crr := &ResponseWriter{ResponseWriter: w, Cache: c, state: state}
 	return plugin.NextOrFailure(c.Name(), c.Next, ctx, crr, r)
 }
 
@@ -127,6 +127,13 @@ var (
 		Name:      "prefetch_total",
 		Help:      "The number of time the cache has prefetched a cached item.",
 	})
+
+	cacheDrops = prometheus.NewCounter(prometheus.CounterOpts{
+		Namespace: plugin.Namespace,
+		Subsystem: "cache",
+		Name:      "drops_total",
+		Help:      "The number responses that are not cached, because the reply is malformed.",
+	})
 )
 
 var once sync.Once

plugin/cache/setup.go

@@ -42,6 +42,7 @@ func setup(c *caddy.Controller) error {
 				x.MustRegister(cacheHits)
 				x.MustRegister(cacheMisses)
 				x.MustRegister(cachePrefetches)
+				x.MustRegister(cacheDrops)
 			}
 		})
 		return nil

plugin/cache/spoof_test.go

@@ -0,0 +1,66 @@
+package cache
+
+import (
+	"testing"
+
+	"github.com/coredns/coredns/plugin"
+	"github.com/coredns/coredns/plugin/pkg/dnstest"
+
+	"github.com/coredns/coredns/plugin/test"
+	"github.com/miekg/dns"
+	"golang.org/x/net/context"
+)
+
+func TestSpoof(t *testing.T) {
+	// Send query for example.org, get reply for example.net; should not be cached.
+	c := New()
+	c.Next = spoofHandler()
+
+	req := new(dns.Msg)
+	req.SetQuestion("example.org.", dns.TypeA)
+	rec := dnstest.NewRecorder(&test.ResponseWriter{})
+
+	c.ServeDNS(context.TODO(), rec, req)
+
+	qname := rec.Msg.Question[0].Name
+	if c.pcache.Len() != 0 {
+		t.Errorf("cached %s, while reply had %s", "example.org.", qname)
+	}
+
+	// qtype
+	c.Next = spoofHandlerType()
+	req.SetQuestion("example.org.", dns.TypeMX)
+
+	c.ServeDNS(context.TODO(), rec, req)
+
+	qtype := rec.Msg.Question[0].Qtype
+	if c.pcache.Len() != 0 {
+		t.Errorf("cached %s type %d, while reply had %d", "example.org.", dns.TypeMX, qtype)
+	}
+}
+
+// spoofHandler is a fake plugin implementation which returns a single A records for example.org. The qname in the
+// question section is set to example.NET (i.e. they *don't* match).
+func spoofHandler() plugin.Handler {
+	return plugin.HandlerFunc(func(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+		m := new(dns.Msg)
+		m.SetQuestion("example.net.", dns.TypeA)
+		m.Response = true
+		m.Answer = []dns.RR{test.A("example.org. IN A 127.0.0.53")}
+		w.WriteMsg(m)
+		return dns.RcodeSuccess, nil
+	})
+}
+
+// spoofHandlerType is a fake plugin implementation which returns a single MX records for example.org. The qtype in the
+// question section is set to A.
+func spoofHandlerType() plugin.Handler {
+	return plugin.HandlerFunc(func(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+		m := new(dns.Msg)
+		m.SetQuestion("example.org.", dns.TypeA)
+		m.Response = true
+		m.Answer = []dns.RR{test.MX("example.org. IN MX 10 mail.example.org.")}
+		w.WriteMsg(m)
+		return dns.RcodeSuccess, nil
+	})
+}

plugin/forward/forward.go

@@ -119,6 +119,13 @@ func (f *Forward) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg
 			break
 		}
 
+		// Check if the reply is correct; if not return FormErr.
+		if !state.Match(ret) {
+			formerr := state.ErrorMessage(dns.RcodeFormatError)
+			w.WriteMsg(formerr)
+			return 0, nil
+		}
+
 		ret.Compress = true
 		// When using force_tcp the upstream can send a message that is too big for
 		// the udp buffer, hence we need to truncate the message to at least make it

plugin/proxy/proxy.go

@@ -100,6 +100,14 @@ func (p Proxy) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (
 			taperr := toDnstap(ctx, host.Name, upstream.Exchanger(), state, reply, start)
 
 			if backendErr == nil {
+
+				// Check if the reply is correct; if not return FormErr.
+				if !state.Match(reply) {
+					formerr := state.ErrorMessage(dns.RcodeFormatError)
+					w.WriteMsg(formerr)
+					return 0, taperr
+				}
+
 				w.WriteMsg(reply)
 
 				RequestDuration.WithLabelValues(state.Proto(), upstream.Exchanger().Protocol(), familyToString(state.Family()), host.Name).Observe(time.Since(start).Seconds())