fix: wrong DoQ resolver rewriting upstream responses with SetReply

The DoQ resolver called SetReply on the already-unpacked upstream
response. SetReply is meant to build a reply from a request, so it
forces the RCODE to NOERROR and overwrites the Question with the
request's question. This masked upstream failures from the proxy's
failover logic (a SERVFAIL looked like a successful empty response) and
corrupted the Question section of the response served to clients.

Restore only the downstream transaction ID instead (RFC 9250 section
4.2.1 puts the DNS Message ID at 0 on the wire), preserving the upstream
RCODE, Question, and answer sections untouched. This matches how the DoH
and DoT resolvers return unpacked upstream responses.

Refs github.com/Control-D-Inc/ctrld/issues/322

doq.go

@@ -251,7 +251,15 @@ func (p *doqConnPool) doResolve(ctx context.Context, msg *dns.Msg) (*dns.Msg, er
 	if err := answer.Unpack(buf[2 : 2+respLen]); err != nil {
 		return nil, err
 	}
-	answer.SetReply(msg)
+	// RFC 9250 section 4.2.1 requires the DNS Message ID to be 0 on the wire,
+	// so restore the downstream transaction ID for the client. Do NOT use
+	// SetReply here: it rewrites the RCODE to NOERROR and overwrites the
+	// Question with the request's, which would mask upstream failures from the
+	// failover logic (a SERVFAIL would look like success) and let a
+	// wrong-question answer pass validation and poison the cache. Preserve the
+	// upstream RCODE, Question, and answer sections untouched so the proxy can
+	// evaluate them. See github.com/Control-D-Inc/ctrld/issues/322.
+	answer.Id = msg.Id
 	return answer, nil
 }
 

doq_test.go

@@ -714,3 +714,105 @@ func TestDoQResolve_OversizedResponse_Rejected(t *testing.T) {
 		t.Fatalf("Resolve returned non-nil answer alongside error: %v", answer)
 	}
 }
+
+// frameDoQResponse packs msg and prepends the RFC 9250 2-octet length prefix,
+// producing the exact bytes a DoQ server writes on the wire.
+func frameDoQResponse(t *testing.T, msg *dns.Msg) []byte {
+	t.Helper()
+	b, err := msg.Pack()
+	if err != nil {
+		t.Fatalf("pack response: %v", err)
+	}
+	n := uint16(len(b))
+	return append([]byte{byte(n >> 8), byte(n & 0xFF)}, b...)
+}
+
+// TestDoQResolve_PreservesRcode locks in the fix for
+// github.com/Control-D-Inc/ctrld/issues/322: the DoQ resolver must not rewrite
+// an upstream response with SetReply, which would clobber a SERVFAIL into
+// NOERROR and hide the failure from the proxy's failover logic. The upstream
+// RCODE must survive; only the transaction ID is restored for the client.
+func TestDoQResolve_PreservesRcode(t *testing.T) {
+	t.Parallel()
+
+	// RFC 9250 puts the DNS Message ID at 0 on the wire.
+	resp := new(dns.Msg)
+	resp.SetQuestion("example.com.", dns.TypeA)
+	resp.Response = true
+	resp.Id = 0
+	resp.Rcode = dns.RcodeServerFailure
+
+	server := newMalformedDoQServer(t, frameDoQResponse(t, resp))
+	uc := newMalformedDoQUpstream(t, server.cert, server.addr)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	pool := newDOQConnPool(uc, []string{"127.0.0.1"})
+	t.Cleanup(pool.CloseIdleConnections)
+
+	msg := new(dns.Msg)
+	msg.SetQuestion("example.com.", dns.TypeA)
+	msg.RecursionDesired = true
+
+	answer, err := pool.Resolve(ctx, msg)
+	if err != nil {
+		t.Fatalf("Resolve failed: %v", err)
+	}
+	if answer.Rcode != dns.RcodeServerFailure {
+		t.Fatalf("upstream SERVFAIL was rewritten to %s; failover would be bypassed",
+			dns.RcodeToString[answer.Rcode])
+	}
+	if answer.Id != msg.Id {
+		t.Fatalf("transaction ID not restored: got %d, want %d", answer.Id, msg.Id)
+	}
+}
+
+// TestDoQResolve_PreservesWrongQuestion locks in the fix for
+// github.com/Control-D-Inc/ctrld/issues/322: when an upstream answers a
+// different name than asked, the resolver must preserve the upstream's
+// question rather than rewriting it to the request's question (as SetReply
+// did). Rewriting would hide the mismatch and let wrong-domain records poison
+// the shared cache.
+func TestDoQResolve_PreservesWrongQuestion(t *testing.T) {
+	t.Parallel()
+
+	resp := new(dns.Msg)
+	resp.SetQuestion("attacker.example.", dns.TypeA)
+	resp.Response = true
+	resp.Id = 0
+	resp.Answer = append(resp.Answer, &dns.A{
+		Hdr: dns.RR_Header{
+			Name:   "attacker.example.",
+			Rrtype: dns.TypeA,
+			Class:  dns.ClassINET,
+			Ttl:    300,
+		},
+		A: net.ParseIP("192.0.2.1"),
+	})
+
+	server := newMalformedDoQServer(t, frameDoQResponse(t, resp))
+	uc := newMalformedDoQUpstream(t, server.cert, server.addr)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	pool := newDOQConnPool(uc, []string{"127.0.0.1"})
+	t.Cleanup(pool.CloseIdleConnections)
+
+	msg := new(dns.Msg)
+	msg.SetQuestion("victim.example.", dns.TypeA)
+	msg.RecursionDesired = true
+
+	answer, err := pool.Resolve(ctx, msg)
+	if err != nil {
+		t.Fatalf("Resolve failed: %v", err)
+	}
+	if len(answer.Question) == 0 || !strings.EqualFold(answer.Question[0].Name, "attacker.example.") {
+		t.Fatalf("upstream question was rewritten; got %v, want the upstream's attacker.example.",
+			answer.Question)
+	}
+	if answer.Id != msg.Id {
+		t.Fatalf("transaction ID not restored: got %d, want %d", answer.Id, msg.Id)
+	}
+}