Reject certificate chains containing small RSA and DSA keys.

"Small" means less than 1024 bits.

BUG=102949
TEST=net_unittests, X509CertificateTest.*

Review URL: http://codereview.chromium.org/8568040

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@114709 0039d316-1c4b-4281-b951-d872f2087c98

net/base/cert_status_flags.cc

@@ -41,6 +41,8 @@ CertStatus MapNetErrorToCertStatus(int error) {
       return CERT_STATUS_INVALID;
     case ERR_CERT_WEAK_SIGNATURE_ALGORITHM:
       return CERT_STATUS_WEAK_SIGNATURE_ALGORITHM;
+    case ERR_CERT_WEAK_KEY:
+      return CERT_STATUS_WEAK_KEY;
     case ERR_CERT_NOT_IN_DNS:
       return CERT_STATUS_NOT_IN_DNS;
     default:
@@ -65,6 +67,8 @@ int MapCertStatusToNetError(CertStatus cert_status) {
     return ERR_CERT_COMMON_NAME_INVALID;
   if (cert_status & CERT_STATUS_WEAK_SIGNATURE_ALGORITHM)
     return ERR_CERT_WEAK_SIGNATURE_ALGORITHM;
+  if (cert_status & CERT_STATUS_WEAK_KEY)
+    return ERR_CERT_WEAK_KEY;
   if (cert_status & CERT_STATUS_DATE_INVALID)
     return ERR_CERT_DATE_INVALID;
 

net/base/cert_status_flags.h

@@ -32,6 +32,7 @@ static const CertStatus CERT_STATUS_INVALID                    = 1 << 7;
 static const CertStatus CERT_STATUS_WEAK_SIGNATURE_ALGORITHM   = 1 << 8;
 static const CertStatus CERT_STATUS_NOT_IN_DNS                 = 1 << 9;
 static const CertStatus CERT_STATUS_NON_UNIQUE_NAME            = 1 << 10;
+static const CertStatus CERT_STATUS_WEAK_KEY                   = 1 << 11;
 
 // Bits 16 to 31 are for non-error statuses.
 static const CertStatus CERT_STATUS_IS_EV                      = 1 << 16;

net/base/net_error_list.h

@@ -380,13 +380,17 @@ NET_ERROR(CERT_NOT_IN_DNS, -209)
 // The host name specified in the certificate is not unique.
 NET_ERROR(CERT_NON_UNIQUE_NAME, -210)
 
+// The server responded with a certificate that contains a weak key (e.g.
+// a too-small RSA key).
+NET_ERROR(CERT_WEAK_KEY, -211)
+
 // Add new certificate error codes here.
 //
 // Update the value of CERT_END whenever you add a new certificate error
 // code.
 
 // The value immediately past the last certificate error code.
-NET_ERROR(CERT_END, -211)
+NET_ERROR(CERT_END, -212)
 
 // The URL is invalid.
 NET_ERROR(INVALID_URL, -300)

net/base/x509_certificate.cc

@@ -224,6 +224,20 @@ X509Certificate::OSCertHandle CreateOSCert(base::StringPiece der_cert) {
 }
 #endif
 
+// Returns true if |type| is |kPublicKeyTypeRSA| or |kPublicKeyTypeDSA|, and
+// if |size_bits| is < 1024. Note that this means there may be false
+// negatives: keys for other algorithms and which are weak will pass this
+// test.
+bool IsWeakKey(X509Certificate::PublicKeyType type, size_t size_bits) {
+  switch (type) {
+    case X509Certificate::kPublicKeyTypeRSA:
+    case X509Certificate::kPublicKeyTypeDSA:
+      return size_bits < 1024;
+    default:
+      return false;
+  }
+}
+
 }  // namespace
 
 bool X509Certificate::LessThan::operator()(X509Certificate* lhs,
@@ -597,6 +611,31 @@ int X509Certificate::Verify(const std::string& hostname,
     rv = MapCertStatusToNetError(verify_result->cert_status);
   }
 
+  // Check for weak keys in the entire verified chain.
+  size_t size_bits = 0;
+  PublicKeyType type = kPublicKeyTypeUnknown;
+  bool weak_key = false;
+
+  GetPublicKeyInfo(verify_result->verified_cert->os_cert_handle(), &size_bits,
+                   &type);
+  if (IsWeakKey(type, size_bits)) {
+    weak_key = true;
+  } else {
+    const OSCertHandles& intermediates =
+        verify_result->verified_cert->GetIntermediateCertificates();
+    for (OSCertHandles::const_iterator i = intermediates.begin();
+         i != intermediates.end(); ++i) {
+      GetPublicKeyInfo(*i, &size_bits, &type);
+      if (IsWeakKey(type, size_bits))
+        weak_key = true;
+    }
+  }
+
+  if (weak_key) {
+    verify_result->cert_status |= CERT_STATUS_WEAK_KEY;
+    return MapCertStatusToNetError(verify_result->cert_status);
+  }
+
   // Treat certificates signed using broken signature algorithms as invalid.
   if (verify_result->has_md2 || verify_result->has_md4) {
     verify_result->cert_status |= CERT_STATUS_INVALID;

net/base/x509_certificate.h

@@ -74,6 +74,15 @@ class NET_EXPORT X509Certificate
 
   typedef std::vector<OSCertHandle> OSCertHandles;
 
+  enum PublicKeyType {
+    kPublicKeyTypeUnknown,
+    kPublicKeyTypeRSA,
+    kPublicKeyTypeDSA,
+    kPublicKeyTypeECDSA,
+    kPublicKeyTypeDH,
+    kPublicKeyTypeECDH
+  };
+
   // Predicate functor used in maps when X509Certificate is used as the key.
   class NET_EXPORT LessThan {
    public:
@@ -422,6 +431,13 @@ class NET_EXPORT X509Certificate
   // the first element.
   bool GetPEMEncodedChain(std::vector<std::string>* pem_encoded) const;
 
+  // Sets |*size_bits| to be the length of the public key in bits, and sets
+  // |*type| to one of the |PublicKeyType| values. In case of
+  // |kPublicKeyTypeUnknown|, |*size_bits| will be set to 0.
+  static void GetPublicKeyInfo(OSCertHandle cert_handle,
+                               size_t* size_bits,
+                               PublicKeyType* type);
+
   // Returns the OSCertHandle of this object. Because of caching, this may
   // differ from the OSCertHandle originally supplied during initialization.
   // Note: On Windows, CryptoAPI may return unexpected results if this handle

net/base/x509_certificate_mac.cc

@@ -812,7 +812,7 @@ X509Certificate* X509Certificate::CreateSelfSigned(
   }
 
   CSSM_BOOL confirmRequired;
-  CSSM_TP_RESULT_SET *resultSet = NULL;
+  CSSM_TP_RESULT_SET* resultSet = NULL;
   crtn = CSSM_TP_RetrieveCredResult(tp_handle, &refId, NULL, &estTime,
                                     &confirmRequired, &resultSet);
   ScopedEncodedCertResults scopedResults(resultSet);
@@ -1514,4 +1514,49 @@ bool X509Certificate::WriteOSCertHandleToPickle(OSCertHandle cert_handle,
                            cert_data.Length);
 }
 
+// static
+void X509Certificate::GetPublicKeyInfo(OSCertHandle cert_handle,
+                                       size_t* size_bits,
+                                       PublicKeyType* type) {
+  // Since we might fail, set the output parameters to default values first.
+  *type = kPublicKeyTypeUnknown;
+  *size_bits = 0;
+
+  SecKeyRef key;
+  OSStatus status = SecCertificateCopyPublicKey(cert_handle, &key);
+  if (status) {
+    NOTREACHED() << "SecCertificateCopyPublicKey failed: " << status;
+    return;
+  }
+  ScopedCFTypeRef<SecKeyRef> scoped_key;
+
+  const CSSM_KEY* cssm_key;
+  status = SecKeyGetCSSMKey(key, &cssm_key);
+  if (status) {
+    NOTREACHED() << "SecKeyGetCSSMKey failed: " << status;
+    return;
+  }
+
+  *size_bits = cssm_key->KeyHeader.LogicalKeySizeInBits;
+
+  switch (cssm_key->KeyHeader.AlgorithmId) {
+    case CSSM_ALGID_RSA:
+      *type = kPublicKeyTypeRSA;
+      break;
+    case CSSM_ALGID_DSA:
+      *type = kPublicKeyTypeDSA;
+      break;
+    case CSSM_ALGID_ECDSA:
+      *type = kPublicKeyTypeECDSA;
+      break;
+    case CSSM_ALGID_DH:
+      *type = kPublicKeyTypeDH;
+      break;
+    default:
+      *type = kPublicKeyTypeUnknown;
+      *size_bits = 0;
+      break;
+  }
+}
+
 }  // namespace net

net/base/x509_certificate_nss.cc

@@ -1142,4 +1142,38 @@ bool X509Certificate::WriteOSCertHandleToPickle(OSCertHandle cert_handle,
       cert_handle->derCert.len);
 }
 
+// static
+void X509Certificate::GetPublicKeyInfo(OSCertHandle cert_handle,
+                                       size_t* size_bits,
+                                       PublicKeyType* type) {
+  // Since we might fail, set the output parameters to default values first.
+  *type = kPublicKeyTypeUnknown;
+  *size_bits = 0;
+
+  SECKEYPublicKey* key = CERT_ExtractPublicKey(cert_handle);
+  if (!key)
+    return;
+
+  *size_bits = SECKEY_PublicKeyStrengthInBits(key);
+
+  switch (key->keyType) {
+    case rsaKey:
+      *type = kPublicKeyTypeRSA;
+      break;
+    case dsaKey:
+      *type = kPublicKeyTypeDSA;
+      break;
+    case dhKey:
+      *type = kPublicKeyTypeDH;
+      break;
+    case ecKey:
+      *type = kPublicKeyTypeECDSA;
+      break;
+    default:
+      *type = kPublicKeyTypeUnknown;
+      *size_bits = 0;
+      break;
+  }
+}
+
 }  // namespace net

net/base/x509_certificate_openssl.cc

@@ -662,6 +662,37 @@ bool X509Certificate::WriteOSCertHandleToPickle(OSCertHandle cert_handle,
       der_cache.data_length);
 }
 
+// static
+void X509Certificate::GetPublicKeyInfo(OSCertHandle cert_handle,
+                                       size_t* size_bits,
+                                       PublicKeyType* type) {
+  EVP_PKEY* key = X509_get_pubkey(cert_handle);
+  CHECK(key);
+
+  switch (key->type) {
+    case EVP_PKEY_RSA:
+      *type = kPublicKeyTypeRSA;
+      *size_bits = EVP_PKEY_size(key) * 8;
+      break;
+    case EVP_PKEY_DSA:
+      *type = kPublicKeyTypeDSA;
+      *size_bits = EVP_PKEY_size(key) * 8;
+      break;
+    case EVP_PKEY_EC:
+      *type = kPublicKeyTypeECDSA;
+      *size_bits = EVP_PKEY_size(key);
+      break;
+    case EVP_PKEY_DH:
+      *type = kPublicKeyTypeDH;
+      *size_bits = EVP_PKEY_size(key) * 8;
+      break;
+    default:
+      *type = kPublicKeyTypeUnknown;
+      *size_bits = 0;
+      break;
+  }
+}
+
 #if defined(OS_ANDROID)
 void X509Certificate::GetChainDEREncodedBytes(
     std::vector<std::string>* chain_bytes) const {

net/base/x509_certificate_unittest.cc

@@ -24,6 +24,12 @@
 #include <cert.h>
 #endif
 
+#if defined(OS_WIN)
+#include "base/win/windows_version.h"
+#elif defined(OS_MACOSX)
+#include "base/mac/mac_util.h"
+#endif
+
 // Unit tests aren't allowed to access external resources. Unfortunately, to
 // properly verify the EV-ness of a cert, we need to check for its revocation
 // through online servers. If you're manually running unit tests, feel free to
@@ -611,6 +617,90 @@ TEST(X509CertificateTest, DISABLED_GlobalSignR3EVTest) {
     EXPECT_EQ(ERR_CERT_DATE_INVALID, error);
 }
 
+// Currently, only RSA and DSA keys are checked for weakness, and our example
+// weak size is 768. These could change in the future.
+//
+// Note that this means there may be false negatives: keys for other
+// algorithms and which are weak will pass this test.
+static bool IsWeakKeyType(const std::string& key_type) {
+  size_t pos = key_type.find("-");
+  std::string size = key_type.substr(0, pos);
+  std::string type = key_type.substr(pos + 1);
+
+  if (type == "rsa" || type == "dsa")
+    return size == "768";
+
+  return false;
+}
+
+TEST(X509CertificateTest, RejectWeakKeys) {
+  FilePath certs_dir = GetTestCertsDirectory();
+  typedef std::vector<std::string> Strings;
+  Strings key_types;
+
+  // generate-weak-test-chains.sh currently has:
+  //     key_types="768-rsa 1024-rsa 2048-rsa prime256v1-ecdsa"
+  // We must use the same key types here. The filenames generated look like:
+  //     2048-rsa-ee-by-768-rsa-intermediate.pem
+  key_types.push_back("768-rsa");
+  key_types.push_back("1024-rsa");
+  key_types.push_back("2048-rsa");
+
+  bool use_ecdsa = true;
+#if defined(OS_WIN)
+  use_ecdsa = base::win::GetVersion() > base::win::VERSION_XP;
+#elif defined(OS_MACOSX)
+  use_ecdsa = base::mac::IsOSSnowLeopardOrLater();
+#endif
+
+  if (use_ecdsa)
+    key_types.push_back("prime256v1-ecdsa");
+
+  // Add the root that signed the intermediates for this test.
+  scoped_refptr<X509Certificate> root_cert =
+      ImportCertFromFile(certs_dir, "2048-rsa-root.pem");
+  ASSERT_NE(static_cast<X509Certificate*>(NULL), root_cert);
+  TestRootCerts::GetInstance()->Add(root_cert.get());
+
+  // Now test each chain.
+  for (Strings::const_iterator ee_type = key_types.begin();
+       ee_type != key_types.end(); ++ee_type) {
+    for (Strings::const_iterator signer_type = key_types.begin();
+         signer_type != key_types.end(); ++signer_type) {
+      std::string basename = *ee_type + "-ee-by-" + *signer_type +
+          "-intermediate.pem";
+      scoped_refptr<X509Certificate> ee_cert =
+          ImportCertFromFile(certs_dir, basename);
+      ASSERT_NE(static_cast<X509Certificate*>(NULL), ee_cert);
+
+      basename = *signer_type + "-intermediate.pem";
+      scoped_refptr<X509Certificate> intermediate =
+          ImportCertFromFile(certs_dir, basename);
+      ASSERT_NE(static_cast<X509Certificate*>(NULL), intermediate);
+
+      X509Certificate::OSCertHandles intermediates;
+      intermediates.push_back(intermediate->os_cert_handle());
+      scoped_refptr<X509Certificate> cert_chain =
+          X509Certificate::CreateFromHandle(ee_cert->os_cert_handle(),
+                                            intermediates);
+
+      CertVerifyResult verify_result;
+      int error = cert_chain->Verify("127.0.0.1", 0, NULL, &verify_result);
+
+      if (IsWeakKeyType(*ee_type) || IsWeakKeyType(*signer_type)) {
+        EXPECT_NE(OK, error);
+        EXPECT_EQ(CERT_STATUS_WEAK_KEY,
+                  verify_result.cert_status & CERT_STATUS_WEAK_KEY);
+      } else {
+        EXPECT_EQ(OK, error);
+        EXPECT_EQ(0U, verify_result.cert_status & CERT_STATUS_WEAK_KEY);
+      }
+    }
+  }
+
+  TestRootCerts::GetInstance()->Clear();
+}
+
 // Test for bug 94673.
 TEST(X509CertificateTest, GoogleDigiNotarTest) {
   FilePath certs_dir = GetTestCertsDirectory();
@@ -727,7 +817,7 @@ TEST(X509CertificateTest, ExtractSPKIFromDERCert) {
   base::SHA1HashBytes(reinterpret_cast<const uint8*>(spkiBytes.data()),
                       spkiBytes.size(), hash);
 
-  EXPECT_TRUE(0 == memcmp(hash, nistSPKIHash, sizeof(hash)));
+  EXPECT_EQ(0, memcmp(hash, nistSPKIHash, sizeof(hash)));
 }
 
 TEST(X509CertificateTest, ExtractCRLURLsFromDERCert) {
@@ -1382,7 +1472,7 @@ const CertificateNameVerifyTestData kNameVerifyTestData[] = {
     { false, "f.uk", ".uk" },
     { false, "w.bar.foo.com", "?.bar.foo.com" },
     { false, "www.foo.com", "(www|ftp).foo.com" },
-    { false, "www.foo.com", "www.foo.com#" }, // # = null char.
+    { false, "www.foo.com", "www.foo.com#" },  // # = null char.
     { false, "www.foo.com", "", "www.foo.com#*.foo.com,#,#" },
     { false, "www.house.example", "ww.house.example" },
     { false, "test.org", "", "www.test.org,*.test.org,*.org" },
@@ -1520,7 +1610,7 @@ TEST_P(X509CertificateNameVerifyTest, VerifyHostname) {
     for (size_t i = 0; i < ip_addressses_ascii.size(); ++i) {
       std::string& addr_ascii = ip_addressses_ascii[i];
       ASSERT_NE(0U, addr_ascii.length());
-      if (addr_ascii[0] == 'x') { // Hex encoded address
+      if (addr_ascii[0] == 'x') {  // Hex encoded address
         addr_ascii.erase(0, 1);
         std::vector<uint8> bytes;
         EXPECT_TRUE(base::HexStringToBytes(addr_ascii, &bytes))