Skip to content

Commit

Permalink
Bug 1549225 - Up front Signature Scheme validation, r=ueno
Browse files Browse the repository at this point in the history 
Summary:
This patch started as an attempt to ensure that a DSA signature scheme would not
be advertised if we weren't willing to negotiate versions less than TLS 1.3.
Then I realized that we didn't do the same for PKCS#1 RSA.

Then I realized that we were still willing to try to establish connections when
we had a certificate that we couldn't use.

Then I realized that ssl3_config_match_init() wasn't being run consistently.  On
resumption, we only ran it when we were PARANOID.  That's silly because we
weren't checking policies.

Then I realized that we were allowing ECDSA certificates to be used when the
named group in the certificate was disabled.  We weren't enforcing that
consistently either.  However, I also discovered that the check we have wouldn't
work without a tweak because in TLS 1.3 the named group is part of the signature
scheme; the configured named groups are only used prior to TLS 1.3 when
selecting ECDSA/ECDH certificates.

So that sounds like a lot of changes but what it boils down to is more robust
checking of the configuration prior to starting a connection.  As a result, we
should be offering fewer options that we're unwilling or unable to follow
through on.  A good number of tests needed tweaking as a result because we were
relying on getting past the checks in those tests.  No real problems were found
as a result; this just moves failures that might arise from misconfiguration a
little earlier in the process.

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

--HG--
extra : rebase_source : 44632658baf414f035f13493d3f2f0ff5753ae9e
  • Loading branch information
@martinthomson
martinthomson committed Sep 6, 2019
1 parent d56f918 commit 0d386a8
Show file tree
Hide file tree
Showing 9 changed files with 303 additions and 78 deletions.
120 changes: 119 additions & 1 deletion gtests/ssl_gtest/ssl_auth_unittest.cc
View file
Expand Up @@ -783,14 +783,29 @@ TEST_P(TlsConnectTls13, ClientAuthPkcs1SignatureScheme) {
1024);
}

// Client should refuse to connect without a usable signature scheme.
TEST_P(TlsConnectTls13, ClientAuthPkcs1SignatureSchemeOnly) {
static const SSLSignatureScheme kSignatureScheme[] = {
ssl_sig_rsa_pkcs1_sha256};

Reset(TlsAgent::kServerRsa, "rsa");
client_->SetSignatureSchemes(kSignatureScheme,
PR_ARRAY_SIZE(kSignatureScheme));
server_->SetSignatureSchemes(kSignatureScheme,
client_->SetupClientAuth();
client_->StartConnect();
client_->Handshake();
EXPECT_EQ(TlsAgent::STATE_ERROR, client_->state());
client_->CheckErrorCode(SSL_ERROR_NO_SUPPORTED_SIGNATURE_ALGORITHM);
}

// Though the client has a usable signature scheme, when a certificate is
// requested, it can't produce one.
TEST_P(TlsConnectTls13, ClientAuthPkcs1AndEcdsaScheme) {
static const SSLSignatureScheme kSignatureScheme[] = {
ssl_sig_rsa_pkcs1_sha256, ssl_sig_ecdsa_secp256r1_sha256};

Reset(TlsAgent::kServerRsa, "rsa");
client_->SetSignatureSchemes(kSignatureScheme,
PR_ARRAY_SIZE(kSignatureScheme));
client_->SetupClientAuth();
server_->RequestClientAuth(true);
Expand Down Expand Up @@ -1433,6 +1448,109 @@ TEST_F(TlsAgentStreamTestServer, ConfigureCertRsaPss) {
&ServerCertDataRsaPss));
}

// A server should refuse to even start a handshake with
// misconfigured certificate and signature scheme.
TEST_P(TlsConnectTls12Plus, MisconfiguredCertScheme) {
Reset(TlsAgent::kServerDsa);
static const SSLSignatureScheme kScheme[] = {ssl_sig_ecdsa_secp256r1_sha256};
server_->SetSignatureSchemes(kScheme, PR_ARRAY_SIZE(kScheme));
ConnectExpectAlert(server_, kTlsAlertHandshakeFailure);
if (version_ < SSL_LIBRARY_VERSION_TLS_1_3) {
// TLS 1.2 disables cipher suites, which leads to a different error.
server_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);
} else {
server_->CheckErrorCode(SSL_ERROR_NO_SUPPORTED_SIGNATURE_ALGORITHM);
}
client_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);
}

// In TLS 1.2, disabling an EC group causes ECDSA to be invalid.
TEST_P(TlsConnectTls12, Tls12CertDisabledGroup) {
Reset(TlsAgent::kServerEcdsa256);
static const std::vector<SSLNamedGroup> k25519 = {ssl_grp_ec_curve25519};
server_->ConfigNamedGroups(k25519);
ConnectExpectAlert(server_, kTlsAlertHandshakeFailure);
server_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);
client_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);
}

// In TLS 1.3, ECDSA configuration only depends on the signature scheme.
TEST_P(TlsConnectTls13, Tls13CertDisabledGroup) {
Reset(TlsAgent::kServerEcdsa256);
static const std::vector<SSLNamedGroup> k25519 = {ssl_grp_ec_curve25519};
server_->ConfigNamedGroups(k25519);
Connect();
}

// A client should refuse to even start a handshake with only DSA.
TEST_P(TlsConnectTls13, Tls13DsaOnlyClient) {
static const SSLSignatureScheme kDsa[] = {ssl_sig_dsa_sha256};
client_->SetSignatureSchemes(kDsa, PR_ARRAY_SIZE(kDsa));
client_->StartConnect();
client_->Handshake();
EXPECT_EQ(TlsAgent::STATE_ERROR, client_->state());
client_->CheckErrorCode(SSL_ERROR_NO_SUPPORTED_SIGNATURE_ALGORITHM);
}

TEST_P(TlsConnectTls13, Tls13DsaOnlyServer) {
Reset(TlsAgent::kServerDsa);
static const SSLSignatureScheme kDsa[] = {ssl_sig_dsa_sha256};
server_->SetSignatureSchemes(kDsa, PR_ARRAY_SIZE(kDsa));
ConnectExpectAlert(server_, kTlsAlertHandshakeFailure);
server_->CheckErrorCode(SSL_ERROR_NO_SUPPORTED_SIGNATURE_ALGORITHM);
client_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);
}

TEST_P(TlsConnectTls13, Tls13Pkcs1OnlyClient) {
static const SSLSignatureScheme kPkcs1[] = {ssl_sig_rsa_pkcs1_sha256};
client_->SetSignatureSchemes(kPkcs1, PR_ARRAY_SIZE(kPkcs1));
client_->StartConnect();
client_->Handshake();
EXPECT_EQ(TlsAgent::STATE_ERROR, client_->state());
client_->CheckErrorCode(SSL_ERROR_NO_SUPPORTED_SIGNATURE_ALGORITHM);
}

TEST_P(TlsConnectTls13, Tls13Pkcs1OnlyServer) {
static const SSLSignatureScheme kPkcs1[] = {ssl_sig_rsa_pkcs1_sha256};
server_->SetSignatureSchemes(kPkcs1, PR_ARRAY_SIZE(kPkcs1));
ConnectExpectAlert(server_, kTlsAlertHandshakeFailure);
server_->CheckErrorCode(SSL_ERROR_NO_SUPPORTED_SIGNATURE_ALGORITHM);
client_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);
}

TEST_P(TlsConnectTls13, Tls13DsaIsNotAdvertisedClient) {
EnsureTlsSetup();
static const SSLSignatureScheme kSchemes[] = {ssl_sig_dsa_sha256,
ssl_sig_rsa_pss_rsae_sha256};
client_->SetSignatureSchemes(kSchemes, PR_ARRAY_SIZE(kSchemes));
auto capture =
MakeTlsFilter<TlsExtensionCapture>(client_, ssl_signature_algorithms_xtn);
Connect();
// We should only have the one signature algorithm advertised.
static const uint8_t kExpectedExt[] = {0, 2, ssl_sig_rsa_pss_rsae_sha256 >> 8,
ssl_sig_rsa_pss_rsae_sha256 & 0xff};
ASSERT_EQ(DataBuffer(kExpectedExt, sizeof(kExpectedExt)),
capture->extension());
}

TEST_P(TlsConnectTls13, Tls13DsaIsNotAdvertisedServer) {
EnsureTlsSetup();
static const SSLSignatureScheme kSchemes[] = {ssl_sig_dsa_sha256,
ssl_sig_rsa_pss_rsae_sha256};
server_->SetSignatureSchemes(kSchemes, PR_ARRAY_SIZE(kSchemes));
auto capture = MakeTlsFilter<TlsExtensionCapture>(
server_, ssl_signature_algorithms_xtn, true);
capture->SetHandshakeTypes({kTlsHandshakeCertificateRequest});
capture->EnableDecryption();
server_->RequestClientAuth(false); // So we get a CertificateRequest.
Connect();
// We should only have the one signature algorithm advertised.
static const uint8_t kExpectedExt[] = {0, 2, ssl_sig_rsa_pss_rsae_sha256 >> 8,
ssl_sig_rsa_pss_rsae_sha256 & 0xff};
ASSERT_EQ(DataBuffer(kExpectedExt, sizeof(kExpectedExt)),
capture->extension());
}

// variant, version, certificate, auth type, signature scheme
typedef std::tuple<SSLProtocolVariant, uint16_t, std::string, SSLAuthType,
SSLSignatureScheme>
Expand Down
19 changes: 19 additions & 0 deletions gtests/ssl_gtest/ssl_ciphersuite_unittest.cc
View file
Expand Up @@ -56,6 +56,9 @@ class TlsCipherSuiteTestBase : public TlsConnectTestBase {

if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
std::vector<SSLNamedGroup> groups = {group_};
if (cert_group_ != ssl_grp_none) {
groups.push_back(cert_group_);
}
client_->ConfigNamedGroups(groups);
server_->ConfigNamedGroups(groups);
kea_type_ = SSLInt_GetKEAType(group_);
Expand All @@ -69,34 +72,47 @@ class TlsCipherSuiteTestBase : public TlsConnectTestBase {
if (version_ >= SSL_LIBRARY_VERSION_TLS_1_3) {
switch (sig_scheme_) {
case ssl_sig_rsa_pss_rsae_sha256:
std::cerr << "Signature scheme: rsa_pss_rsae_sha256" << std::endl;
Reset(TlsAgent::kServerRsaSign);
auth_type_ = ssl_auth_rsa_sign;
break;
case ssl_sig_rsa_pss_rsae_sha384:
std::cerr << "Signature scheme: rsa_pss_rsae_sha384" << std::endl;
Reset(TlsAgent::kServerRsaSign);
auth_type_ = ssl_auth_rsa_sign;
break;
case ssl_sig_rsa_pss_rsae_sha512:
// You can't fit SHA-512 PSS in a 1024-bit key.
std::cerr << "Signature scheme: rsa_pss_rsae_sha512" << std::endl;
Reset(TlsAgent::kRsa2048);
auth_type_ = ssl_auth_rsa_sign;
break;
case ssl_sig_rsa_pss_pss_sha256:
std::cerr << "Signature scheme: rsa_pss_pss_sha256" << std::endl;
Reset(TlsAgent::kServerRsaPss);
auth_type_ = ssl_auth_rsa_pss;
break;
case ssl_sig_rsa_pss_pss_sha384:
std::cerr << "Signature scheme: rsa_pss_pss_sha384" << std::endl;
Reset("rsa_pss384");
auth_type_ = ssl_auth_rsa_pss;
break;
case ssl_sig_rsa_pss_pss_sha512:
std::cerr << "Signature scheme: rsa_pss_pss_sha512" << std::endl;
Reset("rsa_pss512");
auth_type_ = ssl_auth_rsa_pss;
break;
case ssl_sig_ecdsa_secp256r1_sha256:
std::cerr << "Signature scheme: ecdsa_secp256r1_sha256" << std::endl;
Reset(TlsAgent::kServerEcdsa256);
auth_type_ = ssl_auth_ecdsa;
cert_group_ = ssl_grp_ec_secp256r1;
break;
case ssl_sig_ecdsa_secp384r1_sha384:
std::cerr << "Signature scheme: ecdsa_secp384r1_sha384" << std::endl;
Reset(TlsAgent::kServerEcdsa384);
auth_type_ = ssl_auth_ecdsa;
cert_group_ = ssl_grp_ec_secp384r1;
break;
default:
ADD_FAILURE() << "Unsupported signature scheme: " << sig_scheme_;
Expand All @@ -112,9 +128,11 @@ class TlsCipherSuiteTestBase : public TlsConnectTestBase {
break;
case ssl_auth_ecdsa:
Reset(TlsAgent::kServerEcdsa256);
cert_group_ = ssl_grp_ec_secp256r1;
break;
case ssl_auth_ecdh_ecdsa:
Reset(TlsAgent::kServerEcdhEcdsa);
cert_group_ = ssl_grp_ec_secp256r1;
break;
case ssl_auth_ecdh_rsa:
Reset(TlsAgent::kServerEcdhRsa);
Expand Down Expand Up @@ -192,6 +210,7 @@ class TlsCipherSuiteTestBase : public TlsConnectTestBase {
SSLAuthType auth_type_;
SSLKEAType kea_type_;
SSLNamedGroup group_;
SSLNamedGroup cert_group_ = ssl_grp_none;
SSLSignatureScheme sig_scheme_;
SSLCipherSuiteInfo csinfo_;
};
Expand Down
2 changes: 1 addition & 1 deletion gtests/ssl_gtest/ssl_extension_unittest.cc
View file
Expand Up @@ -652,7 +652,7 @@ TEST_P(TlsExtensionTest12, SignatureAlgorithmDisableDSA) {
MakeTlsFilter<TlsExtensionCapture>(client_, ssl_signature_algorithms_xtn);
client_->SetSignatureSchemes(schemes.data(), schemes.size());
ConnectExpectAlert(server_, kTlsAlertHandshakeFailure);
server_->CheckErrorCode(SSL_ERROR_UNSUPPORTED_SIGNATURE_ALGORITHM);
server_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);
client_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);

// Check if no DSA algorithms are advertised.
Expand Down
2 changes: 1 addition & 1 deletion gtests/ssl_gtest/ssl_fuzz_unittest.cc
View file
Expand Up @@ -252,4 +252,4 @@ INSTANTIATE_TEST_CASE_P(
FuzzDatagram, TlsFuzzTest,
::testing::Combine(TlsConnectTestBase::kTlsVariantsDatagram,
TlsConnectTestBase::kTlsV11Plus));
}
} // namespace nss_test
2 changes: 1 addition & 1 deletion gtests/ssl_gtest/tls_esni_unittest.cc
View file
Expand Up @@ -472,4 +472,4 @@ TEST_P(TlsConnectTls13, EsniButTLS12Server) {
ASSERT_FALSE(SSLInt_ExtensionNegotiated(server_->ssl_fd(),
ssl_tls13_encrypted_sni_xtn));
}
}
} // namespace nss_test

0 comments on commit 0d386a8

Please sign in to comment.