Refuse to accept certificate chains containing any RSA public key smaller

net/base/x509_certificate_unittest.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "base/pickle.h"
 #include "base/sha1.h"
 #include "base/string_number_conversions.h"
@@ skipping 593 matching lines @@
   int flags = X509Certificate::VERIFY_REV_CHECKING_ENABLED |
               X509Certificate::VERIFY_EV_CERT;
   int error = cert_chain->Verify("2029.globalsign.com", flags, NULL,
                                  &verify_result);
   if (error == OK)
     EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_IS_EV);
   else
     EXPECT_EQ(ERR_CERT_DATE_INVALID, error);
 }
 
+// Currently, only RSA and DSA are checked for weakness, and our example weak
+// size is 768. These could change in the future.
+static bool is_weak_key_type(const std::string& key_type) {

[Ryan Sleevi, 2011/12/13 05:45:35]

nit: Function naming guidelines are
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names

IsWeakKeyType()

+  size_t pos = key_type.find("-");
+  std::string size = key_type.substr(0, pos);
+  std::string type = key_type.substr(pos + 1);
+
+  return 0 == size.compare("768") &&

[Ryan Sleevi, 2011/12/13 05:45:35]

nit: Why use .compare() == 0 instead of the more common 

return size == "768" && (type == "rsa" || type == "dsa")

+         (0 == type.compare("rsa") || 0 == type.compare("dsa"));
+}
+
+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 secp256k1-ecdsa"

[Ryan Sleevi, 2011/12/13 05:45:35]

typo? You refer to secp256k1-ecdsa in the comment, but then in the code refer to
it as prime256v1-ecdsa (line 637)

+  // 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");
+  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) {

[Ryan Sleevi, 2011/12/13 05:45:35]

nit: This sort of iteration within a test, while not forbidden, can be replaced
by parameterized tests, which are consistent with the rest of the file.

One of the benefits for parameterized tests is you can track test
successes/failures a bit more independently (as opposed to one giant test
pass/fail). Another is that you don't have to worry about test timeouts (which
probably aren't much of a concern here).

It's a suggestion - It just struck me because you're wanting to test 16
different test permutations with only a single test.

const char* const kWeakKeyCertTypes[] = {
  "768-rsa",
  "1024-rsa",
  "2048-rsa",
  "prime256v1-ecdsa",
};

class X509CertificateWeakKeyTest :
  public ::testing::TestWithParam<std::tr1::tuple<const char*, const char*> > {
  virtual void SetUp() OVERRIDE {
    const ParamType& param = GetParam();
    end_entity_type_ = std::tr1::get<0>(param);
    signer_type_ = std::tr1::get<1>(param);
  }
  
  std::string end_entity_type_;
  std::string signer_type_;
};

TEST_P(X509CertificateWeakKeyTest, IsRejected) {
   ...
  // Remove the two for loops, replace *ee_type with ee_type_ and
  // *signer_type with signer_type_
  // You could also move more of the X509Certificate initialization into
SetUp(),
  // so that you just have to do cert_chain_->Verify(...) in this test. Up to
you
  // if you do decide to use this approach.
}

INSTANTIATE_TEST_P(, X509CertificateWeakKeyTest,
                   ::testing::Combine(
                       ::testing::ValuesIn(kWeakCertTypes),
                       ::testing::ValuesIn(kWeakCertTypes)))

+      std::string basename = *ee_type + "-ee-by-" + *signer_type +
+          "-intermediate.pem";
+      DLOG(WARNING) << "Now trying " << basename;
+      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 (is_weak_key_type(*ee_type) || is_weak_key_type(*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);
+      }
+    }
+  }
+}
+
 // Test for bug 94673.
 TEST(X509CertificateTest, GoogleDigiNotarTest) {
   FilePath certs_dir = GetTestCertsDirectory();
 
   scoped_refptr<X509Certificate> server_cert =
       ImportCertFromFile(certs_dir, "google_diginotar.pem");
   ASSERT_NE(static_cast<X509Certificate*>(NULL), server_cert);
 
   scoped_refptr<X509Certificate> intermediate_cert =
       ImportCertFromFile(certs_dir, "diginotar_public_ca_2025.pem");
@@ skipping 96 matching lines @@
   EXPECT_TRUE(X509Certificate::GetDEREncoded(cert->os_cert_handle(),
                                              &derBytes));
 
   base::StringPiece spkiBytes;
   EXPECT_TRUE(asn1::ExtractSPKIFromDERCert(derBytes, &spkiBytes));
 
   uint8 hash[base::kSHA1Length];
   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) {
   FilePath certs_dir = GetTestCertsDirectory();
   scoped_refptr<X509Certificate> cert =
       ImportCertFromFile(certs_dir, "nist.der");
   ASSERT_NE(static_cast<X509Certificate*>(NULL), cert);
 
   std::string derBytes;
   EXPECT_TRUE(X509Certificate::GetDEREncoded(cert->os_cert_handle(),
@@ skipping 634 matching lines @@
     { true, "f", "f" },
     { false, "h", "i" },
     { true, "bar.foo.com", "*.foo.com" },
     { true, "www.test.fr", "common.name",
         "*.test.com,*.test.co.uk,*.test.de,*.test.fr" },
     { true, "wwW.tESt.fr",  "common.name",
         ",*.*,*.test.de,*.test.FR,www" },
     { 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" },
     { false, "w.bar.foo.com", "w*.bar.foo.com" },
     { false, "www.bar.foo.com", "ww*ww.bar.foo.com" },
     { false, "wwww.bar.foo.com", "ww*ww.bar.foo.com" },
     { true, "wwww.bar.foo.com", "w*w.bar.foo.com" },
     { false, "wwww.bar.foo.com", "w*w.bar.foo.c0m" },
     { true, "WALLY.bar.foo.com", "wa*.bar.foo.com" },
     { true, "wally.bar.foo.com", "*Ly.bar.foo.com" },
@@ skipping 117 matching lines @@
   }
 
   if (test_data.ip_addrs) {
     // Build up the certificate IP address list.
     std::string ip_addrs_line(test_data.ip_addrs);
     std::vector<std::string> ip_addressses_ascii;
     base::SplitString(ip_addrs_line, ',', &ip_addressses_ascii);
     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))
             << "Could not parse hex address " << addr_ascii << " i = " << i;
         ip_addressses.push_back(std::string(reinterpret_cast<char*>(&bytes[0]),
                                             bytes.size()));
         ASSERT_EQ(16U, ip_addressses.back().size()) << i;
       } else {  // Decimal groups
         std::vector<std::string> decimals_ascii;
         base::SplitString(addr_ascii, '.', &decimals_ascii);
@@ skipping 217 matching lines @@
 #define MAYBE_VerifyMixed DISABLED_VerifyMixed
 #else
 #define MAYBE_VerifyMixed VerifyMixed
 #endif
 WRAPPED_INSTANTIATE_TEST_CASE_P(
     MAYBE_VerifyMixed,
     X509CertificateWeakDigestTest,
     testing::ValuesIn(kVerifyMixedTestData));
 
 }  // namespace net