Handle an API edge case with PSKs and client certificates If a client is configured to accept PSKs or server certificates, the server may authenticate with a certificate and then request a client certificate. The client may then wish to decline to send a client certificate. We normally express this with an empty credential list. But such a client cannot have an empty credential list because it contains PSKs. Due to the order of protocol decisions, the client's credential list is effectively two credential lists: a set of "non-certificate" credentials (PSKs, PAKEs), which can pick non-certificate modes, and then a set of "certificate" credentials. We only care if the second one is empty. In other words, skip over PSK/PAKE credentials when evaluating emptiness. They're still one combined list because, on the server, the choice is made at the same time and having one combined list is helpful to express an ordering. (For this purpose, a raw public key is a certificate credential.) Bug: 369963041 Change-Id: If7b06e2bd347a187499a8621eb692c4a87c5dbf6 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/89567 Reviewed-by: Lily Chen <chlily@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index 23dc602..137516b 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc
@@ -1317,29 +1317,37 @@ return ssl_hs_error; } - // TODO(crbug.com/369963041): It is currently impossible for a client that - // accepts PSKs or server certs to then decline to send a client cert. - if (creds.empty()) { - // If there were no credentials, proceed without a client certificate. In - // this case, the handshake buffer may be released early. - hs->transcript.FreeBuffer(); - } else { - // Select the credential to use. - for (SSL_CREDENTIAL *cred : creds) { - ERR_clear_error(); - uint16_t sigalg; - if (check_credential(hs, cred, &sigalg)) { - hs->credential = UpRef(cred); - hs->signature_algorithm = sigalg; - break; - } + // Select the credential, if any, to use. + bool may_proceed_anonymously = true; + for (SSL_CREDENTIAL *cred : creds) { + if (!cred->UsesPrivateKey()) { + // Non-certificate credentials (e.g. PSKs) do not participate in deciding + // whether to error or proceed anonymously. + continue; } - if (hs->credential == nullptr) { + + ERR_clear_error(); + may_proceed_anonymously = false; + uint16_t sigalg; + if (check_credential(hs, cred, &sigalg)) { + hs->credential = UpRef(cred); + hs->signature_algorithm = sigalg; + break; + } + } + + // Fail the connection if no credentials matched, but only if the caller + // configured at least one certificate credential. If there were no + // candidates, proceed anonymously. + if (hs->credential == nullptr) { + if (!may_proceed_anonymously) { // The error from the last attempt is in the error queue. assert(ERR_peek_error() != 0); ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE); return ssl_hs_error; } + // Without CertificateVerify, we can release the handshake buffer. + hs->transcript.FreeBuffer(); } if (!ssl_send_tls12_certificate(hs)) {
diff --git a/ssl/test/runner/psk_tests.go b/ssl/test/runner/psk_tests.go index d00cf86..36f6de6 100644 --- a/ssl/test/runner/psk_tests.go +++ b/ssl/test/runner/psk_tests.go
@@ -323,6 +323,37 @@ }) } + // When a client is configured with PSKs or certificates, the server picks certificates, + // and the server sends CertificateRequests, it must be possible for the client to + // proceed without sending any client certificate, even if the credential list has a + // PSK credential. + testCases = append(testCases, testCase{ + protocol: protocol, + name: fmt.Sprintf("PSK-Client-PSKOrCert-CertRequest-NoClientCert-%s", protocol), + testType: clientTest, + config: Config{ + MaxVersion: VersionTLS13, + Credential: &rsaCertificate, + ClientAuth: RequestClientCert, + }, + shimCredentials: []*Credential{&pskSHA256Credential}, + flags: []string{"-verify-peer"}, + }) + if protocol != quic { + testCases = append(testCases, testCase{ + protocol: protocol, + name: fmt.Sprintf("PSK-Client-PSKOrCert-CertRequest-NoClientCert-TLS12-%s", protocol), + testType: clientTest, + config: Config{ + MaxVersion: VersionTLS12, + Credential: &rsaCertificate, + ClientAuth: RequestClientCert, + }, + shimCredentials: []*Credential{&pskSHA256Credential}, + flags: []string{"-verify-peer"}, + }) + } + // The client should reject CertificateRequest messages on PSK connections. testCases = append(testCases, testCase{ protocol: protocol,
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc index 59dc387..9df39b0 100644 --- a/ssl/tls13_client.cc +++ b/ssl/tls13_client.cc
@@ -1007,27 +1007,35 @@ return ssl_hs_error; } - // TODO(crbug.com/369963041): It is currently impossible for a client that - // accepts PSKs or server certs to then decline to send a client cert. - if (!creds.empty()) { - // Select the credential to use. - for (SSL_CREDENTIAL *cred : creds) { - ERR_clear_error(); - uint16_t sigalg; - if (check_credential(hs, cred, &sigalg)) { - hs->credential = UpRef(cred); - hs->signature_algorithm = sigalg; - break; - } + // Select the credential, if any, to use. + bool may_proceed_anonymously = true; + for (SSL_CREDENTIAL *cred : creds) { + if (!cred->UsesPrivateKey()) { + // Non-certificate credentials (e.g. PSKs) do not participate in deciding + // whether to error or proceed anonymously. + continue; } - if (hs->credential == nullptr) { - // The error from the last attempt is in the error queue. - assert(ERR_peek_error() != 0); - ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE); - return ssl_hs_error; + + ERR_clear_error(); + may_proceed_anonymously = false; + uint16_t sigalg; + if (check_credential(hs, cred, &sigalg)) { + hs->credential = UpRef(cred); + hs->signature_algorithm = sigalg; + break; } } + // Fail the connection if no credentials matched, but only if the caller + // configured at least one certificate credential. If there were no + // candidates, proceed anonymously. + if (!may_proceed_anonymously && hs->credential == nullptr) { + // The error from the last attempt is in the error queue. + assert(ERR_peek_error() != 0); + ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE); + return ssl_hs_error; + } + if (!tls13_add_certificate(hs)) { return ssl_hs_error; }