Bug 1493936, add a new "DSA" policy keyword, r=kaie

Summary:
This adds a new policy keyword "DSA" to explicitly disable DSA in TLS 1.2 or earlier.

We could make this a bit more generic, e.g., by adding "ECDSA", "RSA-PSS" etc.   However, considering the current use of policy in [fedora-crypto-policies](https://gitlab.com/redhat-crypto/fedora-crypto-policies), I realized that adding new keywords may cause compatibility problems; because the Fedora configuration has `disallow=ALL`, all new keywords would be disabled by default.   I think it's okay for DSA, though.

Reviewers: kaie

Reviewed By: kaie

Bug #: 1493936

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

--HG--
extra : amend_source : 4b0c81307bd50c953eea7df6dc611bdf63ae5710

gtests/ssl_gtest/ssl_extension_unittest.cc

@@ -547,6 +547,56 @@ TEST_P(TlsExtensionTest12, SignatureAlgorithmConfiguration) {
   }
 }
 
+// This only works on TLS 1.2, since it relies on DSA.
+TEST_P(TlsExtensionTest12, SignatureAlgorithmDisableDSA) {
+  const std::vector<SSLSignatureScheme> schemes = {
+      ssl_sig_dsa_sha1, ssl_sig_dsa_sha256, ssl_sig_dsa_sha384,
+      ssl_sig_dsa_sha512, ssl_sig_rsa_pss_rsae_sha256};
+
+  // Connect with DSA enabled by policy.
+  SECStatus rv = NSS_SetAlgorithmPolicy(SEC_OID_ANSIX9_DSA_SIGNATURE,
+                                        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);
+
+  Reset(TlsAgent::kServerDsa);
+  auto capture1 =
+      MakeTlsFilter<TlsExtensionCapture>(client_, ssl_signature_algorithms_xtn);
+  client_->SetSignatureSchemes(schemes.data(), schemes.size());
+  Connect();
+
+  // Check if all the signature algorithms are advertised.
+  EXPECT_TRUE(capture1->captured());
+  const DataBuffer& ext1 = capture1->extension();
+  EXPECT_EQ(2U + 2U * schemes.size(), ext1.len());
+
+  // Connect with DSA disabled by policy.
+  rv = NSS_SetAlgorithmPolicy(SEC_OID_ANSIX9_DSA_SIGNATURE, 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);
+
+  Reset(TlsAgent::kServerDsa);
+  auto capture2 =
+      MakeTlsFilter<TlsExtensionCapture>(client_, ssl_signature_algorithms_xtn);
+  client_->SetSignatureSchemes(schemes.data(), schemes.size());
+  ConnectExpectAlert(server_, kTlsAlertHandshakeFailure);
+  server_->CheckErrorCode(SSL_ERROR_UNSUPPORTED_SIGNATURE_ALGORITHM);
+  client_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);
+
+  // Check if no DSA algorithms are advertised.
+  EXPECT_TRUE(capture2->captured());
+  const DataBuffer& ext2 = capture2->extension();
+  EXPECT_EQ(2U + 2U, ext2.len());
+  uint32_t v = 0;
+  EXPECT_TRUE(ext2.Read(2, 2, &v));
+  EXPECT_EQ(ssl_sig_rsa_pss_rsae_sha256, v);
+}
+
 // Temporary test to verify that we choke on an empty ClientKeyShare.
 // This test will fail when we implement HelloRetryRequest.
 TEST_P(TlsExtensionTest13, EmptyClientKeyShare) {
@@ -1121,6 +1171,10 @@ INSTANTIATE_TEST_CASE_P(
 INSTANTIATE_TEST_CASE_P(ExtensionDatagramOnly, TlsExtensionTestDtls,
                         TlsConnectTestBase::kTlsV11Plus);
 
+INSTANTIATE_TEST_CASE_P(ExtensionTls12, TlsExtensionTest12,
+                        ::testing::Combine(TlsConnectTestBase::kTlsVariantsAll,
+                                           TlsConnectTestBase::kTlsV12));
+
 INSTANTIATE_TEST_CASE_P(ExtensionTls12Plus, TlsExtensionTest12Plus,
                         ::testing::Combine(TlsConnectTestBase::kTlsVariantsAll,
                                            TlsConnectTestBase::kTlsV12Plus));

gtests/ssl_gtest/ssl_versionpolicy_unittest.cc

@@ -214,12 +214,6 @@ class TestPolicyVersionRange
       ASSERT_EQ(SECSuccess, rv);
       rv = NSS_OptionSet(NSS_DTLS_VERSION_MAX_POLICY, saved_max_dtls_);
       ASSERT_EQ(SECSuccess, rv);
-      // If it wasn't set initially, clear the bit that we set.
-      if (!(saved_algorithm_policy_ & NSS_USE_POLICY_IN_SSL)) {
-        rv = NSS_SetAlgorithmPolicy(SEC_OID_APPLY_SSL_POLICY, 0,
-                                    NSS_USE_POLICY_IN_SSL);
-        ASSERT_EQ(SECSuccess, rv);
-      }
     }
 
    private:
@@ -233,16 +227,12 @@ class TestPolicyVersionRange
       ASSERT_EQ(SECSuccess, rv);
       rv = NSS_OptionGet(NSS_DTLS_VERSION_MAX_POLICY, &saved_max_dtls_);
       ASSERT_EQ(SECSuccess, rv);
-      rv = NSS_GetAlgorithmPolicy(SEC_OID_APPLY_SSL_POLICY,
-                                  &saved_algorithm_policy_);
-      ASSERT_EQ(SECSuccess, rv);
     }
 
     int32_t saved_min_tls_;
     int32_t saved_max_tls_;
     int32_t saved_min_dtls_;
     int32_t saved_max_dtls_;
-    uint32_t saved_algorithm_policy_;
   };
 
   VersionPolicy saved_version_policy_;

gtests/ssl_gtest/tls_connect.cc

@@ -183,12 +183,33 @@ void TlsConnectTestBase::ClearServerCache() {
   SSL_ConfigServerSessionIDCache(1024, 0, 0, g_working_dir_path.c_str());
 }
 
+void TlsConnectTestBase::SaveAlgorithmPolicy() {
+  saved_policies_.clear();
+  for (auto it = algorithms_.begin(); it != algorithms_.end(); ++it) {
+    uint32_t policy;
+    SECStatus rv = NSS_GetAlgorithmPolicy(*it, &policy);
+    ASSERT_EQ(SECSuccess, rv);
+    saved_policies_.push_back(std::make_tuple(*it, policy));
+  }
+}
+
+void TlsConnectTestBase::RestoreAlgorithmPolicy() {
+  for (auto it = saved_policies_.begin(); it != saved_policies_.end(); ++it) {
+    auto algorithm = std::get<0>(*it);
+    auto policy = std::get<1>(*it);
+    SECStatus rv = NSS_SetAlgorithmPolicy(
+        algorithm, policy, NSS_USE_POLICY_IN_SSL | NSS_USE_ALG_IN_SSL_KX);
+    ASSERT_EQ(SECSuccess, rv);
+  }
+}
+
 void TlsConnectTestBase::SetUp() {
   SSL_ConfigServerSessionIDCache(1024, 0, 0, g_working_dir_path.c_str());
   SSLInt_ClearSelfEncryptKey();
   SSLInt_SetTicketLifetime(30);
   SSL_SetupAntiReplay(1 * PR_USEC_PER_SEC, 1, 3);
   ClearStats();
+  SaveAlgorithmPolicy();
   Init();
 }
 
@@ -199,6 +220,7 @@ void TlsConnectTestBase::TearDown() {
   SSL_ClearSessionCache();
   SSLInt_ClearSelfEncryptKey();
   SSL_ShutdownServerSessionIDCache();
+  RestoreAlgorithmPolicy();
 }
 
 void TlsConnectTestBase::Init() {

gtests/ssl_gtest/tls_connect.h

@@ -132,6 +132,9 @@ class TlsConnectTestBase : public ::testing::Test {
   // Move the DTLS timers for both endpoints to pop the next timer.
   void ShiftDtlsTimers();
 
+  void SaveAlgorithmPolicy();
+  void RestoreAlgorithmPolicy();
+
  protected:
   SSLProtocolVariant variant_;
   std::shared_ptr<TlsAgent> client_;
@@ -149,6 +152,13 @@ class TlsConnectTestBase : public ::testing::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};
+  std::vector<std::tuple<SECOidTag, uint32_t>> saved_policies_;
+
  private:
   void CheckResumption(SessionResumptionMode expected);
   void CheckExtendedMasterSecret();

lib/certhigh/certvfy.c

@@ -37,7 +37,7 @@ CERT_CertTimesValid(CERTCertificate *c)
     return (valid == secCertTimeValid) ? SECSuccess : SECFailure;
 }
 
-SECStatus
+static SECStatus
 checkKeyParams(const SECAlgorithmID *sigAlgorithm, const SECKEYPublicKey *key)
 {
     SECStatus rv;
@@ -47,6 +47,12 @@ checkKeyParams(const SECAlgorithmID *sigAlgorithm, const SECKEYPublicKey *key)
     PRInt32 minLen, len;
 
     sigAlg = SECOID_GetAlgorithmTag(sigAlgorithm);
+    rv = NSS_GetAlgorithmPolicy(sigAlg, &policyFlags);
+    if (rv == SECSuccess &&
+        !(policyFlags & NSS_USE_ALG_IN_CERT_SIGNATURE)) {
+        PORT_SetError(SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED);
+        return SECFailure;
+    }
 
     switch (sigAlg) {
         case SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE:

lib/pk11wrap/pk11pars.c

@@ -384,18 +384,26 @@ static const oidValDef kxOptList[] = {
     { CIPHER_NAME("ECDH-RSA"), SEC_OID_TLS_ECDH_RSA, NSS_USE_ALG_IN_SSL_KX },
 };
 
+static const oidValDef signOptList[] = {
+    /* Signatures */
+    { CIPHER_NAME("DSA"), SEC_OID_ANSIX9_DSA_SIGNATURE,
+      NSS_USE_ALG_IN_SSL_KX | NSS_USE_ALG_IN_CERT_SIGNATURE },
+};
+
 typedef struct {
     const oidValDef *list;
     PRUint32 entries;
     const char *description;
+    PRBool allowEmpty;
 } algListsDef;
 
 static const algListsDef algOptLists[] = {
-    { curveOptList, PR_ARRAY_SIZE(curveOptList), "ECC" },
-    { hashOptList, PR_ARRAY_SIZE(hashOptList), "HASH" },
-    { macOptList, PR_ARRAY_SIZE(macOptList), "MAC" },
-    { cipherOptList, PR_ARRAY_SIZE(cipherOptList), "CIPHER" },
-    { kxOptList, PR_ARRAY_SIZE(kxOptList), "OTHER-KX" },
+    { curveOptList, PR_ARRAY_SIZE(curveOptList), "ECC", PR_FALSE },
+    { hashOptList, PR_ARRAY_SIZE(hashOptList), "HASH", PR_FALSE },
+    { macOptList, PR_ARRAY_SIZE(macOptList), "MAC", PR_FALSE },
+    { cipherOptList, PR_ARRAY_SIZE(cipherOptList), "CIPHER", PR_FALSE },
+    { kxOptList, PR_ARRAY_SIZE(kxOptList), "OTHER-KX", PR_FALSE },
+    { signOptList, PR_ARRAY_SIZE(signOptList), "OTHER-SIGN", PR_TRUE },
 };
 
 static const optionFreeDef sslOptList[] = {
@@ -718,7 +726,7 @@ secmod_sanityCheckCryptoPolicy(void)
     for (i = 0; i < PR_ARRAY_SIZE(algOptLists); i++) {
         const algListsDef *algOptList = &algOptLists[i];
         fprintf(stderr, "NSS-POLICY-%s: NUMBER-OF-%s: %u\n", enabledCount[i] ? sInfo : sWarn, algOptList->description, enabledCount[i]);
-        if (!enabledCount[i]) {
+        if (!enabledCount[i] && !algOptList->allowEmpty) {
             haveWarning = PR_TRUE;
         }
     }

lib/ssl/ssl3con.c

@@ -64,6 +64,7 @@ static SECStatus ssl3_FlushHandshakeMessages(sslSocket *ss, PRInt32 flags);
 static CK_MECHANISM_TYPE ssl3_GetHashMechanismByHashType(SSLHashType hashType);
 static CK_MECHANISM_TYPE ssl3_GetMgfMechanismByHashType(SSLHashType hash);
 PRBool ssl_IsRsaPssSignatureScheme(SSLSignatureScheme scheme);
+PRBool ssl_IsDsaSignatureScheme(SSLSignatureScheme scheme);
 
 const PRUint8 ssl_hello_retry_random[] = {
     0xCF, 0x21, 0xAD, 0x74, 0xE5, 0x9A, 0x61, 0x11,
@@ -4319,6 +4320,22 @@ ssl_IsRsaPssSignatureScheme(SSLSignatureScheme scheme)
     return PR_FALSE;
 }
 
+PRBool
+ssl_IsDsaSignatureScheme(SSLSignatureScheme scheme)
+{
+    switch (scheme) {
+        case ssl_sig_dsa_sha256:
+        case ssl_sig_dsa_sha384:
+        case ssl_sig_dsa_sha512:
+        case ssl_sig_dsa_sha1:
+            return PR_TRUE;
+
+        default:
+            return PR_FALSE;
+    }
+    return PR_FALSE;
+}
+
 SSLAuthType
 ssl_SignatureSchemeToAuthType(SSLSignatureScheme scheme)
 {
@@ -6031,6 +6048,13 @@ ssl_CanUseSignatureScheme(SSLSignatureScheme scheme,
         return PR_FALSE;
     }
 
+    if (ssl_IsDsaSignatureScheme(scheme) &&
+        (NSS_GetAlgorithmPolicy(SEC_OID_ANSIX9_DSA_SIGNATURE, &policy) ==
+         SECSuccess) &&
+        !(policy & NSS_USE_ALG_IN_SSL_KX)) {
+        return PR_FALSE;
+    }
+
     hashType = ssl_SignatureSchemeToHashType(scheme);
     if (requireSha1 && (hashType != ssl_hash_sha1)) {
         return PR_FALSE;
@@ -9509,6 +9533,14 @@ ssl3_EncodeSigAlgs(const sslSocket *ss, sslBuffer *buf)
             continue;
         }
 
+        /* Skip DSA scheme if it is disabled by policy. */
+        if (ssl_IsDsaSignatureScheme(ss->ssl3.signatureSchemes[i]) &&
+            (NSS_GetAlgorithmPolicy(SEC_OID_ANSIX9_DSA_SIGNATURE, &policy) ==
+             SECSuccess) &&
+            !(policy & NSS_USE_ALG_IN_SSL_KX)) {
+            continue;
+        }
+
         if ((NSS_GetAlgorithmPolicy(hashOID, &policy) != SECSuccess) ||
             (policy & NSS_USE_ALG_IN_SSL_KX)) {
             rv = sslBuffer_AppendNumber(buf, ss->ssl3.signatureSchemes[i], 2);

tests/ssl/sslpolicy.txt

@@ -74,6 +74,8 @@
 #	SECT409R1
 #	SECT571K1
 #	SECT571R1
+# Signatures:
+#	DSA
 # Hashes:
 #	MD2
 #	MD4
@@ -172,3 +174,4 @@
   1 noECC  SSL3   d    allow=tls-version-min=tls1.0:tls-version-max=tls1.2 Disallow Version Exlicitly
   1 noECC  SSL3   d    disallow=all_allow=hmac-sha1:sha256:rsa:des-ede3-cbc:tls-version-min=tls1.0:tls-version-max=tls1.2 Disallow Version Implicitly Narrow.
   1 noECC  SSL3   d    disallow=all_allow=md2/all:md4/all:md5/all:sha1/all:sha256/all:sha384/all:sha512/all:hmac-sha1/all:hmac-sha224/all:hmac-sha256/all:hmac-sha384/all:hmac-sha512/all:hmac-md5/all:camellia128-cbc/all:camellia192-cbc/all:camellia256-cbc/all:seed-cbc/all:des-ede3-cbc/all:des-40-cbc/all:des-cbc/all:null-cipher/all:rc2/all:rc4/all:idea/all:rsa/all:rsa-export/all:dhe-rsa/all:dhe-dss/all:ecdhe-ecdsa/all:ecdhe-rsa/all:ecdh-ecdsa/all:ecdh-rsa/all:tls-version-min=tls1.0:tls-version-max=tls1.2 Disallow Version Implicitly.
+  0 noECC  SSL3   d    disallow=dsa Disallow DSA Signatures Explicitly.