# HG changeset patch

# User Daiki Ueno <dueno@redhat.com>

# Date 1573203603 -3600

#      Fri Nov 08 10:00:03 2019 +0100

# Node ID c08947c6af5789510641ad17373e2612361e4ec6

# Parent  e766899c72a517976f5f4abfec2a56712841e411

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

diff --git a/gtests/ssl_gtest/ssl_dhe_unittest.cc b/gtests/ssl_gtest/ssl_dhe_unittest.cc

--- a/gtests/ssl_gtest/ssl_dhe_unittest.cc

+++ b/gtests/ssl_gtest/ssl_dhe_unittest.cc

@@ -682,4 +682,100 @@ TEST_P(TlsConnectTls12, ConnectInconsist

   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

diff --git a/gtests/ssl_gtest/ssl_ecdh_unittest.cc b/gtests/ssl_gtest/ssl_ecdh_unittest.cc

--- a/gtests/ssl_gtest/ssl_ecdh_unittest.cc

+++ b/gtests/ssl_gtest/ssl_ecdh_unittest.cc

@@ -666,6 +666,80 @@ TEST_P(TlsConnectTls12, ConnectIncorrect

   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));

diff --git a/gtests/ssl_gtest/tls_connect.h b/gtests/ssl_gtest/tls_connect.h

--- a/gtests/ssl_gtest/tls_connect.h

+++ b/gtests/ssl_gtest/tls_connect.h

@@ -164,7 +164,7 @@ class TlsConnectTestBase : public ::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:

diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c

--- a/lib/ssl/ssl3con.c

+++ b/lib/ssl/ssl3con.c

@@ -1454,8 +1454,14 @@ ssl3_ComputeCommonKeyHash(SSLHashType ha

 {

     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);

@@ -1469,6 +1475,11 @@ ssl3_ComputeCommonKeyHash(SSLHashType ha

         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);