fix(doh,doq): port oversized response caps to master

2026-07-16 13:17:19 +02:00 · 2026-05-25 18:12:09 +07:00
parent 3c740b9693
commit f281466118
4 changed files with 299 additions and 6 deletions
@@ -25,6 +25,16 @@ const (
 	dohOsHeader           = "x-cd-os"
 	dohClientIDPrefHeader = "x-cd-cpref"
 	headerApplicationDNS  = "application/dns-message"
+
+	// dohMaxResponseSize caps the response body read from a DoH/DoH3
+	// upstream. A DNS message is bounded by the protocol's 16-bit length
+	// field; anything larger cannot be a valid response. The cap stops a
+	// malicious or compromised upstream from driving ctrld into unbounded
+	// memory growth via io.ReadAll on attacker-controlled bytes.
+	dohMaxResponseSize = dns.MaxMsgSize
+	// dohMaxErrorBodySize bounds how much of a non-200 response body is
+	// read for inclusion in the returned error.
+	dohMaxErrorBodySize = 1024
 )

 // EncodeOsNameMap provides mapping from OS name to a shorter string, used for encoding x-cd-os value.
@@ -142,15 +152,18 @@ func (r *dohResolver) Resolve(ctx context.Context, msg *dns.Msg) (*dns.Msg, erro
 	}
 	defer resp.Body.Close()

-	buf, err := io.ReadAll(resp.Body)
+	if resp.StatusCode != http.StatusOK {
+		body, _ := io.ReadAll(io.LimitReader(resp.Body, dohMaxErrorBodySize))
+		return nil, fmt.Errorf("wrong response from DOH server, got: %s, status: %d", string(body), resp.StatusCode)
+	}
+
+	buf, err := io.ReadAll(io.LimitReader(resp.Body, dohMaxResponseSize+1))
 	if err != nil {
 		Log(ctx, logger.Error().Err(err), "Could not read response body")
 		return nil, fmt.Errorf("could not read message from response: %w", err)
 	}
-
-	if resp.StatusCode != http.StatusOK {
-		Log(ctx, logger.Error(), "Wrong response from DOH server, got: %s, status: %d", string(buf), resp.StatusCode)
-		return nil, fmt.Errorf("wrong response from DOH server, got: %s, status: %d", string(buf), resp.StatusCode)
+	if len(buf) > dohMaxResponseSize {
+		return nil, fmt.Errorf("DoH response exceeds %d-byte maximum DNS message size", dohMaxResponseSize)
 	}

 	answer := new(dns.Msg)
@@ -12,6 +12,7 @@ import (
 	"net/url"
 	"runtime"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"

@@ -267,3 +268,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(context.Background())
+	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(context.Background())
+	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(context.Background(), 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(context.Background(), 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(context.Background(), 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)
+	}
+}
@@ -17,6 +17,12 @@ import (
 	"github.com/quic-go/quic-go"
 )

+// doqMaxResponseSize caps the bytes read from a DoQ stream: a 2-byte
+// length prefix plus a DNS message bounded by dns.MaxMsgSize. Anything
+// larger cannot be a valid response and is rejected before buffering more
+// data from the upstream.
+const doqMaxResponseSize = 2 + dns.MaxMsgSize
+
 type doqResolver struct {
 	uc *UpstreamConfig
 }
@@ -200,7 +206,13 @@ func (p *doqConnPool) doResolve(ctx context.Context, msg *dns.Msg) (*dns.Msg, er
 		return nil, err
 	}

-	buf, err := io.ReadAll(stream)
+	// A DoQ response is a 2-byte length prefix followed by a DNS message.
+	// The DNS message is bounded by the protocol at dns.MaxMsgSize, so a
+	// well-formed response is at most doqMaxResponseSize bytes. Read one
+	// byte past that cap to distinguish "at limit" from "over limit" and
+	// reject oversized responses before they can drive memory growth from
+	// a malicious or compromised upstream.
+	buf, err := io.ReadAll(io.LimitReader(stream, doqMaxResponseSize+1))
 	if err != nil {
 		p.putConn(conn, false)
 		return nil, err
@@ -212,6 +224,11 @@ func (p *doqConnPool) doResolve(ctx context.Context, msg *dns.Msg) (*dns.Msg, er
 		return nil, io.EOF
 	}

+	if len(buf) > doqMaxResponseSize {
+		p.putConn(conn, false)
+		return nil, fmt.Errorf("DoQ response exceeds %d-byte maximum", doqMaxResponseSize)
+	}
+
 	// RFC 9250: each DoQ DNS message is encoded as a 2-octet length field
 	// followed by the DNS message. Reject responses that are shorter than
 	// the prefix or whose prefix declares more bytes than were received,
@@ -670,3 +670,47 @@ func countOpenFDs(t *testing.T) int {
 	}
 	return len(entries)
 }
+
+// TestDoQResolve_OversizedResponse_Rejected locks in the fix for
+// github.com/Control-D-Inc/ctrld/issues/312 on the DoQ transport: a
+// malicious upstream that writes a response larger than the DNS protocol
+// allows must be rejected with an explicit size error, not buffered
+// without bound into ctrld memory.
+func TestDoQResolve_OversizedResponse_Rejected(t *testing.T) {
+	t.Parallel()
+
+	// doqMaxResponseSize is 2 + dns.MaxMsgSize. Send something well past
+	// that. 256 KiB is enough to exceed the cap while keeping the test
+	// fast on loopback.
+	response := make([]byte, 256*1024)
+	// A well-formed length prefix isn't required: the size cap should
+	// fire before any framing check runs. Use a non-zero prefix so the
+	// test also documents that the order of validation is "size first,
+	// framing later."
+	response[0] = 0xFF
+	response[1] = 0xFF
+
+	server := newMalformedDoQServer(t, response)
+	uc := newMalformedDoQUpstream(t, server.cert, server.addr)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	pool := newDOQConnPool(context.Background(), 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 unexpectedly succeeded for oversized response; answer=%v", answer)
+	}
+	if !strings.Contains(err.Error(), "exceeds") {
+		t.Fatalf("error %q does not surface the size cap", err)
+	}
+	if answer != nil {
+		t.Fatalf("Resolve returned non-nil answer alongside error: %v", answer)
+	}
+}