net: allow CRLSets to block specific SPKIs.

net/base/crl_set.cc

 // Copyright (c) 2012 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/base64.h"
 #include "base/format_macros.h"
 #include "base/json/json_reader.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
@@ skipping 105 matching lines @@
 // header's "DeltaFrom" value. The delta describes the changes to each CRL
 // in turn with a zlib compressed array of options: either the CRL is the same,
 // a new CRL is inserted, the CRL is deleted or the CRL is updated. In the case
 // of an update, the serials in the CRL are considered in the same fashion
 // except there is no delta update of a serial number: they are either
 // inserted, deleted or left the same.
 
 // ReadHeader reads the header (including length prefix) from |data| and
 // updates |data| to remove the header on return. Caller takes ownership of the
 // returned pointer.
-static DictionaryValue* ReadHeader(base::StringPiece* data) {
+static base::DictionaryValue* ReadHeader(base::StringPiece* data) {

[wtc, 2012/01/10 00:50:59]

Just curious: why didn't we need base:: before?

[agl, 2012/01/10 16:15:29]

There are 'using' statements in the header which make it optional, but it
appears to be only for backwards-compat. New code is supposed to be using the
namespaced names.

   if (data->size() < 2)
     return NULL;
   uint16 header_len;
   memcpy(&header_len, data->data(), 2);  // assumes little-endian.
   data->remove_prefix(2);
 
   if (data->size() < header_len)
     return NULL;
 
   const base::StringPiece header_bytes(data->data(), header_len);
   data->remove_prefix(header_len);
 
   scoped_ptr<Value> header(base::JSONReader::Read(
       header_bytes.as_string(), true /* allow trailing comma */));
   if (header.get() == NULL)
     return NULL;
 
   if (!header->IsType(Value::TYPE_DICTIONARY))
     return NULL;
-  return reinterpret_cast<DictionaryValue*>(header.release());
+  return reinterpret_cast<base::DictionaryValue*>(header.release());
 }
 
 // kCurrentFileVersion is the version of the CRLSet file format that we
 // currently implement.
 static const int kCurrentFileVersion = 0;
 
 static bool ReadCRL(base::StringPiece* data, std::string* out_parent_spki_hash,
                     std::vector<std::string>* out_serials) {
   if (data->size() < crypto::kSHA256Length)
     return false;
@@ skipping 16 matching lines @@
     if (data->size() < serial_length)
       return false;
     std::string serial(data->data(), serial_length);
     data->remove_prefix(serial_length);
     out_serials->push_back(serial);
   }
 
   return true;
 }
 
+bool CRLSet::CopyBlockedSPKIsFromHeader(base::DictionaryValue* header_dict) {
+  ListValue* blocked_spkis_list = NULL;
+  if (!header_dict->GetList("BlockedSPKIs", &blocked_spkis_list)) {

[wtc, 2012/01/10 00:50:59]

Nit: might be better to name this header "RevokedSPKIs".

[agl, 2012/01/10 16:15:29]

I'd actually like to keep it as blocked. Revoked means that the CA has revoked
the certificate and we're simply recording that fact and passing it on. However,
the SPKIs that we list manually might well not have been revoked in the
technical sense (i.e. DigiNotar).

+    // BlockedSPKIs is optional, so it's fine if we don't find it.
+    return true;
+  }
+
+  blocked_spkis_.clear();

[wtc, 2012/01/10 00:50:59]

IMPORTANT: does this blocked_spkis_.clear() call mean
CRLSet::ApplyDelta() will lose the old blocked SPKIs?

[agl, 2012/01/10 16:15:29]

Yes, the blocked SPKIs are included in every CRLSet and aren't delta updated.
Hopefully there won't be many of them.

[wtc, 2012/01/11 01:15:12]

It would be nice to document this somewhere.  This is not
obvious.

+
+  for (size_t i = 0; i < blocked_spkis_list->GetSize(); i++) {

[wtc, 2012/01/10 00:50:59]

Use ++i in all the new for loops.  It's more important to the
for loops below, which use iterators.

[agl, 2012/01/10 16:15:29]

Done, but it's really a waste of time! For example, the following code complies
to *exactly* the .o file when optimisations are on, whether i++ or ++i is used:


#include <vector>
#include <string>

bool f(const std::vector<std::string>& v) {
  for (std::vector<std::string>::const_iterator i = v.begin(); i != v.end();
i++) {
    if (*i == "foo")
      return true;
  }

  return false;
}

+    std::string spki_sha256_base64, spki_sha256;
+    if (!blocked_spkis_list->GetString(i, &spki_sha256_base64))
+      return false;
+    if (!base::Base64Decode(spki_sha256_base64, &spki_sha256))
+      return false;
+    blocked_spkis_.push_back(spki_sha256);
+  }
+
+  return true;
+}
+
 // static
 bool CRLSet::Parse(base::StringPiece data, scoped_refptr<CRLSet>* out_crl_set) {
   // Other parts of Chrome assume that we're little endian, so we don't lose
   // anything by doing this.
 #if defined(__BYTE_ORDER)
   // Linux check
   COMPILE_ASSERT(__BYTE_ORDER == __LITTLE_ENDIAN, assumes_little_endian);
 #elif defined(__BIG_ENDIAN__)
   // Mac check
   #error assumes little endian
 #endif
 
-  scoped_ptr<DictionaryValue> header_dict(ReadHeader(&data));
+  scoped_ptr<base::DictionaryValue> header_dict(ReadHeader(&data));
   if (!header_dict.get())
     return false;
 
   std::string contents;
   if (!header_dict->GetString("ContentType", &contents))
     return false;
   if (contents != "CRLSet")
     return false;
 
   int version;
@@ skipping 12 matching lines @@
   for (size_t crl_index = 0; !data.empty(); crl_index++) {
     std::string parent_spki_sha256;
     std::vector<std::string> serials;
     if (!ReadCRL(&data, &parent_spki_sha256, &serials))
       return false;
 
     crl_set->crls_.push_back(std::make_pair(parent_spki_sha256, serials));
     crl_set->crls_index_by_issuer_[parent_spki_sha256] = crl_index;
   }
 
+  if (!crl_set->CopyBlockedSPKIsFromHeader(header_dict.get()))
+    return false;
+
   *out_crl_set = crl_set;
   return true;
 }
 
 // kMaxUncompressedChangesLength is the largest changes array that we'll
 // accept. This bounds the number of CRLs in the CRLSet as well as the number
 // of serial numbers in a given CRL.
 static const unsigned kMaxUncompressedChangesLength = 1024 * 1024;
 
 static bool ReadChanges(base::StringPiece* data,
@@ skipping 69 matching lines @@
   }
 
   if (i != old_serials.size())
     return false;
   return true;
 }
 
 bool CRLSet::ApplyDelta(const base::StringPiece& in_data,
                         scoped_refptr<CRLSet>* out_crl_set) {
   base::StringPiece data(in_data);
-  scoped_ptr<DictionaryValue> header_dict(ReadHeader(&data));
+  scoped_ptr<base::DictionaryValue> header_dict(ReadHeader(&data));
   if (!header_dict.get())
     return false;
 
   std::string contents;
   if (!header_dict->GetString("ContentType", &contents))
     return false;
   if (contents != "CRLSetDelta")
     return false;
 
   int version;
   if (!header_dict->GetInteger("Version", &version) ||
       version != kCurrentFileVersion) {
     return false;
   }
 
   int sequence, delta_from;
   if (!header_dict->GetInteger("Sequence", &sequence) ||
       !header_dict->GetInteger("DeltaFrom", &delta_from) ||
       delta_from < 0 ||
       static_cast<uint32>(delta_from) != sequence_) {
     return false;
   }
 
   scoped_refptr<CRLSet> crl_set(new CRLSet);
   crl_set->sequence_ = static_cast<uint32>(sequence);
 
+  if (!crl_set->CopyBlockedSPKIsFromHeader(header_dict.get()))
+    return false;
+
   std::vector<uint8> crl_changes;
 
   if (!ReadChanges(&data, &crl_changes))
     return false;
 
   size_t i = 0, j = 0;
   for (std::vector<uint8>::const_iterator k = crl_changes.begin();
        k != crl_changes.end(); ++k) {
     if (*k == SYMBOL_SAME) {
       if (i >= crls_.size())
@@ skipping 36 matching lines @@
     return false;
 
   *out_crl_set = crl_set;
   return true;
 }
 
 // static
 bool CRLSet::GetIsDeltaUpdate(const base::StringPiece& in_data,
                               bool *is_delta) {
   base::StringPiece data(in_data);
-  scoped_ptr<DictionaryValue> header_dict(ReadHeader(&data));
+  scoped_ptr<base::DictionaryValue> header_dict(ReadHeader(&data));
   if (!header_dict.get())
     return false;
 
   std::string contents;
   if (!header_dict->GetString("ContentType", &contents))
     return false;
 
   if (contents == "CRLSet") {
     *is_delta = false;
   } else if (contents == "CRLSetDelta") {
     *is_delta = true;
   } else {
     return false;
   }
 
   return true;
 }
 
 std::string CRLSet::Serialize() const {
   std::string header = StringPrintf(
       "{"
       "\"Version\":0,"
       "\"ContentType\":\"CRLSet\","
       "\"Sequence\":%u,"
       "\"DeltaFrom\":0,"
-      "\"NumParents\":%u"
-      "}",
+      "\"NumParents\":%u,"
+      "\"BlockedSPKIs\":[",
       static_cast<unsigned>(sequence_),
       static_cast<unsigned>(crls_.size()));
 
+  for (std::vector<std::string>::const_iterator i = blocked_spkis_.begin();
+       i != blocked_spkis_.end(); i++) {
+    std::string spki_hash_base64;
+    base::Base64Encode(*i, &spki_hash_base64);
+
+    if (i != blocked_spkis_.begin())
+      header += ",";
+    header += "\"" + spki_hash_base64 + "\"";
+  }
+  header += "]}";
+
   size_t len = 2 /* header len */ + header.size();
 
   for (CRLList::const_iterator i = crls_.begin(); i != crls_.end(); ++i) {
     len += i->first.size() + 4 /* num serials */;
     for (std::vector<std::string>::const_iterator j = i->second.begin();
          j != i->second.end(); j++) {
       len += 1 /* serial length */ + j->size();
     }
   }
 
@@ skipping 18 matching lines @@
       memcpy(out + off, j->data(), j->size());
       off += j->size();
     }
   }
 
   CHECK_EQ(off, len);
   return ret;
 }
 
 CRLSet::Result CRLSet::CheckCertificate(
+    const base::StringPiece& spki_hash,
     const base::StringPiece& serial_number,
-    const base::StringPiece& parent_spki) const {
+    const base::StringPiece& issuer_spki_hash) const {
+  for (std::vector<std::string>::const_iterator i = blocked_spkis_.begin();
+       i != blocked_spkis_.end(); i++) {
+    if (spki_hash.size() == i->size() &&
+        memcmp(spki_hash.data(), i->data(), i->size()) == 0) {
+      return REVOKED;
+    }
+  }

[wtc, 2012/01/10 00:50:59]

Nit: this function performs two independent checks, divided
by this line.

I wonder if it makes sense to break this function into two
functions, to make it clear that spki_hash is only used in
the first check, and serial_number and issuer_spki_hash are
only used in the second check.

[agl, 2012/01/10 16:15:29]

Yes, I think that's a good idea, thanks. I've called them CheckSPKI and
CheckSerial.

+
+  if (issuer_spki_hash.empty())
+    return GOOD;

[wtc, 2012/01/10 00:50:59]

IMPORTANT: the location of this check means blocked_spkis_
can also revoke root certificates.  I think this is a
convenient protection before the system root lists are
updated.

I just want to confirm that this is intentional.

[agl, 2012/01/10 16:15:29]

Yep.

+
   base::StringPiece serial(serial_number);
 
   if (!serial.empty() && (serial[0] & 0x80) != 0) {
     // This serial number is negative but the process which generates CRL sets
     // will reject any certificates with negative serial numbers as invalid.
     return UNKNOWN;
   }
 
   // Remove any leading zero bytes.
   while (serial.size() > 1 && serial[0] == 0x00)
     serial.remove_prefix(1);
 
   std::map<std::string, size_t>::const_iterator i =
-      crls_index_by_issuer_.find(parent_spki.as_string());
+      crls_index_by_issuer_.find(issuer_spki_hash.as_string());
   if (i == crls_index_by_issuer_.end())
     return UNKNOWN;
   const std::vector<std::string>& serials = crls_[i->second].second;
 
   for (std::vector<std::string>::const_iterator i = serials.begin();
        i != serials.end(); ++i) {
     if (base::StringPiece(*i) == serial)
       return REVOKED;
   }
 
   return GOOD;
 }
 
 uint32 CRLSet::sequence() const {
   return sequence_;
 }
 
 const CRLSet::CRLList& CRLSet::crls() const {
   return crls_;
 }
 
 }  // namespace net