Implement SHA-256 fingerprint support

net/base/cert_verify_proc.cc

Index: net/base/cert_verify_proc.cc
===================================================================
--- net/base/cert_verify_proc.cc	(revision 151057)
+++ net/base/cert_verify_proc.cc	(working copy)
@@ -217,8 +217,9 @@
 }
 
 // static
+// NOTE: This implementation assumes and enforces that the hashes are SHA1.
 bool CertVerifyProc::IsPublicKeyBlacklisted(
-    const std::vector<SHA1Fingerprint>& public_key_hashes) {
+    const std::vector<HashValueVector>& public_key_hashes) {
   static const unsigned kNumHashes = 9;
   static const uint8 kHashes[kNumHashes][base::kSHA1Length] = {
     // Subject: CN=DigiNotar Root CA
@@ -263,11 +264,14 @@
      0xd1, 0x72, 0xbd, 0x53, 0xe0, 0xd3, 0x07, 0x83, 0x4b, 0xd1},
   };
 
+  const HashValueVector& sha1_hashes = public_key_hashes[HASH_VALUE_SHA1];

[Ryan Sleevi, 2012/08/11 01:39:55]

BUG: Accessing HASH_VALUE_SHA1 without checking that .size() >= HASH_VALUE_SHA1

Seems like you really want an std::map<HashValueTag, HashValueVector> here as
the arg, but for presumably perf, you're relying on the fact that a HashValueTag
is a viable index into an std::vector<>. I think that's really non-obvious, and
minimally requires more defensiveness.

My gut is that "adding DCHECKs" wouldn't be in itself sufficient

[palmer, 2012/08/14 19:40:42]

Well, the constructor guarantees that public_key_hashes has the right number of
items. Can't we count on that?

As I recall using a vector of vectors instead of a map of vectors was your
idea... :) I'm fine with changing it to a map if people think it's necessary or
better.

[Ryan Sleevi, 2012/08/14 20:02:51]

What constructor?

public_key_hashes is an input argument here. There's no guarantee that the
caller ensured it's in the proper form - or that it's not an empty vector.

That's what I meant here. Adding DCHECKs is great, but since it seems very
unlikely that caller's will be directly supplying hashes (as opposed to crafting
them from user/network input), it seems risky.
You could pass a (fixed-size) array of (dynamically-sized) vectors ;-) At least
then the compiler "should" warn, IIRC. More testing is needed to confirm I'm not
completely mistaken here

IsPublicKeyBlacklisted(const std::vector<HashValueVector>
(&public_key_hashes)[HASH_VALUES_TAG_COUNT])

That will pass a reference to a fixed-size array of 2 vectors. Beyond the
ugliness (which can probably be accomodated with a typedef), I don't recall what
the behaviour is for copy-assignment between two arrays - whether it performs a
memory-wise copy or if it invokes each members' copy constructor.

   for (unsigned i = 0; i < kNumHashes; i++) {
-    for (std::vector<SHA1Fingerprint>::const_iterator
-         j = public_key_hashes.begin(); j != public_key_hashes.end(); ++j) {
-      if (memcmp(j->data, kHashes[i], base::kSHA1Length) == 0)
+    for (HashValueVector::const_iterator j = sha1_hashes.begin();
+         j != sha1_hashes.end(); ++j) {
+      if (j->tag == HASH_VALUE_SHA1 &&
+          memcmp(j->data(), kHashes[i], base::kSHA1Length) == 0) {
         return true;
+      }
     }
   }