Refuse to accept certificate chains containing any RSA public key smaller

net/base/x509_certificate_unittest.cc

Index: net/base/x509_certificate_unittest.cc
===================================================================
--- net/base/x509_certificate_unittest.cc	(revision 113824)
+++ net/base/x509_certificate_unittest.cc	(working copy)
@@ -611,6 +611,75 @@
     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();
@@ -727,7 +796,7 @@
   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 +1451,7 @@
     { 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 +1589,7 @@
     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))