Skip to content

Commit

Permalink
Bug 1412829, reject empty supported_signature_algorithms in CR in TLS…
Browse files Browse the repository at this point in the history
… 1.2, r=mt

Summary: This basically reverts bug 1335069 to align with RFC 5246.

Reviewers: mt

Reviewed By: mt

Bug #: 1412829

Differential Revision: https://phabricator.services.mozilla.com/D12563

--HG--
extra : amend_source : a87f98603e14841654948c7664dbde26ebaf04e4
  • Loading branch information
ueno committed Nov 22, 2018
1 parent 8e292bc commit 96728bb
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 19 deletions.
3 changes: 2 additions & 1 deletion gtests/nss_bogo_shim/config.json
Expand Up @@ -64,7 +64,8 @@
"RequireAnyClientCertificate-TLS1*":"Bug 1339387",
"SendExtensionOnClientCertificate-TLS13":"Bug 1339392",
"ALPNClient-Mismatch-TLS13":"NSS sends alerts in response to errors in protected handshake messages in the clear",
"P224-Server":"NSS doesn't support P-224"
"P224-Server":"NSS doesn't support P-224",
"ClientAuth-SHA1-Fallback*":"Boring wants us to fall back to SHA-1 if supported_signature_algorithms in CR is empty."
},
"ErrorMap" : {
":HANDSHAKE_FAILURE_ON_CLIENT_HELLO:":"SSL_ERROR_NO_CYPHER_OVERLAP",
Expand Down
15 changes: 5 additions & 10 deletions gtests/ssl_gtest/ssl_auth_unittest.cc
Expand Up @@ -386,25 +386,20 @@ class TlsZeroCertificateRequestSigAlgsFilter : public TlsHandshakeFilter {
}
};

// Check that we fall back to SHA-1 when the server doesn't provide any
// Check that we send an alert when the server doesn't provide any
// supported_signature_algorithms in the CertificateRequest message.
TEST_P(TlsConnectTls12, ClientAuthNoSigAlgsFallback) {
TEST_P(TlsConnectTls12, ClientAuthNoSigAlgs) {
EnsureTlsSetup();
MakeTlsFilter<TlsZeroCertificateRequestSigAlgsFilter>(server_);
auto capture_cert_verify = MakeTlsFilter<TlsHandshakeRecorder>(
client_, kTlsHandshakeCertificateVerify);
client_->SetupClientAuth();
server_->RequestClientAuth(true);

ConnectExpectAlert(server_, kTlsAlertDecryptError);

// We're expecting a bad signature here because we tampered with a handshake
// message (CertReq). Previously, without the SHA-1 fallback, we would've
// seen a malformed record alert.
server_->CheckErrorCode(SEC_ERROR_BAD_SIGNATURE);
client_->CheckErrorCode(SSL_ERROR_DECRYPT_ERROR_ALERT);
ConnectExpectAlert(client_, kTlsAlertHandshakeFailure);

CheckSigScheme(capture_cert_verify, 0, server_, ssl_sig_rsa_pkcs1_sha1, 1024);
server_->CheckErrorCode(SSL_ERROR_HANDSHAKE_FAILURE_ALERT);
client_->CheckErrorCode(SSL_ERROR_UNSUPPORTED_SIGNATURE_ALGORITHM);
}

static const SSLSignatureScheme kSignatureSchemeEcdsaSha384[] = {
Expand Down
17 changes: 9 additions & 8 deletions lib/ssl/ssl3con.c
Expand Up @@ -6171,15 +6171,11 @@ ssl_PickClientSignatureScheme(sslSocket *ss, const SSLSignatureScheme *schemes,

PORT_Assert(pubKey);

if (!isTLS13 && numSchemes == 0) {
/* If the server didn't provide any signature algorithms
* then let's assume they support SHA-1. */
rv = ssl_PickFallbackSignatureScheme(ss, pubKey);
SECKEY_DestroyPublicKey(pubKey);
return rv;
}

if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_2) {
/* We should have already checked that a signature scheme was
* listed in the request. */
PORT_Assert(schemes && numSchemes > 0);
}

if (!isTLS13 &&
(SECKEY_GetPublicKeyType(pubKey) == rsaKey ||
Expand Down Expand Up @@ -7331,6 +7327,11 @@ ssl3_HandleCertificateRequest(sslSocket *ss, PRUint8 *b, PRUint32 length)
PORT_SetError(SSL_ERROR_RX_MALFORMED_CERT_REQUEST);
goto loser; /* malformed, alert has been sent */
}
if (signatureSchemeCount == 0) {
errCode = SSL_ERROR_UNSUPPORTED_SIGNATURE_ALGORITHM;
desc = handshake_failure;
goto alert_loser;
}
}

rv = ssl3_ParseCertificateRequestCAs(ss, &b, &length, &ca_list);
Expand Down

0 comments on commit 96728bb

Please sign in to comment.