bdns: error when CAA query response is truncated (#8839)

This change ensures Boulder never trusts a CAA DNS query response when
the TC (truncated) bit is set. Because the truncated portion may contain
a valid CAA record that forbids issuance, it is not safe to trust a
partial response. A truncated record like `CAA 0 issue ";"` could be
safely ignored given a non-truncated `CAA 0 issue "letsencrypt.org"`;
but if a record with an unknown/unsupported Property tag and the
critical bit set, e.g. `CAA 128 unknown "foo"`, is ever truncated,
that's a definitive misissuance per RFC 8659, [Section
4.1](https://datatracker.ietf.org/doc/html/rfc8659#section-4.1-6.2.2.2).

> __Bit 0, Issuer Critical Flag:__ If the value is set to "1", the
Property is critical. A CA __MUST NOT__ issue certificates for any FQDN
if the Relevant RRset for that FQDN contains a CAA critical Property for
an unknown or unsupported Property Tag.


In practice, it would require a freak accident DNS misconfiguration or a
malicious construction of records (an exercise left to the reader) to
trigger this bug. And in any case, doing so doesn't grant an attacker
any issuance capabilities they don't already have. If you have access to
a domain's DNS zone to exploit this bug, you can already edit CAA
records and validate DNS-01 challenges sufficient to issue a legitimate
certificate.

Nevertheless, this bug represents an attacker-controlled compliance risk
and we should patch it.

Author: Preston12321

bdns/dns.go

@@ -304,7 +304,9 @@ func (c *impl) LookupCAA(ctx context.Context, hostname string) (*Result[*dns.CAA
 	// for DNS-01 challenge) and then removed after validation but before CAA
 	// rechecking. But allow NXDOMAIN for TLDs to fall through to the error code
 	// below, so we don't issue for gTLDs that have been removed by ICANN.
-	if err == nil && resp.Rcode == dns.RcodeNameError && strings.Contains(hostname, ".") {
+	// Truncated responses also fall through, since we can't definitively trust
+	// an incomplete response to accurately reflect an NXDOMAIN.
+	if err == nil && !resp.Truncated && resp.Rcode == dns.RcodeNameError && strings.Contains(hostname, ".") {
 		return resultFromMsg[*dns.CAA](resp), resolver, nil
 	}
 

bdns/dns_test.go

@@ -543,7 +543,7 @@ func TestDNSNXDOMAIN(t *testing.T) {
 	test.AssertContains(t, err.Error(), "NXDOMAIN looking up AAAA for")
 
 	_, _, err = obj.LookupTXT(context.Background(), hostname)
-	expected := Error{dns.TypeTXT, hostname, nil, dns.RcodeNameError, nil}
+	expected := Error{dns.TypeTXT, hostname, nil, dns.RcodeNameError, nil, false}
 	test.AssertDeepEquals(t, err, expected)
 }
 
@@ -961,3 +961,39 @@ func TestDOHMetric(t *testing.T) {
 	// Now, we should count 1 "out of retries" errors.
 	test.AssertMetricWithLabelsEquals(t, resolver.timeoutCounter, prometheus.Labels{"qtype": "None", "type": "out of retries", "resolver": "127.0.0.1", "isTLD": "false"}, 1)
 }
+
+// truncatedExchanger returns a truncated (TC bit set) response with the given
+// Rcode. If a caller failed to check for truncation on a CAA query, it would
+// otherwise be fooled into trusting an incomplete set of records, potentially
+// missing an issue record that would forbid issuance.
+type truncatedExchanger struct {
+	rcode int
+}
+
+func (te truncatedExchanger) ExchangeContext(_ context.Context, m *dns.Msg, _ string) (*dns.Msg, time.Duration, error) {
+	resp := new(dns.Msg)
+	resp.SetReply(m)
+	resp.Rcode = te.rcode
+	resp.Truncated = true
+	return resp, time.Millisecond, nil
+}
+
+func TestDNSCAATruncatedResponse(t *testing.T) {
+	staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
+	test.AssertNotError(t, err, "Got error creating StaticProvider")
+
+	client := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.NewMock(), tlsConfig)
+	client.(*impl).exchanger = truncatedExchanger{rcode: dns.RcodeSuccess}
+
+	_, _, err = client.LookupCAA(context.Background(), "example.com")
+	test.AssertError(t, err, "expected error for truncated CAA response")
+	test.AssertContains(t, err.Error(), "response was truncated")
+
+	// A truncated NXDOMAIN response must not be treated as the usual
+	// NXDOMAIN-as-empty-CAA-set special case for non-TLD names: we can't
+	// trust an incomplete response to accurately reflect an NXDOMAIN.
+	client.(*impl).exchanger = truncatedExchanger{rcode: dns.RcodeNameError}
+	_, _, err = client.LookupCAA(context.Background(), "nonexistent.letsencrypt.org")
+	test.AssertError(t, err, "expected error for truncated CAA response, even when NXDOMAIN-shaped")
+	test.AssertContains(t, err.Error(), "response was truncated")
+}

bdns/problem.go

@@ -14,13 +14,18 @@ import (
 type Error struct {
 	recordType uint16
 	hostname   string
-	// Exactly one of rCode or underlying should be set.
+	// Exactly one of underlying, rCode, or truncated should be set.
 	underlying error
 	rCode      int
 
 	// Optional: If the resolver returned extended error information, it will be stored here.
 	// https://www.rfc-editor.org/rfc/rfc8914
 	extended *dns.EDNS0_EDE
+
+	// truncated is set when the response to a CAA query had the TC bit set. We
+	// don't implement fallback to TCP, so we treat a truncated response as an
+	// error rather than risk silently acting on an incomplete set of records.
+	truncated bool
 }
 
 // extendedDNSError returns non-nil if the input message contained an OPT RR
@@ -39,8 +44,10 @@ func extendedDNSError(msg *dns.Msg) *dns.EDNS0_EDE {
 	return nil
 }
 
-// wrapErr returns a non-nil error if err is non-nil or if resp.Rcode is not dns.RcodeSuccess.
-// The error includes appropriate details about the DNS query that failed.
+// wrapErr returns a non-nil error if err is non-nil, if resp.Rcode is not
+// dns.RcodeSuccess, or if resp was truncated (the TC bit was set) for a CAA
+// query. The error includes appropriate details about the DNS query that
+// failed.
 func wrapErr(queryType uint16, hostname string, resp *dns.Msg, err error) error {
 	if err != nil {
 		return Error{
@@ -50,6 +57,13 @@ func wrapErr(queryType uint16, hostname string, resp *dns.Msg, err error) error
 			extended:   nil,
 		}
 	}
+	if queryType == dns.TypeCAA && resp.Truncated {
+		return Error{
+			recordType: queryType,
+			hostname:   hostname,
+			truncated:  true,
+		}
+	}
 	if resp.Rcode != dns.RcodeSuccess {
 		return Error{
 			recordType: queryType,
@@ -120,6 +134,8 @@ func (d Error) Error() string {
 		} else {
 			detail = detailServerFailure
 		}
+	} else if d.truncated {
+		detail = detailDNSTruncated
 	} else if d.rCode != dns.RcodeSuccess {
 		detail = dns.RcodeToString[d.rCode]
 		if explanation, ok := rcodeExplanations[d.rCode]; ok {
@@ -150,6 +166,7 @@ const detailDNSTimeout = "query timed out"
 const detailCanceled = "query timed out (and was canceled)"
 const detailDNSNetFailure = "networking error"
 const detailServerFailure = "server failure at resolver"
+const detailDNSTruncated = "response was truncated"
 
 // rcodeExplanations provide additional friendly explanatory text to be included in DNS
 // error messages, for select inscrutable RCODEs.

bdns/problem_test.go

@@ -18,41 +18,44 @@ func TestError(t *testing.T) {
 		expected string
 	}{
 		{
-			&Error{dns.TypeMX, "hostname", &net.OpError{Err: errors.New("some net error")}, -1, nil},
+			&Error{dns.TypeMX, "hostname", &net.OpError{Err: errors.New("some net error")}, -1, nil, false},
 			"DNS problem: networking error looking up MX for hostname",
 		}, {
-			&Error{dns.TypeTXT, "hostname", nil, dns.RcodeNameError, nil},
+			&Error{dns.TypeTXT, "hostname", nil, dns.RcodeNameError, nil, false},
 			"DNS problem: NXDOMAIN looking up TXT for hostname - check that a DNS record exists for this domain",
 		}, {
-			&Error{dns.TypeTXT, "hostname", context.DeadlineExceeded, -1, nil},
+			&Error{dns.TypeTXT, "hostname", context.DeadlineExceeded, -1, nil, false},
 			"DNS problem: query timed out looking up TXT for hostname",
 		}, {
-			&Error{dns.TypeTXT, "hostname", context.Canceled, -1, nil},
+			&Error{dns.TypeTXT, "hostname", context.Canceled, -1, nil, false},
 			"DNS problem: query timed out (and was canceled) looking up TXT for hostname",
 		}, {
-			&Error{dns.TypeCAA, "hostname", nil, dns.RcodeServerFailure, nil},
+			&Error{dns.TypeCAA, "hostname", nil, dns.RcodeServerFailure, nil, false},
 			"DNS problem: SERVFAIL looking up CAA for hostname - the domain's nameservers may be malfunctioning",
 		}, {
-			&Error{dns.TypeA, "hostname", nil, dns.RcodeServerFailure, &dns.EDNS0_EDE{InfoCode: 1, ExtraText: "oh no"}},
+			&Error{dns.TypeA, "hostname", nil, dns.RcodeServerFailure, &dns.EDNS0_EDE{InfoCode: 1, ExtraText: "oh no"}, false},
 			"DNS problem: looking up A for hostname: DNSSEC: Unsupported DNSKEY Algorithm: oh no",
 		}, {
-			&Error{dns.TypeA, "hostname", nil, dns.RcodeServerFailure, &dns.EDNS0_EDE{InfoCode: 6, ExtraText: ""}},
+			&Error{dns.TypeA, "hostname", nil, dns.RcodeServerFailure, &dns.EDNS0_EDE{InfoCode: 6, ExtraText: ""}, false},
 			"DNS problem: looking up A for hostname: DNSSEC: Bogus",
 		}, {
-			&Error{dns.TypeA, "hostname", nil, dns.RcodeServerFailure, &dns.EDNS0_EDE{InfoCode: 1337, ExtraText: "mysterious"}},
+			&Error{dns.TypeA, "hostname", nil, dns.RcodeServerFailure, &dns.EDNS0_EDE{InfoCode: 1337, ExtraText: "mysterious"}, false},
 			"DNS problem: looking up A for hostname: Unknown Extended DNS Error code 1337: mysterious",
 		}, {
-			&Error{dns.TypeCAA, "hostname", nil, dns.RcodeServerFailure, nil},
+			&Error{dns.TypeCAA, "hostname", nil, dns.RcodeServerFailure, nil, false},
 			"DNS problem: SERVFAIL looking up CAA for hostname - the domain's nameservers may be malfunctioning",
 		}, {
-			&Error{dns.TypeCAA, "hostname", nil, dns.RcodeServerFailure, nil},
+			&Error{dns.TypeCAA, "hostname", nil, dns.RcodeServerFailure, nil, false},
 			"DNS problem: SERVFAIL looking up CAA for hostname - the domain's nameservers may be malfunctioning",
 		}, {
-			&Error{dns.TypeA, "hostname", nil, dns.RcodeFormatError, nil},
+			&Error{dns.TypeA, "hostname", nil, dns.RcodeFormatError, nil, false},
 			"DNS problem: FORMERR looking up A for hostname",
 		}, {
-			&Error{dns.TypeA, "hostname", &url.Error{Op: "GET", URL: "https://example.com/", Err: dohTimeoutError{}}, -1, nil},
+			&Error{dns.TypeA, "hostname", &url.Error{Op: "GET", URL: "https://example.com/", Err: dohTimeoutError{}}, -1, nil, false},
 			"DNS problem: query timed out looking up A for hostname",
+		}, {
+			&Error{dns.TypeCAA, "hostname", nil, dns.RcodeSuccess, nil, true},
+			"DNS problem: response was truncated looking up CAA for hostname",
 		},
 	}
 	for _, tc := range testCases {
@@ -87,4 +90,14 @@ func TestWrapErr(t *testing.T) {
 		MsgHdr: dns.MsgHdr{Rcode: dns.RcodeSuccess},
 	}, errors.New("oh no"))
 	test.AssertError(t, err, "expected error")
+
+	// A truncated response should be treated as an error, even though its
+	// Rcode is RcodeSuccess: a truncated CAA response could be silently
+	// missing the issue/issuewild records that would otherwise forbid
+	// issuance.
+	err = wrapErr(dns.TypeCAA, "hostname", &dns.Msg{
+		MsgHdr: dns.MsgHdr{Rcode: dns.RcodeSuccess, Truncated: true},
+	}, nil)
+	test.AssertError(t, err, "expected error for truncated response")
+	test.AssertContains(t, err.Error(), "response was truncated")
 }