Send only usable trust anchor IDs in EncryptedExtensions
CL originally by Bob Beck.
We did not filter this list to things that would be usable in the
handshake. This allows the client to not bother retrying with a
credential that wouldn't be usable anyway.
Fixed: 402692373
Change-Id: I78850ada5014bfd18235cfe5463fa2973da91a30
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/79188
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/ssl/extensions.cc b/ssl/extensions.cc
index 38205c5..ec505e3 100644
--- a/ssl/extensions.cc
+++ b/ssl/extensions.cc
@@ -2681,24 +2681,26 @@
static bool ext_trust_anchors_add_serverhello(SSL_HANDSHAKE *hs, CBB *out) {
SSL *const ssl = hs->ssl;
- const auto &creds = hs->config->cert->credentials;
- if (ssl_protocol_version(ssl) < TLS1_3_VERSION ||
- // Check if any credentials have trust anchor IDs.
- std::none_of(creds.begin(), creds.end(), [](const auto &cred) {
- return !cred->trust_anchor_id.empty();
- })) {
+ if (ssl_protocol_version(ssl) < TLS1_3_VERSION) {
return true;
}
- CBB contents, list;
- if (!CBB_add_u16(out, TLSEXT_TYPE_trust_anchors) || //
- !CBB_add_u16_length_prefixed(out, &contents) || //
+
+ const size_t old_len = CBB_len(out);
+ CBB contents, list, child;
+ if (!CBB_add_u16(out, TLSEXT_TYPE_trust_anchors) ||
+ !CBB_add_u16_length_prefixed(out, &contents) ||
!CBB_add_u16_length_prefixed(&contents, &list)) {
return false;
}
- for (const auto &cred : creds) {
+ for (const auto &cred : hs->config->cert->credentials) {
if (!cred->trust_anchor_id.empty()) {
- CBB child;
- if (!CBB_add_u8_length_prefixed(&list, &child) || //
+ uint16_t unused_sigalg;
+ if (!ssl_check_tls13_credential_ignoring_issuer(hs, cred.get(),
+ &unused_sigalg)) {
+ ERR_clear_error();
+ continue;
+ }
+ if (!CBB_add_u8_length_prefixed(&list, &child) ||
!CBB_add_bytes(&child, cred->trust_anchor_id.data(),
cred->trust_anchor_id.size()) ||
!CBB_flush(&list)) {
@@ -2706,7 +2708,17 @@
}
}
}
- return CBB_flush(out);
+
+ bool empty = CBB_len(&list) == 0;
+ if (!CBB_flush(out)) {
+ return false;
+ }
+
+ // Don't send the extension if it would have been empty.
+ if (empty) {
+ CBB_discard(out, CBB_len(out) - old_len);
+ }
+ return true;
}
static bool ext_trust_anchors_parse_serverhello(SSL_HANDSHAKE *hs,
diff --git a/ssl/internal.h b/ssl/internal.h
index 86670d0..fe41e49 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1550,6 +1550,15 @@
bool ssl_credential_matches_requested_issuers(SSL_HANDSHAKE *hs,
const SSL_CREDENTIAL *cred);
+// ssl_check_tls13_credential_ignoring_issuer returns true if |cred| is usable
+// as the certificate in a TLS 1.3 handshake, ignoring the issuer check.
+// |out_sigalg| will be set to a matching signature algorithm if true is
+// returned.
+bool ssl_check_tls13_credential_ignoring_issuer(SSL_HANDSHAKE *hs,
+ const SSL_CREDENTIAL *cred,
+ uint16_t *out_sigalg);
+
+
// Handshake functions.
enum ssl_hs_wait_t {
diff --git a/ssl/test/runner/delegated_credential_tests.go b/ssl/test/runner/delegated_credential_tests.go
index a39c3ed..84506a5 100644
--- a/ssl/test/runner/delegated_credential_tests.go
+++ b/ssl/test/runner/delegated_credential_tests.go
@@ -297,4 +297,24 @@
},
})
+ // Delegated credentials participate in trust anchor IDs.
+ id1 := []byte{1, 1}
+ id2 := []byte{2, 2, 2}
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "DelegatedCredentials-TrustAnchorIDs",
+ config: Config{
+ RequestTrustAnchors: [][]byte{id2},
+ DelegatedCredentialAlgorithms: []signatureAlgorithm{signatureECDSAWithP256AndSHA256},
+ Bugs: ProtocolBugs{
+ ExpectPeerAvailableTrustAnchors: [][]byte{id1, id2},
+ },
+ },
+ shimCredentials: []*Credential{
+ p256DC.WithTrustAnchorID(id1), p256DCFromECDSA.WithTrustAnchorID(id2)},
+ flags: []string{"-expect-selected-credential", "1"},
+ expectations: connectionExpectations{
+ peerCertificate: p256DCFromECDSA,
+ },
+ })
}
diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go
index 71aa840..7eecb06 100644
--- a/ssl/test/runner/handshake_messages.go
+++ b/ssl/test/runner/handshake_messages.go
@@ -1183,7 +1183,7 @@
}
case extensionTrustAnchors:
// An empty list is allowed here.
- if !parseTrustAnchors(&body, &m.trustAnchors) {
+ if !parseTrustAnchors(&body, &m.trustAnchors) || len(body) != 0 {
return false
}
}
@@ -1876,7 +1876,8 @@
if version < VersionTLS13 {
return false
}
- if !parseTrustAnchors(&body, &m.trustAnchors) || len(body) != 0 {
+ // The list cannot be empty here. If empty, the peer should have omitted the extension.
+ if !parseTrustAnchors(&body, &m.trustAnchors) || len(m.trustAnchors) == 0 || len(body) != 0 {
return false
}
default:
diff --git a/ssl/test/runner/trust_anchor_tests.go b/ssl/test/runner/trust_anchor_tests.go
index bdb7812..f3f8462 100644
--- a/ssl/test/runner/trust_anchor_tests.go
+++ b/ssl/test/runner/trust_anchor_tests.go
@@ -28,6 +28,7 @@
id1 := []byte{1}
id2 := []byte{2, 2}
id3 := []byte{3, 3, 3}
+ id4 := []byte{4, 4, 4, 4}
// Unsolicited trust_anchors extensions should be rejected.
testCases = append(testCases, testCase{
@@ -206,6 +207,58 @@
flags: []string{"-expect-selected-credential", "2"},
})
+ // When the server sends available trust anchors, it should filter the list
+ // by whether the credential would be usable with the connection at all.
+ p256DC := createDelegatedCredential(&rsaCertificate, delegatedCredentialConfig{
+ dcAlgo: signatureECDSAWithP256AndSHA256,
+ algo: signatureRSAPSSWithSHA256,
+ })
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "TrustAnchors-Server-FilterAvailable",
+ config: Config{
+ MinVersion: VersionTLS13,
+ RequestTrustAnchors: [][]byte{id1},
+ VerifySignatureAlgorithms: []signatureAlgorithm{signatureRSAPSSWithSHA256, signatureECDSAWithP256AndSHA256},
+ Bugs: ProtocolBugs{
+ ExpectPeerAvailableTrustAnchors: [][]byte{id2},
+ ExpectPeerMatchTrustAnchor: ptrTo(false),
+ },
+ },
+ shimCredentials: []*Credential{
+ rsaCertificate.WithTrustAnchorID(id2),
+ // Ineligible because of signature algorithms.
+ rsaCertificate.WithTrustAnchorID(id3).WithSignatureAlgorithms(signatureRSAPSSWithSHA384),
+ // Ineligible because of credential type.
+ p256DC.WithTrustAnchorID(id4),
+ &rsaCertificate,
+ },
+ flags: []string{"-expect-selected-credential", "3"},
+ })
+
+ // After filtering, the list of available trust anchors may even be empty.
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "TrustAnchors-Server-FilterAvailable-Empty",
+ config: Config{
+ MinVersion: VersionTLS13,
+ RequestTrustAnchors: [][]byte{id1},
+ VerifySignatureAlgorithms: []signatureAlgorithm{signatureRSAPSSWithSHA256, signatureECDSAWithP256AndSHA256},
+ Bugs: ProtocolBugs{
+ ExpectPeerAvailableTrustAnchors: [][]byte{},
+ ExpectPeerMatchTrustAnchor: ptrTo(false),
+ },
+ },
+ shimCredentials: []*Credential{
+ // Ineligible because of signature algorithms.
+ rsaCertificate.WithTrustAnchorID(id2).WithSignatureAlgorithms(signatureRSAPSSWithSHA384),
+ // Ineligible because of credential type.
+ p256DC.WithTrustAnchorID(id4),
+ &rsaCertificate,
+ },
+ flags: []string{"-expect-selected-credential", "2"},
+ })
+
// The ClientHello list may be empty. The client must be able to send it and
// receive available trust anchors.
testCases = append(testCases, testCase{
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc
index 98bac6e..185d26a 100644
--- a/ssl/tls13_server.cc
+++ b/ssl/tls13_server.cc
@@ -256,9 +256,9 @@
return true;
}
-static bool check_signature_credential(SSL_HANDSHAKE *hs,
- const SSL_CREDENTIAL *cred,
- uint16_t *out_sigalg) {
+bool ssl_check_tls13_credential_ignoring_issuer(SSL_HANDSHAKE *hs,
+ const SSL_CREDENTIAL *cred,
+ uint16_t *out_sigalg) {
switch (cred->type) {
case SSLCredentialType::kX509:
break;
@@ -279,12 +279,16 @@
// If we reach here then the credential requires a signature. If |cred| is a
// delegated credential, this also checks that the peer supports delegated
// credentials and matched |dc_cert_verify_algorithm|.
- if (!tls1_choose_signature_algorithm(hs, cred, out_sigalg)) {
- return false;
- }
- // Use this credential if it either matches a requested issuer,
- // or does not require issuer matching.
- return ssl_credential_matches_requested_issuers(hs, cred);
+ return tls1_choose_signature_algorithm(hs, cred, out_sigalg);
+}
+
+static bool check_signature_credential(SSL_HANDSHAKE *hs,
+ const SSL_CREDENTIAL *cred,
+ uint16_t *out_sigalg) {
+ return ssl_check_tls13_credential_ignoring_issuer(hs, cred, out_sigalg) &&
+ // Use this credential if it either matches a requested issuer,
+ // or does not require issuer matching.
+ ssl_credential_matches_requested_issuers(hs, cred);
}
static bool check_pake_credential(SSL_HANDSHAKE *hs,
