Fix the "certificate is not yet valid" error for server certificates

Fix the "certificate is not yet valid" error for server certificates
issued by a VeriSign intermediate CA.

Change the CertVerifier cache to identify a certificate chain by the
hash of the entire chain rather than just the server certificate.

This requires adding X509Certificate::chain_fingerprint(), and the
X509Certificate::CalculateChainFingerprint() method to compute the
chain fingerprint.

R=agl@chromium.org,rsleevi@chromium.org
BUG=101555
TEST=X509CertificateTest.ChainFingerprints and
CertVerifierTest.DifferentCACerts in net_unittests

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

[wtc, 2011-10-28 22:24:04]

Please review.  You can review it in the following order.

x509_certificate.h: new methods chain_fingerprint() and
CalculateChainFingerprint().

x509_certificate_xxx.cc: implementation of the new methods.

x509_certificate.cc: use chain_fingerprint() in the LessThan
operator for X509Certificate objects.

x509_certificate_unittestc.cc: new unit test.

cert_verifier.cc: use X509Certificate::chain_fingerprint()
isntead of X509Certificate::fingerprint().

cert_verifier_unittest.cc: new unit test

http://codereview.chromium.org/8400075/diff/1/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/8400075/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:375: SHA1Fingerprint CalculateChainFingerprint()
const;

This method should be private.  I list it here so that it
can be defined next to the closely related CalculateFingerprint
method.

http://codereview.chromium.org/8400075/diff/1/net/base/x509_certificate_win.cc
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/8400075/diff/1/net/base/x509_certificate_win.c...
net/base/x509_certificate_win.cc:1024: return
CalculateFingerprint(cert_handle_);

I still need to implement this on Windows.  If necessary,
I can comment out the two unit tests for Windows because
this bug affects Mac OS X primarily.

[Ryan Sleevi, 2011-10-28 22:47:00]

I haven't looked closely at the SHA-1 implementations yet, because I'm wondering
whether or not this should be calculated for every X509Certificate.

I don't think there is particular advantage in calculating this during the
X509Certificate initialization, and can see particular harm to performance,
because we use and create many X509Certificate's independent of those that
directly end up in the cached cert verifier.

I realize this is almost certainly going to be merged into Beta/Stable, and so
there is a desire to limit impact, but I would suggest the following design
changes:
- Make it a member function (or a free function) to calculate the chain
fingerprint. At most, this will likely only be called once (by the cert
verifier, when handling new certs)
- TODO (in a future CL) Move the SHA-1 code out from the platform-specific bits
- I don't think it belongs in X509Certificate
- TODO (in a future CL) Make GetDEREncoded a static function that takes an
OSCertHandle. This recently came up with Palmer's change to report invalid
chains, as he needed to PEM encode, and so this strikes me as exactly the kind
of thing that should be abstracted out.
  - Consider his current implementation, which creates an X509Certificate for
each OSCertHandle simply in order to be able to call GetDEREncoded(). Now you
also have to pay the performance penalty of chain_fingerprint() for those certs,
which will never be fingerprinted.

Because of the changes earlier in the year to the X509Certificate cache, many
more X509Certificates are created (in the past, a cached one was returned), so
I'm more inclined to keep work out of construction cost, unless it ends up being
so universally accessed that it benefits from caching.

This also overlaps some with sergeyu's thinking, which moved to separate
"certificate information" from "certificate handle".

What do you think about these suggestions?

http://codereview.chromium.org/8400075/diff/1/net/base/x509_certificate_mac.cc
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/8400075/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:1077: // The CC_SHA(3cc) man page says all
CC_SHA1_xxx routines return 1, so
TODO - I think we should look to abstract out the platform-specifics of these
SHA-1 implementations. There is base/sha1.h, but it doesn't support streaming
updates. Presumably crypto/sha2.h would also be updated.

This applies to all the implementations.

http://codereview.chromium.org/8400075/diff/1/net/data/ssl/certificates/README
File net/data/ssl/certificates/README (right):

http://codereview.chromium.org/8400075/diff/1/net/data/ssl/certificates/READM...
net/data/ssl/certificates/README:60: - salesforce_com_test.der
Can you run these through OpenSSL to PEM encode them

openssl x509 -in [foo.der] -inform DER -out [foo.pem] -outform PEM -text

So that human readable certificate information is there, and for easier
comparison?

http://codereview.chromium.org/8400075/diff/1/net/third_party/nss/ssl/sslsock.c
File net/third_party/nss/ssl/sslsock.c (right):

http://codereview.chromium.org/8400075/diff/1/net/third_party/nss/ssl/sslsock...
net/third_party/nss/ssl/sslsock.c:1329: return SECSuccess;
Leaked from agl's CL?

[Ryan Sleevi, 2011-10-28 22:48:23]

On 2011/10/28 22:47:00, Ryan Sleevi wrote:
<snip>
> - I don't think it belongs in X509Certificate

Sorry, to clarify - I don't think it belongs as a member variable in
X509Certificate - that is, calculated on construction. I think it should be
calculated on demand, as I believe that in most cases it will only be
calculated/accessed once. This should be easy to confirm with a DCHECK and an
access counter.

[wtc, 2011-10-28 23:21:37]

rsleevi: thank you for the review and suggestions.

http://codereview.chromium.org/8400075/diff/1/net/data/ssl/certificates/README
File net/data/ssl/certificates/README (right):

http://codereview.chromium.org/8400075/diff/1/net/data/ssl/certificates/READM...
net/data/ssl/certificates/README:60: - salesforce_com_test.der

On 2011/10/28 22:47:01, Ryan Sleevi wrote:
> Can you run these through OpenSSL to PEM encode them

This quadruples the file size (from 1KB to 4KB),  Seems
like a high price for human readability :-)

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:235: return
fingerprint_functor(lhs->chain_fingerprint_, rhs->chain_fingerprint_);

This LessThan operator is what made me calculate the chain
fingerprint for every X509Certificate.

The LessThan operator needs to take intermediate_ca_certs_
into account.  Do you have any suggestion?

Perhaps we can just do pointer comparison on the
intermediate CA cert handles, because of the OSCertHandle
cache?

[Ryan Sleevi, 2011-10-28 23:32:43]

http://codereview.chromium.org/8400075/diff/1/net/data/ssl/certificates/README
File net/data/ssl/certificates/README (right):

http://codereview.chromium.org/8400075/diff/1/net/data/ssl/certificates/READM...
net/data/ssl/certificates/README:60: - salesforce_com_test.der
On 2011/10/28 23:21:37, wtc wrote:
> 
> On 2011/10/28 22:47:01, Ryan Sleevi wrote:
> > Can you run these through OpenSSL to PEM encode them
> 
> This quadruples the file size (from 1KB to 4KB),  Seems
> like a high price for human readability :-)

For source-only downloads, I didn't think this was a particular concern (eg:
considering how many third-party deps are pulled in, or considering the NewGit
work that pulls in the entire git history of the WebKit repository)

It also allows you to run tryjobs - otherwise you have to do staged landings.

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:235: return
fingerprint_functor(lhs->chain_fingerprint_, rhs->chain_fingerprint_);
On 2011/10/28 23:21:37, wtc wrote:
> 
> This LessThan operator is what made me calculate the chain
> fingerprint for every X509Certificate.
> 
> The LessThan operator needs to take intermediate_ca_certs_
> into account.  Do you have any suggestion?
> 
> Perhaps we can just do pointer comparison on the
> intermediate CA cert handles, because of the OSCertHandle
> cache?

The only instance I see using this is the content/browser/cert_store.h
(previously it was also used by the old cache).

Perhaps I'm over-thinking it, but it would seem that it's possible to just
update the CertStore to use the ChainFingerprint() as the key, since it doesn't
need to look up the actual |X509Certificate*|.

Then we could remove this functor entirely.

So this would make it most likely just two calls (the first time during the
CertVerifier cache lookup, the second time during the CertStore lookup).

I know without metrics, it's hard to judge which is worse, but considering
you've expressed concerns with just the |fingerprint_| in the past, and based on
how widely we use X509Certificate for things other than CertVerifier/CertStore,
my gut is that as-is will end up calculating fingerprints more often than just
updating those two.

[agl, 2011-10-28 23:45:47]

LGTM.

I'd suggest landing a minimal change first because it's a merge candidate. I
think calculating the chain fingerprint everytime is a little unfortunate, but
adding complexity to a change that we hope to merge to stable is worse.

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:1087: status =
SecCertificateGetData(intermediate_ca_certs_[i], &cert_data);
I note in the OS X documentation that this is listed as deprecated as of 10.7. I
don't think that it's important right now.

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate_wi...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate_wi...
net/base/x509_certificate_win.cc:1023: // TODO(wtc): implement this.
It's not clear how this can be a TODO if the change is to fix the bug. Will this
not result in the current behaviour? Are you assuming that it's not urgent to
fix on Windows because we don't have any reports of problems on Windows? Ok if
so.

[Ryan Sleevi, 2011-10-28 23:55:02]

I agree with agl's prioritization (smaller is better for the merge), which is
why I tried to indicate the bits as TODO.

This LGTM as-is (although with the hope that this can be fixed in trunk/M-17),
with the following two BUGs and concern below.

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:1087: status =
SecCertificateGetData(intermediate_ca_certs_[i], &cert_data);
On 2011/10/28 23:45:47, agl wrote:
> I note in the OS X documentation that this is listed as deprecated as of 10.7.
I
> don't think that it's important right now.

I would go as far as to say that > 80% of the certificate APIs we use are
deprecated on 10.7, many with no suitable equivalents. Which is very
unfortunate.

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:1089: return sha1;
BUG: |sha1_ctx| is improperly cleaned up here.

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate_op...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate_op...
net/base/x509_certificate_openssl.cc:373: return sha1;
BUG: sha1_ctx is leaked/improperly cleaned up here

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate_wi...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate_wi...
net/base/x509_certificate_win.cc:1023: // TODO(wtc): implement this.
On 2011/10/28 23:45:47, agl wrote:
> It's not clear how this can be a TODO if the change is to fix the bug. Will
this
> not result in the current behaviour? Are you assuming that it's not urgent to
> fix on Windows because we don't have any reports of problems on Windows? Ok if
> so.

I fear once http://codereview.chromium.org/7324039/ lands that this will become
an issue, which is why I'm holding on to it for now.

It would be good if you could implement it here on Windows, as I think it is
potentially a concern there as well. Two reasons why I think it's possible on
Windows:
1) The cert may not have AIA info, preventing CryptoAPI from network fetching
2) Because of the thread-safety concerns with cert_store(), the newly added
(from the network) cert may be skipped when the CryptoAPI verification routines
are enumerating the PCCERT_CONTEXT->hCertStore, thus not picking them up and
using them.

[wtc, 2011-10-29 01:32:03]

Please review Patch Set 3.  Thanks.

I believe I have addressed all of your review comments.

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:1089: return sha1;

On 2011/10/28 23:55:03, Ryan Sleevi wrote:
> BUG: |sha1_ctx| is improperly cleaned up here.

Thank you for pointing out this issue.

Based on how SHA-1 etc. are implemented in NSS, I know
a SHA-1 context does not need to allocate memory.  So
a missing CC_SHA1_Final call should not leak memory.

Here is the definition of the CC_SHA1_CTX structure:

typedef struct CC_SHA1state_st
{
        CC_LONG h0,h1,h2,h3,h4;        CC_LONG Nl,Nh;
        CC_LONG data[CC_SHA1_BLOCK_LONG];
        int num;
} CC_SHA1_CTX;

Note that there are no pointer members.

The only problem is if we hash sensitive info, which may
be left behind in the CC_SHA1_CTX.  That's not a concern
when we hash certificates.

OpenSSL's SHA_CTX is defined as
typedef struct SHAstate_st
        {
        SHA_LONG h0,h1,h2,h3,h4;
        SHA_LONG Nl,Nh;
        SHA_LONG data[SHA_LBLOCK];
        unsigned int num;
        } SHA_CTX;

So similarly I believe a missing SHA1_Final call should not
leak memory.

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate_wi...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/8400075/diff/1017/net/base/x509_certificate_wi...
net/base/x509_certificate_win.cc:1023: // TODO(wtc): implement this.

On 2011/10/28 23:45:47, agl wrote:
> It's not clear how this can be a TODO if the change is to fix the bug.

Sorry about the confusion.  I have implemented this function
now.

I didn't use CryptoAPI because it was considered too slow
before.  I use NSS to implement this function.

http://codereview.chromium.org/8400075/diff/5005/net/data/ssl/certificates/sa...
File net/data/ssl/certificates/salesforce_com_test.pem (right):

http://codereview.chromium.org/8400075/diff/5005/net/data/ssl/certificates/sa...
net/data/ssl/certificates/salesforce_com_test.pem:55: -----BEGIN
CERTIFICATE-----

rsleevi: do you just want the base64-encode certificate?
Or do you also want the human readable part above?

[Ryan Sleevi, 2011-10-29 02:53:15]

Thanks for clarifying.

I wonder, should a test also be added (in a follow-up, not to be merged) to the
content/browser/cert_store, which is/was also at risk of over-sharing certs?

http://codereview.chromium.org/8400075/diff/5005/net/data/ssl/certificates/sa...
File net/data/ssl/certificates/salesforce_com_test.pem (right):

http://codereview.chromium.org/8400075/diff/5005/net/data/ssl/certificates/sa...
net/data/ssl/certificates/salesforce_com_test.pem:55: -----BEGIN
CERTIFICATE-----
On 2011/10/29 01:32:04, wtc wrote:
> 
> rsleevi: do you just want the base64-encode certificate?
> Or do you also want the human readable part above?

Yeah, I think as-is is helpful, particularly on Windows where getting a
certificate viewed can be a pain (I find the default dialog may switch out
certificates with ones from the default stores).

http://codereview.chromium.org/8400075/diff/1019/net/base/x509_certificate_wi...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/8400075/diff/1019/net/base/x509_certificate_wi...
net/base/x509_certificate_win.cc:1025: SHA1Fingerprint sha1;
Perhaps document here (or on line 7), why NSS was used. It seems out of place
compared to, say, the CryptoAPI calls, such as on line 1016.

[wtc, 2011-10-29 05:08:34]

http://codereview.chromium.org/8400075/diff/1019/net/base/x509_certificate_wi...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/8400075/diff/1019/net/base/x509_certificate_wi...
net/base/x509_certificate_win.cc:1025: SHA1Fingerprint sha1;
On 2011/10/29 02:53:15, Ryan Sleevi wrote:
> Perhaps document here (or on line 7), why NSS was used. It seems out of place
> compared to, say, the CryptoAPI calls, such as on line 1016.

Done.

In Patch Set 5 I also switched to the NSS low-level hash
functions to ensure this is fast.  The high-level hash
functions require NSS to be initialized, so I'd also need a
crypto::EnsureNSSInit() call.

[commit-bot: I haz the power, 2011-10-29 22:40:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/8400075/23

[commit-bot: I haz the power, 2011-10-29 23:45:41]

Change committed as 107888