Skip to content

Commit

Permalink
Bug 1566131, check policy against hash algorithms used for ServerKeyE…
Browse files Browse the repository at this point in the history 
…xchange, r=mt

Summary: This adds necessary policy checks in `ssl3_ComputeCommonKeyHash()`, right before calculating hashes.  Note that it currently doesn't check MD5 as it still needs to be allowed in TLS 1.1 or earlier and many tests fail if we change that.

Reviewers: mt

Reviewed By: mt

Bug #: 1566131

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

--HG--
extra : rebase_source : 3a41138caf32f7c71ebc68eaab74e471b48dc2d3
extra : amend_source : dd7038f5201ba3669017443dc081af0d2ace8c41
  • Loading branch information
@ueno
ueno committed Nov 8, 2019
1 parent e34a8da commit 8ad375f
Show file tree
Hide file tree
Showing 4 changed files with 182 additions and 1 deletion.
96 changes: 96 additions & 0 deletions gtests/ssl_gtest/ssl_dhe_unittest.cc
View file
Expand Up @@ -682,4 +682,100 @@ TEST_P(TlsConnectTls12, ConnectInconsistentSigAlgDHE) {
ConnectExpectAlert(client_, kTlsAlertIllegalParameter);
}

static void CheckSkeSigScheme(
std::shared_ptr<TlsHandshakeRecorder>& capture_ske,
uint16_t expected_scheme) {
TlsParser parser(capture_ske->buffer());
EXPECT_TRUE(parser.SkipVariable(2)) << " read dh_p";
EXPECT_TRUE(parser.SkipVariable(2)) << " read dh_q";
EXPECT_TRUE(parser.SkipVariable(2)) << " read dh_Ys";

uint32_t tmp;
EXPECT_TRUE(parser.Read(&tmp, 2)) << " read sig_scheme";
EXPECT_EQ(expected_scheme, static_cast<uint16_t>(tmp));
}

TEST_P(TlsConnectTls12, ConnectSigAlgEnabledByPolicyDhe) {
EnableOnlyDheCiphers();

const std::vector<SSLSignatureScheme> schemes = {ssl_sig_rsa_pkcs1_sha1,
ssl_sig_rsa_pkcs1_sha384};

EnsureTlsSetup();
client_->SetSignatureSchemes(schemes.data(), schemes.size());
server_->SetSignatureSchemes(schemes.data(), schemes.size());
auto capture_ske = MakeTlsFilter<TlsHandshakeRecorder>(
server_, kTlsHandshakeServerKeyExchange);

StartConnect();
client_->Handshake(); // Send ClientHello

// Enable SHA-1 by policy.
SECStatus rv = NSS_SetAlgorithmPolicy(SEC_OID_SHA1, NSS_USE_ALG_IN_SSL_KX, 0);
ASSERT_EQ(SECSuccess, rv);
rv = NSS_SetAlgorithmPolicy(SEC_OID_APPLY_SSL_POLICY, NSS_USE_POLICY_IN_SSL,
0);
ASSERT_EQ(SECSuccess, rv);

Handshake(); // Remainder of handshake
// The server should now report that it is connected
EXPECT_EQ(TlsAgent::STATE_CONNECTED, server_->state());

CheckSkeSigScheme(capture_ske, ssl_sig_rsa_pkcs1_sha1);
}

TEST_P(TlsConnectTls12, ConnectSigAlgDisabledByPolicyDhe) {
EnableOnlyDheCiphers();

const std::vector<SSLSignatureScheme> schemes = {ssl_sig_rsa_pkcs1_sha1,
ssl_sig_rsa_pkcs1_sha384};

EnsureTlsSetup();
client_->SetSignatureSchemes(schemes.data(), schemes.size());
server_->SetSignatureSchemes(schemes.data(), schemes.size());
auto capture_ske = MakeTlsFilter<TlsHandshakeRecorder>(
server_, kTlsHandshakeServerKeyExchange);

StartConnect();
client_->Handshake(); // Send ClientHello

// Disable SHA-1 by policy after sending ClientHello so that CH
// includes SHA-1 signature scheme.
SECStatus rv = NSS_SetAlgorithmPolicy(SEC_OID_SHA1, 0, NSS_USE_ALG_IN_SSL_KX);
ASSERT_EQ(SECSuccess, rv);
rv = NSS_SetAlgorithmPolicy(SEC_OID_APPLY_SSL_POLICY, NSS_USE_POLICY_IN_SSL,
0);
ASSERT_EQ(SECSuccess, rv);

Handshake(); // Remainder of handshake
// The server should now report that it is connected
EXPECT_EQ(TlsAgent::STATE_CONNECTED, server_->state());

CheckSkeSigScheme(capture_ske, ssl_sig_rsa_pkcs1_sha384);
}

TEST_P(TlsConnectPre12, ConnectSigAlgDisabledByPolicyDhePre12) {
EnableOnlyDheCiphers();

EnsureTlsSetup();
StartConnect();
client_->Handshake(); // Send ClientHello

// Disable SHA-1 by policy. This will cause the connection fail as
// TLS 1.1 or earlier uses combined SHA-1 + MD5 signature.
SECStatus rv = NSS_SetAlgorithmPolicy(SEC_OID_SHA1, 0, NSS_USE_ALG_IN_SSL_KX);
ASSERT_EQ(SECSuccess, rv);
rv = NSS_SetAlgorithmPolicy(SEC_OID_APPLY_SSL_POLICY, NSS_USE_POLICY_IN_SSL,
0);
ASSERT_EQ(SECSuccess, rv);

server_->ExpectSendAlert(kTlsAlertHandshakeFailure);
client_->ExpectReceiveAlert(kTlsAlertHandshakeFailure);

// Remainder of handshake
Handshake();

server_->CheckErrorCode(SSL_ERROR_UNSUPPORTED_HASH_ALGORITHM);
}

} // namespace nss_test
74 changes: 74 additions & 0 deletions gtests/ssl_gtest/ssl_ecdh_unittest.cc
View file
Expand Up @@ -666,6 +666,80 @@ TEST_P(TlsConnectTls12, ConnectIncorrectSigAlg) {
client_->CheckErrorCode(SSL_ERROR_INCORRECT_SIGNATURE_ALGORITHM);
}

static void CheckSkeSigScheme(
std::shared_ptr<TlsHandshakeRecorder> &capture_ske,
uint16_t expected_scheme) {
TlsParser parser(capture_ske->buffer());
uint32_t tmp = 0;
EXPECT_TRUE(parser.Read(&tmp, 1)) << " read curve_type";
EXPECT_EQ(3U, tmp) << "curve type has to be 3";
EXPECT_TRUE(parser.Skip(2)) << " read namedcurve";
EXPECT_TRUE(parser.SkipVariable(1)) << " read public";

EXPECT_TRUE(parser.Read(&tmp, 2)) << " read sig_scheme";
EXPECT_EQ(expected_scheme, static_cast<uint16_t>(tmp));
}

TEST_P(TlsConnectTls12, ConnectSigAlgEnabledByPolicy) {
EnsureTlsSetup();
client_->DisableAllCiphers();
client_->EnableCiphersByKeyExchange(ssl_kea_ecdh);

const std::vector<SSLSignatureScheme> schemes = {ssl_sig_rsa_pkcs1_sha1,
ssl_sig_rsa_pkcs1_sha384};

client_->SetSignatureSchemes(schemes.data(), schemes.size());
server_->SetSignatureSchemes(schemes.data(), schemes.size());
auto capture_ske = MakeTlsFilter<TlsHandshakeRecorder>(
server_, kTlsHandshakeServerKeyExchange);

StartConnect();
client_->Handshake(); // Send ClientHello

// Enable SHA-1 by policy.
SECStatus rv = NSS_SetAlgorithmPolicy(SEC_OID_SHA1, NSS_USE_ALG_IN_SSL_KX, 0);
ASSERT_EQ(SECSuccess, rv);
rv = NSS_SetAlgorithmPolicy(SEC_OID_APPLY_SSL_POLICY, NSS_USE_POLICY_IN_SSL,
0);
ASSERT_EQ(SECSuccess, rv);

Handshake(); // Remainder of handshake
// The server should now report that it is connected
EXPECT_EQ(TlsAgent::STATE_CONNECTED, server_->state());

CheckSkeSigScheme(capture_ske, ssl_sig_rsa_pkcs1_sha1);
}

TEST_P(TlsConnectTls12, ConnectSigAlgDisabledByPolicy) {
EnsureTlsSetup();
client_->DisableAllCiphers();
client_->EnableCiphersByKeyExchange(ssl_kea_ecdh);

const std::vector<SSLSignatureScheme> schemes = {ssl_sig_rsa_pkcs1_sha1,
ssl_sig_rsa_pkcs1_sha384};

client_->SetSignatureSchemes(schemes.data(), schemes.size());
server_->SetSignatureSchemes(schemes.data(), schemes.size());
auto capture_ske = MakeTlsFilter<TlsHandshakeRecorder>(
server_, kTlsHandshakeServerKeyExchange);

StartConnect();
client_->Handshake(); // Send ClientHello

// Disable SHA-1 by policy.
SECStatus rv = NSS_SetAlgorithmPolicy(SEC_OID_SHA1, 0, NSS_USE_ALG_IN_SSL_KX);
ASSERT_EQ(SECSuccess, rv);
rv = NSS_SetAlgorithmPolicy(SEC_OID_APPLY_SSL_POLICY, NSS_USE_POLICY_IN_SSL,
0);
ASSERT_EQ(SECSuccess, rv);

Handshake(); // Remainder of handshake
// The server should now report that it is connected
EXPECT_EQ(TlsAgent::STATE_CONNECTED, server_->state());

CheckSkeSigScheme(capture_ske, ssl_sig_rsa_pkcs1_sha384);
}

INSTANTIATE_TEST_CASE_P(KeyExchangeTest, TlsKeyExchangeTest,
::testing::Combine(TlsConnectTestBase::kTlsVariantsAll,
TlsConnectTestBase::kTlsV11Plus));
Expand Down
2 changes: 1 addition & 1 deletion gtests/ssl_gtest/tls_connect.h
View file
Expand Up @@ -164,7 +164,7 @@ class TlsConnectTestBase : public ::testing::Test {
// ssl_extension_unittest.cc.
const std::vector<SECOidTag> algorithms_ = {SEC_OID_APPLY_SSL_POLICY,
SEC_OID_ANSIX9_DSA_SIGNATURE,
SEC_OID_CURVE25519};
SEC_OID_CURVE25519, SEC_OID_SHA1};
std::vector<std::tuple<SECOidTag, uint32_t>> saved_policies_;

private:
Expand Down
11 changes: 11 additions & 0 deletions lib/ssl/ssl3con.c
View file
Expand Up @@ -1454,8 +1454,14 @@ ssl3_ComputeCommonKeyHash(SSLHashType hashAlg,
{
SECStatus rv;
SECOidTag hashOID;
PRUint32 policy;

if (hashAlg == ssl_hash_none) {
if ((NSS_GetAlgorithmPolicy(SEC_OID_SHA1, &policy) == SECSuccess) &&
!(policy & NSS_USE_ALG_IN_SSL_KX)) {
ssl_MapLowLevelError(SSL_ERROR_UNSUPPORTED_HASH_ALGORITHM);
return SECFailure;
}
rv = PK11_HashBuf(SEC_OID_MD5, hashes->u.s.md5, hashBuf, bufLen);
if (rv != SECSuccess) {
ssl_MapLowLevelError(SSL_ERROR_MD5_DIGEST_FAILURE);
Expand All @@ -1469,6 +1475,11 @@ ssl3_ComputeCommonKeyHash(SSLHashType hashAlg,
hashes->len = MD5_LENGTH + SHA1_LENGTH;
} else {
hashOID = ssl3_HashTypeToOID(hashAlg);
if ((NSS_GetAlgorithmPolicy(hashOID, &policy) == SECSuccess) &&
!(policy & NSS_USE_ALG_IN_SSL_KX)) {
ssl_MapLowLevelError(SSL_ERROR_UNSUPPORTED_HASH_ALGORITHM);
return SECFailure;
}
hashes->len = HASH_ResultLenByOidTag(hashOID);
if (hashes->len == 0 || hashes->len > sizeof(hashes->u.raw)) {
ssl_MapLowLevelError(SSL_ERROR_UNSUPPORTED_HASH_ALGORITHM);
Expand Down

0 comments on commit 8ad375f

Please sign in to comment.