diff --git a/doh.go b/doh.go index 23df6e3..70c4f9d 100644 --- a/doh.go +++ b/doh.go @@ -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) diff --git a/doh_test.go b/doh_test.go index 9757f5e..c9faceb 100644 --- a/doh_test.go +++ b/doh_test.go @@ -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) + } +} diff --git a/doq.go b/doq.go index f6711f3..d10188c 100644 --- a/doq.go +++ b/doq.go @@ -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, diff --git a/doq_test.go b/doq_test.go index 463f7e9..1749c3b 100644 --- a/doq_test.go +++ b/doq_test.go @@ -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) + } +}