fix(doh,doq): reject oversized upstream DNS responses

DoH, DoH3, and DoQ response paths previously used io.ReadAll on attacker-controlled upstream responses before enforcing any protocol-level size limit. A malicious or compromised upstream could return an oversized body or stream and force ctrld to buffer unbounded data before eventually failing DNS parsing. Cap DoH/DoH3 response bodies at dns.MaxMsgSize and cap DoQ streams at the 2-byte length prefix plus dns.MaxMsgSize. Also limit non-200 DoH error bodies so error formatting cannot consume large upstream responses.
2026-07-16 13:17:19 +02:00 · 2026-05-25 18:12:09 +07:00
parent 35455eb0b9
commit 3fe9b27fb4
4 changed files with 299 additions and 5 deletions
@@ -12,6 +12,7 @@ import (
 	"net/url"
 	"runtime"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"

@@ -266,3 +267,221 @@ func newTestHTTP3Server(t *testing.T, handler http.Handler) *testHTTP3Server {

 	return h3Server
 }
+
+// oversizedDoHHandler streams `bodyBytes` bytes of zeros with the given
+// HTTP status. The atomic counter records bytes the handler actually
+// wrote, so tests can confirm the client tore down the stream before
+// consuming the whole attacker-controlled body.
+func oversizedDoHHandler(status int, bodyBytes int64, written *atomic.Int64) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", headerApplicationDNS)
+		w.WriteHeader(status)
+		chunk := make([]byte, 64*1024)
+		var sent int64
+		for sent < bodyBytes {
+			n := int64(len(chunk))
+			if remaining := bodyBytes - sent; remaining < n {
+				n = remaining
+			}
+			m, err := w.Write(chunk[:n])
+			if err != nil {
+				return
+			}
+			sent += int64(m)
+			if written != nil {
+				written.Add(int64(m))
+			}
+			if f, ok := w.(http.Flusher); ok {
+				f.Flush()
+			}
+		}
+	}
+}
+
+// dohUpstreamForTLSServer wires an UpstreamConfig at a local httptest TLS
+// server, trusting its self-signed certificate. BootstrapIP is set so no
+// real DNS lookup runs.
+func dohUpstreamForTLSServer(t *testing.T, srv *httptest.Server) *UpstreamConfig {
+	t.Helper()
+	pool := x509.NewCertPool()
+	pool.AddCert(srv.Certificate())
+	u, err := url.Parse(srv.URL)
+	if err != nil {
+		t.Fatalf("parse server URL: %v", err)
+	}
+	uc := &UpstreamConfig{
+		Name:        "doh-oversize",
+		Type:        ResolverTypeDOH,
+		Endpoint:    srv.URL + "/dns-query",
+		BootstrapIP: u.Hostname(),
+		Timeout:     2000,
+	}
+	uc.SetCertPool(pool)
+	uc.Init()
+	return uc
+}
+
+// doh3UpstreamForAddr wires an UpstreamConfig at a local HTTP/3 server,
+// trusting its self-signed certificate.
+func doh3UpstreamForAddr(t *testing.T, addr string, cert *x509.Certificate) *UpstreamConfig {
+	t.Helper()
+	pool := x509.NewCertPool()
+	pool.AddCert(cert)
+	host, _, err := net.SplitHostPort(addr)
+	if err != nil {
+		t.Fatalf("split host/port %q: %v", addr, err)
+	}
+	uc := &UpstreamConfig{
+		Name:        "doh3-oversize",
+		Type:        ResolverTypeDOH3,
+		Endpoint:    "h3://" + addr + "/dns-query",
+		BootstrapIP: host,
+		Timeout:     5000,
+	}
+	uc.SetCertPool(pool)
+	uc.Init()
+	return uc
+}
+
+// TestDoHResolve_OversizedBody_Rejected locks in the fix for
+// github.com/Control-D-Inc/ctrld/issues/312: a malicious DoH upstream
+// returning a body larger than the DNS protocol allows must be rejected
+// with an explicit size error rather than buffered into ctrld memory.
+func TestDoHResolve_OversizedBody_Rejected(t *testing.T) {
+	const oversized = 2 * 1024 * 1024 // far past dohMaxResponseSize (~64 KiB)
+	var written atomic.Int64
+	srv := httptest.NewUnstartedServer(oversizedDoHHandler(http.StatusOK, oversized, &written))
+	testCert := generateTestCertificate(t)
+	srv.TLS = &tls.Config{
+		Certificates: []tls.Certificate{testCert.tlsCert},
+		NextProtos:   []string{"h2", "http/1.1"},
+		MinVersion:   tls.VersionTLS12,
+	}
+	srv.StartTLS()
+	t.Cleanup(srv.Close)
+
+	uc := dohUpstreamForTLSServer(t, srv)
+	r, err := NewResolver(uc)
+	if err != nil {
+		t.Fatalf("NewResolver: %v", err)
+	}
+
+	msg := new(dns.Msg)
+	msg.SetQuestion("example.com.", dns.TypeA)
+	msg.RecursionDesired = true
+
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	answer, err := r.Resolve(ctx, msg)
+	if err == nil {
+		t.Fatalf("Resolve unexpectedly succeeded; answer=%v", answer)
+	}
+	if !strings.Contains(err.Error(), "maximum DNS message size") {
+		t.Fatalf("error %q does not mention the size cap", err)
+	}
+	if answer != nil {
+		t.Fatalf("Resolve returned non-nil answer alongside error: %v", answer)
+	}
+	if got := written.Load(); got >= int64(oversized) {
+		t.Fatalf("server wrote the entire %d-byte body before client tore down (wrote=%d) — cap not effective", oversized, got)
+	}
+}
+
+// TestDoHResolve_NonOKStatus_BoundedErrorBody locks in that a non-200
+// response with a huge body does not pull the body fully into ctrld
+// memory just to format an error string.
+func TestDoHResolve_NonOKStatus_BoundedErrorBody(t *testing.T) {
+	const huge = 8 * 1024 * 1024
+	var written atomic.Int64
+	srv := httptest.NewUnstartedServer(oversizedDoHHandler(http.StatusBadGateway, huge, &written))
+	testCert := generateTestCertificate(t)
+	srv.TLS = &tls.Config{
+		Certificates: []tls.Certificate{testCert.tlsCert},
+		NextProtos:   []string{"h2", "http/1.1"},
+		MinVersion:   tls.VersionTLS12,
+	}
+	srv.StartTLS()
+	t.Cleanup(srv.Close)
+
+	uc := dohUpstreamForTLSServer(t, srv)
+	r, err := NewResolver(uc)
+	if err != nil {
+		t.Fatalf("NewResolver: %v", err)
+	}
+
+	msg := new(dns.Msg)
+	msg.SetQuestion("example.com.", dns.TypeA)
+	msg.RecursionDesired = true
+
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	answer, err := r.Resolve(ctx, msg)
+	if err == nil {
+		t.Fatalf("Resolve unexpectedly succeeded; answer=%v", answer)
+	}
+	if !strings.Contains(err.Error(), "status: 502") {
+		t.Fatalf("error %q does not surface the upstream status", err)
+	}
+	if answer != nil {
+		t.Fatalf("Resolve returned non-nil answer alongside error: %v", answer)
+	}
+	if got := written.Load(); got > 1024*1024 {
+		t.Fatalf("server wrote %d bytes before client tore down — error path is reading too much body", got)
+	}
+}
+
+// TestDoHResolve_OversizedBody_DoH3 mirrors the DoH oversized-body check
+// on the HTTP/3 transport, since github-312 specifically reproduced the
+// OOM via DoH3.
+func TestDoHResolve_OversizedBody_DoH3(t *testing.T) {
+	const oversized = 2 * 1024 * 1024
+	testCert := generateTestCertificate(t)
+	udpConn, err := net.ListenUDP("udp", &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: 0})
+	if err != nil {
+		t.Fatalf("udp listen: %v", err)
+	}
+	h3 := &http3.Server{
+		Handler: oversizedDoHHandler(http.StatusOK, oversized, nil),
+		TLSConfig: &tls.Config{
+			Certificates: []tls.Certificate{testCert.tlsCert},
+			NextProtos:   []string{"h3"},
+			MinVersion:   tls.VersionTLS12,
+		},
+	}
+	go func() {
+		if err := h3.Serve(udpConn); err != nil && !errors.Is(err, http.ErrServerClosed) {
+			t.Logf("h3 server: %v", err)
+		}
+	}()
+	t.Cleanup(func() {
+		_ = h3.Close()
+		_ = udpConn.Close()
+	})
+	time.Sleep(100 * time.Millisecond)
+
+	uc := doh3UpstreamForAddr(t, udpConn.LocalAddr().String(), testCert.cert)
+	r, err := NewResolver(uc)
+	if err != nil {
+		t.Fatalf("NewResolver: %v", err)
+	}
+
+	msg := new(dns.Msg)
+	msg.SetQuestion("example.com.", dns.TypeA)
+	msg.RecursionDesired = true
+
+	ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
+	defer cancel()
+
+	answer, err := r.Resolve(ctx, msg)
+	if err == nil {
+		t.Fatalf("Resolve unexpectedly succeeded; answer=%v", answer)
+	}
+	if !strings.Contains(err.Error(), "maximum DNS message size") {
+		t.Fatalf("error %q does not mention the size cap", err)
+	}
+	if answer != nil {
+		t.Fatalf("Resolve returned non-nil answer alongside error: %v", answer)
+	}
+}