net: allow CRLSets to block specific SPKIs.

net: allow CRLSets to block specific SPKIs.

This change allows CRLSets to include a list of blocked SPKI fingerprints,
which may save us doing emergency binary pushes in the future.

It also corrects a bug where the NSS code was passing in the full SPKI rather
than the SHA256 hash.

BUG=none
TEST=net_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117069

[wtc, 2012-01-10 00:50:58]

Patch Set 3 LGTM.

My comments are mostly nits and questions, but please note
the two comments marked with "IMPORTANT".

http://codereview.chromium.org/9149010/diff/10001/crypto/sha2.h
File crypto/sha2.h (right):

http://codereview.chromium.org/9149010/diff/10001/crypto/sha2.h#newcode12
crypto/sha2.h:12: #include "base/string_piece.h"

Nit: list "base" before "crypto".

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc
File net/base/crl_set.cc (right):

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc#newcode126
net/base/crl_set.cc:126: static base::DictionaryValue*
ReadHeader(base::StringPiece* data) {

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

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc#newcode185
net/base/crl_set.cc:185: if (!header_dict->GetList("BlockedSPKIs",
&blocked_spkis_list)) {

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

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc#newcode190
net/base/crl_set.cc:190: blocked_spkis_.clear();

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

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc#newcode192
net/base/crl_set.cc:192: for (size_t i = 0; i < blocked_spkis_list->GetSize();
i++) {

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

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc#newcode516
net/base/crl_set.cc:516: }

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.

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc#newcode519
net/base/crl_set.cc:519: return GOOD;

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.

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.h
File net/base/crl_set.h (right):

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.h#newcode46
net/base/crl_set.h:46: //   spki_hash: the SHA256 of the SubjectPublicKeyInfo

Nit: add "of the certificate".

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.h#newcode48
net/base/crl_set.h:48: //   issuer_spki_hash: the SHA256 of the
SubjectPublicKeyInfo of the CRL

You should describe what happens if issuer_spki_hash is empty
(the function returns GOOD),  because you call
CheckCertificate() with an empty issuer_spki_hash in
x509_certificate_nss.cc (the first iteration of the for
loop).

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.h#newcode87
net/base/crl_set.h:87: bool CopyBlockedSPKIsFromHeader(base::DictionaryValue*
header_dict);

Nit: perhaps call these "revoked SPKIs" rather than "blocked
SPKIs"?

Unlike the original function CRLSetFromHeader, I cannot
guess how CopyBlockedSPKIsFromHeader works by looking at
the function prototype alone.  So a brief description would
be helpful.

(What wasn't obvious to me was the output of this function.
I just looked at its implementation and saw that the output
is the blocked_spkis_ member.)

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.h#newcode96
net/base/crl_set.h:96: // blocked_spkis_ contains the SHA256 hash of SPKIs which
are to be blocked no

Nit: hash => hashes ?

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

Thanks. Running by try bots.

http://codereview.chromium.org/9149010/diff/10001/crypto/sha2.h
File crypto/sha2.h (right):

http://codereview.chromium.org/9149010/diff/10001/crypto/sha2.h#newcode12
crypto/sha2.h:12: #include "base/string_piece.h"
On 2012/01/10 00:50:59, wtc wrote:
> 
> Nit: list "base" before "crypto".

Done.

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc
File net/base/crl_set.cc (right):

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc#newcode126
net/base/crl_set.cc:126: static base::DictionaryValue*
ReadHeader(base::StringPiece* data) {
On 2012/01/10 00:50:59, wtc wrote:
> Just curious: why didn't we need base:: before?

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.

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc#newcode185
net/base/crl_set.cc:185: if (!header_dict->GetList("BlockedSPKIs",
&blocked_spkis_list)) {
On 2012/01/10 00:50:59, wtc wrote:
> Nit: might be better to name this header "RevokedSPKIs".

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).

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc#newcode190
net/base/crl_set.cc:190: blocked_spkis_.clear();
On 2012/01/10 00:50:59, wtc wrote:
> 
> IMPORTANT: does this blocked_spkis_.clear() call mean
> CRLSet::ApplyDelta() will lose the old blocked SPKIs?

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

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc#newcode192
net/base/crl_set.cc:192: for (size_t i = 0; i < blocked_spkis_list->GetSize();
i++) {
On 2012/01/10 00:50:59, wtc wrote:
> 
> Use ++i in all the new for loops.  It's more important to the
> for loops below, which use iterators.

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;
}

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc#newcode516
net/base/crl_set.cc:516: }
On 2012/01/10 00:50:59, wtc wrote:
> 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.

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

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc#newcode519
net/base/crl_set.cc:519: return GOOD;
On 2012/01/10 00:50:59, wtc wrote:
> I just want to confirm that this is intentional.

Yep.

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.h
File net/base/crl_set.h (right):

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.h#newcode46
net/base/crl_set.h:46: //   spki_hash: the SHA256 of the SubjectPublicKeyInfo
On 2012/01/10 00:50:59, wtc wrote:
> 
> Nit: add "of the certificate".

Done.

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.h#newcode48
net/base/crl_set.h:48: //   issuer_spki_hash: the SHA256 of the
SubjectPublicKeyInfo of the CRL
On 2012/01/10 00:50:59, wtc wrote:
> You should describe what happens if issuer_spki_hash is empty
> (the function returns GOOD),  because you call
> CheckCertificate() with an empty issuer_spki_hash in
> x509_certificate_nss.cc (the first iteration of the for
> loop).

(Have split the function to hopefully make it clearer.)

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.h#newcode87
net/base/crl_set.h:87: bool CopyBlockedSPKIsFromHeader(base::DictionaryValue*
header_dict);
On 2012/01/10 00:50:59, wtc wrote:
> Nit: perhaps call these "revoked SPKIs" rather than "blocked
> SPKIs"?

(See other comment for why I want to stick with "blocked".)

> Unlike the original function CRLSetFromHeader, I cannot
> guess how CopyBlockedSPKIsFromHeader works by looking at
> the function prototype alone.  So a brief description would
> be helpful.

Done.

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.h#newcode96
net/base/crl_set.h:96: // blocked_spkis_ contains the SHA256 hash of SPKIs which
are to be blocked no
On 2012/01/10 00:50:59, wtc wrote:
> 
> Nit: hash => hashes ?

[commit-bot: I haz the power, 2012-01-10 18:04:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/9149010/5005

[commit-bot: I haz the power, 2012-01-10 19:10:35]

Change committed as 117069

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

Patch Set 5 LGTM.

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc
File net/base/crl_set.cc (right):

http://codereview.chromium.org/9149010/diff/10001/net/base/crl_set.cc#newcode190
net/base/crl_set.cc:190: blocked_spkis_.clear();

On 2012/01/10 16:15:29, agl wrote:
>
> Yes, the blocked SPKIs are included in every CRLSet and aren't delta updated.

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