Bug 1566131, check policy against hash algorithms used for ServerKeyExchange, r=mt
authorDaiki Ueno <dueno@redhat.com>
Fri, 08 Nov 2019 10:00:03 +0100
changeset 15383 c08947c6af5789510641ad17373e2612361e4ec6
parent 15382 e766899c72a517976f5f4abfec2a56712841e411
child 15384 c33b214b2ec8a8f85ac472aaf6dea15ac5cead2b
push id3576
push userdueno@redhat.com
push dateFri, 08 Nov 2019 09:00:52 +0000
reviewersmt
bugs1566131
Bug 1566131, check policy against hash algorithms used for ServerKeyExchange, 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
gtests/ssl_gtest/ssl_dhe_unittest.cc
gtests/ssl_gtest/ssl_ecdh_unittest.cc
gtests/ssl_gtest/tls_connect.h
lib/ssl/ssl3con.c
--- a/gtests/ssl_gtest/ssl_dhe_unittest.cc
+++ b/gtests/ssl_gtest/ssl_dhe_unittest.cc
@@ -677,9 +677,105 @@ class DHEServerKEXSigAlgReplacer : publi
 TEST_P(TlsConnectTls12, ConnectInconsistentSigAlgDHE) {
   EnableOnlyDheCiphers();
 
   MakeTlsFilter<DHEServerKEXSigAlgReplacer>(server_,
                                             ssl_sig_ecdsa_secp256r1_sha256);
   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
--- a/gtests/ssl_gtest/ssl_ecdh_unittest.cc
+++ b/gtests/ssl_gtest/ssl_ecdh_unittest.cc
@@ -661,16 +661,90 @@ TEST_P(TlsConnectTls12, ConnectIncorrect
   client_->EnableCiphersByKeyExchange(ssl_kea_ecdh);
 
   MakeTlsFilter<ECCServerKEXSigAlgReplacer>(server_,
                                             ssl_sig_ecdsa_secp256r1_sha256);
   ConnectExpectAlert(client_, kTlsAlertIllegalParameter);
   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));
 
 #ifndef NSS_DISABLE_TLS_1_3
 INSTANTIATE_TEST_CASE_P(KeyExchangeTest, TlsKeyExchangeTest13,
                         ::testing::Combine(TlsConnectTestBase::kTlsVariantsAll,
                                            TlsConnectTestBase::kTlsV13));
--- a/gtests/ssl_gtest/tls_connect.h
+++ b/gtests/ssl_gtest/tls_connect.h
@@ -159,17 +159,17 @@ class TlsConnectTestBase : public ::test
   // NSS will move this final entry to the front when used with ALPN.
   const uint8_t alpn_dummy_val_[4] = {0x01, 0x62, 0x01, 0x61};
 
   // A list of algorithm IDs whose policies need to be preserved
   // around test cases.  In particular, DSA is checked in
   // 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:
   void CheckResumption(SessionResumptionMode expected);
   void CheckExtendedMasterSecret();
   void CheckEarlyDataAccepted();
   static PRTime TimeFunc(void* arg);
 
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -1449,31 +1449,42 @@ ssl3_VerifySignedHashes(sslSocket *ss, S
  */
 SECStatus
 ssl3_ComputeCommonKeyHash(SSLHashType hashAlg,
                           PRUint8 *hashBuf, unsigned int bufLen,
                           SSL3Hashes *hashes)
 {
     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);
             return rv;
         }
         rv = PK11_HashBuf(SEC_OID_SHA1, hashes->u.s.sha, hashBuf, bufLen);
         if (rv != SECSuccess) {
             ssl_MapLowLevelError(SSL_ERROR_SHA_DIGEST_FAILURE);
             return rv;
         }
         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);
             return SECFailure;
         }
         rv = PK11_HashBuf(hashOID, hashes->u.raw, hashBuf, bufLen);
         if (rv != SECSuccess) {
             ssl_MapLowLevelError(SSL_ERROR_DIGEST_FAILURE);