diff --git a/cmd/cli/dns_proxy.go b/cmd/cli/dns_proxy.go index 5b8955e..45c00e3 100644 --- a/cmd/cli/dns_proxy.go +++ b/cmd/cli/dns_proxy.go @@ -974,6 +974,17 @@ func (p *prog) processUpstream(ctx context.Context, req *proxyRequest, upstream } ctrld.Log(ctx, p.Debug(), "Upstream query successful") + // Reject an answer whose question does not match the request before it + // can be served or cached. A mismatched question means the upstream + // answered a different name/type than asked; caching it would poison + // the shared cache with wrong-domain records for the requested name. + // See github.com/Control-D-Inc/ctrld/issues/322. + if !sameQuestion(req.msg, answer) { + ctrld.Log(ctx, p.Debug(), + "Discarding answer from %s: question mismatch (asked %q, got %q)", + upstream, questionString(req.msg), questionString(answer)) + return nil + } if p.shouldContinueWithNextUpstream(ctx, req, answer, upstream, lastUpstream) { return nil } @@ -1132,6 +1143,33 @@ func requestID() string { return hex.EncodeToString(b) } +// sameQuestion reports whether the upstream answer echoes the request's +// question. A well-behaved resolver always copies the question section from +// the query (RFC 1035 section 4.1.2); names are compared case-insensitively +// because DNS names are case-insensitive. A mismatch means the upstream +// answered a different name/type than asked - malformed or malicious - and the +// answer must not be served or cached, or it would poison the shared cache with +// wrong-domain records. See github.com/Control-D-Inc/ctrld/issues/322. +func sameQuestion(req, answer *dns.Msg) bool { + if req == nil || answer == nil { + return false + } + if len(req.Question) == 0 || len(answer.Question) == 0 { + return false + } + rq, aq := req.Question[0], answer.Question[0] + return rq.Qtype == aq.Qtype && rq.Qclass == aq.Qclass && strings.EqualFold(rq.Name, aq.Name) +} + +// questionString renders a message's first question as "name/type" for logging. +func questionString(msg *dns.Msg) string { + if msg == nil || len(msg.Question) == 0 { + return "" + } + q := msg.Question[0] + return q.Name + "/" + dns.TypeToString[q.Qtype] +} + // setCachedAnswerTTL updates the TTL of each DNS record in the provided message based on the current and expiration times. func setCachedAnswerTTL(answer *dns.Msg, now, expiredTime time.Time) { ttlSecs := expiredTime.Sub(now).Seconds() diff --git a/cmd/cli/dns_proxy_test.go b/cmd/cli/dns_proxy_test.go index 9b8b164..492b248 100644 --- a/cmd/cli/dns_proxy_test.go +++ b/cmd/cli/dns_proxy_test.go @@ -811,6 +811,36 @@ func Test_prog_queryFromSelf(t *testing.T) { }) } +func Test_sameQuestion(t *testing.T) { + mk := func(name string, qtype uint16) *dns.Msg { + m := new(dns.Msg) + m.SetQuestion(name, qtype) + return m + } + tests := []struct { + name string + req *dns.Msg + answer *dns.Msg + want bool + }{ + {"identical", mk("example.com.", dns.TypeA), mk("example.com.", dns.TypeA), true}, + {"case insensitive", mk("Example.COM.", dns.TypeA), mk("example.com.", dns.TypeA), true}, + {"different name", mk("victim.example.", dns.TypeA), mk("attacker.example.", dns.TypeA), false}, + {"different type", mk("example.com.", dns.TypeA), mk("example.com.", dns.TypeAAAA), false}, + {"nil req", nil, mk("example.com.", dns.TypeA), false}, + {"nil answer", mk("example.com.", dns.TypeA), nil, false}, + {"empty answer question", mk("example.com.", dns.TypeA), new(dns.Msg), false}, + } + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + if got := sameQuestion(tc.req, tc.answer); got != tc.want { + t.Errorf("sameQuestion() = %v, want %v", got, tc.want) + } + }) + } +} + // newTestProg creates a properly initialized *prog for testing. func newTestProg(t *testing.T) *prog { p := &prog{cfg: testhelper.SampleConfig(t)} diff --git a/doq_test.go b/doq_test.go index ae60ad3..407d0f4 100644 --- a/doq_test.go +++ b/doq_test.go @@ -748,7 +748,7 @@ func TestDoQResolve_PreservesRcode(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - pool := newDOQConnPool(uc, []string{"127.0.0.1"}) + pool := newDOQConnPool(ctx, uc, []string{"127.0.0.1"}) t.Cleanup(pool.CloseIdleConnections) msg := new(dns.Msg) @@ -797,7 +797,7 @@ func TestDoQResolve_PreservesWrongQuestion(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - pool := newDOQConnPool(uc, []string{"127.0.0.1"}) + pool := newDOQConnPool(ctx, uc, []string{"127.0.0.1"}) t.Cleanup(pool.CloseIdleConnections) msg := new(dns.Msg)