Initial support for HSTS certificate locking. This isn't a finished work, but

Initial support for HSTS certificate locking. This isn't a finished work, but
should serve as a nice base to iterate on top of. In particular, this is only
exposed via the chrome://net-internals/#hsts UI and _not_ via the HTTP header.

Aside from the core tweaks, there's also:
- Enhance GTK dialog so that fingerprints for the root and intermediaries can
easily be seen (the Windows system dialog is already OK here).
- Moved some code for chain navigation into net/base to avoid breaching layerz.

BUG=78369
TEST=TransportSecurityStateTest.CertLocks

[Chris Evans, 2011-04-04 22:35:32]

Seems like a reasonably-sized chunk to start with :)

[abarth-chromium, 2011-04-04 22:49:36]

Here are some random comments.  The parts of the code that I know look
reasonable, but you're touching a bunch of areas (e.g., GTK, net-internals HTML)
that I'm not qualified to review.

http://codereview.chromium.org/6793026/diff/3007/chrome/browser/resources/net...
File chrome/browser/resources/net_internals/hstsview.js (right):

http://codereview.chromium.org/6793026/diff/3007/chrome/browser/resources/net...
chrome/browser/resources/net_internals/hstsview.js:16: deleteInputId,
deleteFormId) {
Merge these lines?

http://codereview.chromium.org/6793026/diff/3007/net/base/transport_security_...
File net/base/transport_security_state.cc (right):

http://codereview.chromium.org/6793026/diff/3007/net/base/transport_security_...
net/base/transport_security_state.cc:157: hash += StringPrintf("%02X",
fingerprint.data[i]);
This code looks pretty low-level.  Why can't SHA1Fingerprint do this work for
us?

http://codereview.chromium.org/6793026/diff/3007/net/base/transport_security_...
net/base/transport_security_state.cc:362: state->Set("cert_locks", locks);
Again, pin is probably a better term.

http://codereview.chromium.org/6793026/diff/3007/net/base/transport_security_...
File net/base/transport_security_state.h (right):

http://codereview.chromium.org/6793026/diff/3007/net/base/transport_security_...
net/base/transport_security_state.h:60: std::vector<std::string> cert_locks;  //
optional: fingerprint locking
People seem to like the name "pinning" better than locking.

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

http://codereview.chromium.org/6793026/diff/3007/net/base/x509_certificate.h#...
net/base/x509_certificate.h:332: static void DestroyCertChain(OSCertHandles*
cert_handles);
We should probably have a stack-allocated, scoped object to manage the lifetime
of these objects.

[agl, 2011-04-04 23:46:42]

1) you're hashing only the leaf certificate, which is probably not what sites
want to do. They'll either want to hash the last public key (which survives a
certificate rollover) or they'll want to pin to a CA certificate, in which case
you have to hash the public key.

2) You're checking the fingerprint at the URLRequest level. This can go wrong
because we can open a socket too foo.example.com (which gives a cert for
*.example.com) and then have a request for bar.example.com. If bar shares an IP
address with foo, we'll reuse the socket. However, it might be the case that
foo.example.com has a different, SNI triggered, certificate. Thus the requests
will fail in a hard to diagnose manner.

3) Upper case hex strings: bad for readability if it's getting exposed in the
HSTS UI.

I'm also not sure about whether your X509 certificate API is correct. You might
find that it's better to add a function IsSPKIHashInChain rather than GetChain
with the OS X and Windows APIs.

http://codereview.chromium.org/6793026/diff/3007/chrome/browser/resources/net...
File chrome/browser/resources/net_internals/index.html (right):

http://codereview.chromium.org/6793026/diff/3007/chrome/browser/resources/net...
chrome/browser/resources/net_internals/index.html:390: Include subdomains:
<input type="checkbox" id=hstsCheckInput />
Add a <br/> at the end of this line?

[agl, 2011-04-06 01:29:08]

On Tue, Apr 5, 2011 at 9:07 PM, Chris Evans <cevans@google.com> wrote:
> Can we resolve this one later? Seems like agl@'s review has exposed gremlins
> in the per-platform certificate chain handling.

I can do the plumbing here if you like, by the way. The problem is
that wtc is on vacation soon and he's the obvious reviewer.


Cheers

AGL

[cevans, 2011-04-06 01:36:35]

On Mon, Apr 4, 2011 at 3:49 PM, <abarth@chromium.org> wrote:

> Here are some random comments.  The parts of the code that I know look
> reasonable, but you're touching a bunch of areas (e.g., GTK, net-internals
> HTML)
> that I'm not qualified to review.
>
>
>
>
http://codereview.chromium.org/6793026/diff/3007/chrome/browser/resources/net...
> File chrome/browser/resources/net_internals/hstsview.js (right):
>
>
>
http://codereview.chromium.org/6793026/diff/3007/chrome/browser/resources/net...
> chrome/browser/resources/net_internals/hstsview.js:16: deleteInputId,
> deleteFormId) {
> Merge these lines?
>

Done.


>
>
http://codereview.chromium.org/6793026/diff/3007/net/base/transport_security_...
> File net/base/transport_security_state.cc (right):
>
>
>
http://codereview.chromium.org/6793026/diff/3007/net/base/transport_security_...
> net/base/transport_security_state.cc:157: hash += StringPrintf("%02X",
> fingerprint.data[i]);
> This code looks pretty low-level.  Why can't SHA1Fingerprint do this
> work for us?
>

Agreed, and done.


>
>
http://codereview.chromium.org/6793026/diff/3007/net/base/transport_security_...
> net/base/transport_security_state.cc:362: state->Set("cert_locks",
> locks);
> Again, pin is probably a better term.
>

Done.


>
>
>
http://codereview.chromium.org/6793026/diff/3007/net/base/transport_security_...
> File net/base/transport_security_state.h (right):
>
>
>
http://codereview.chromium.org/6793026/diff/3007/net/base/transport_security_...
> net/base/transport_security_state.h:60: std::vector<std::string>
> cert_locks;  // optional: fingerprint locking
> People seem to like the name "pinning" better than locking.
>

Done.


>
> http://codereview.chromium.org/6793026/diff/3007/net/base/x509_certificate.h
> File net/base/x509_certificate.h (right):
>
>
>
http://codereview.chromium.org/6793026/diff/3007/net/base/x509_certificate.h#...
> net/base/x509_certificate.h:332: static void
> DestroyCertChain(OSCertHandles* cert_handles);
> We should probably have a stack-allocated, scoped object to manage the
> lifetime of these objects.


Can we resolve this one later? Seems like agl@'s review has exposed gremlins
in the per-platform certificate chain handling.


Cheers
Chris


>
>
> http://codereview.chromium.org/6793026/
>

[cevans, 2011-04-06 03:48:29]

On Mon, Apr 4, 2011 at 4:46 PM, <agl@chromium.org> wrote:

> 1) you're hashing only the leaf certificate, which is probably not what
> sites
> want to do.


The fingerprint match is actually permitted at any level of a validated
chain -- leaf, intermediate or root. This gives flexibility, which is a
goal. We do also see specific use-cases for matching at the leaf level (such
as permitting self-signed certs, leading to a SSH-like trust model and
setup, but that's definitely for a much, much later discussion and
changelist).

They'll either want to hash the last public key (which survives a
> certificate rollover) or they'll want to pin to a CA certificate, in which
> case
> you have to hash the public key.
>

Ah yes, I do promise I was paying attention in that other thread about what
different fields we might want to match on. I added a big TODO. I do want to
keep the match of full fingerprint even as we add other match options later.


> 2) You're checking the fingerprint at the URLRequest level. This can go
> wrong
> because we can open a socket too foo.example.com (which gives a cert for
> *.example.com) and then have a request for bar.example.com. If bar shares
> an IP
> address with foo, we'll reuse the socket.


Just so I'm clear -- the sharing condition is a matching IP address and a
hostname which matches the certificate wildcard associated with the existing
SSL socket?
And there's no new SSL handshake?

However, it might be the case that
> foo.example.com has a different, SNI triggered, certificate. Thus the
> requests
> will fail in a hard to diagnose manner.
>

I think I see. We might apply the foo.example.com "pin" to the
*.example.comcert. Let me look into it.

Actually, doesn't this mean that a site with the above set up would already
have a bug: we would show the wrong cert for bar.example.com in the cert
viewer UI, or possibly even fail to show the EV indicator due to not
handshaking with the intended EV certificate?


> 3) Upper case hex strings: bad for readability if it's getting exposed in
> the
> HSTS UI.
>

Fixed.


> I'm also not sure about whether your X509 certificate API is correct. You
> might
> find that it's better to add a function IsSPKIHashInChain rather than
> GetChain
> with the OS X and Windows APIs.
>

Looks like you're right. I haven't checked, but I think this would have
become very apparent due to compile failure on Win, Mac trybots when I got
that far.
The "generic" code I moved from chrome/common/net to net/base looks like it
might have only been called on Linux. I thought it was libnss all the way
down but obviously not :)


>
>
>
http://codereview.chromium.org/6793026/diff/3007/chrome/browser/resources/net...
> File chrome/browser/resources/net_internals/index.html (right):
>
>
>
http://codereview.chromium.org/6793026/diff/3007/chrome/browser/resources/net...
> chrome/browser/resources/net_internals/index.html:390: Include
> subdomains: <input type="checkbox" id=hstsCheckInput />
> Add a <br/> at the end of this line?


Fixed.

Cheers
Chris


>
>
> http://codereview.chromium.org/6793026/
>

[cevans, 2011-04-06 03:51:06]

On Tue, Apr 5, 2011 at 6:13 PM, Adam Langley <agl@chromium.org> wrote:

> On Tue, Apr 5, 2011 at 9:07 PM, Chris Evans <cevans@google.com> wrote:
> > Can we resolve this one later? Seems like agl@'s review has exposed
> gremlins
> > in the per-platform certificate chain handling.
>
> I can do the plumbing here if you like, by the way.


I can't refuse such a generous offer. I'm hopeful we can land a cert-pinning
prototype for people to experiment with before the M12 branch point.

I uploaded my latest changes to address some of the review comments from
yourself and abarth@.


Cheers
Chris


> The problem is
> that wtc is on vacation soon and he's the obvious reviewer.
>
>
> Cheers
>
> AGL
>

[wtc, 2011-04-07 01:00:29]

I reviewed the files in 'net'.

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

http://codereview.chromium.org/6793026/diff/9002/net/base/x509_certificate.h#...
net/base/x509_certificate.h:329: OSCertHandles* cert_handles);
Please use the X509Certificate and CertificateList types, if
possible.

http://codereview.chromium.org/6793026/diff/9002/net/base/x509_certificate_ns...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/6793026/diff/9002/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:955: CERT_GetCertChainFromCert(cert_handle,
PR_Now(), certUsageSSLServer);
The certificate chain is readily available in the Verify()
method.  It is best to get the certificate chain in Verify().

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

http://codereview.chromium.org/6793026/diff/9002/net/base/x509_certificate_op...
net/base/x509_certificate_openssl.cc:338: // TODO(bulach): how to get the chain
out of a certificate?
I don't know.  Usually a certificate chain is a byproduct
of certificate verification.  But my quick search on this
topic didn't find anything.  Sorry.

[cevans, 2011-04-07 01:05:44]

Thanks for the review.
All of these changes seem to relate to the move of existing certificate code
from chrome/common/net/ to net/base/
I moved that code because I didn't see an API to get an array of objects
representing the certificate chain. That move has other problems that Adam
has volunteered to help with.

I'm not sure what you mean by "It is best to get the certificate chain in
Verify()." -- are you saying that all certificate chain iteration should be
performed within the Verify() call? If so, we may need to define some new
abstractions because we likely shouldn't have the certificate code know
anything about our higher-level concept of Strict Transport Security and
certificate pinning?

On Wed, Apr 6, 2011 at 6:00 PM, <wtc@chromium.org> wrote:

> I reviewed the files in 'net'.
>
>
>
> http://codereview.chromium.org/6793026/diff/9002/net/base/x509_certificate.h
> File net/base/x509_certificate.h (right):
>
>
>
http://codereview.chromium.org/6793026/diff/9002/net/base/x509_certificate.h#...
> net/base/x509_certificate.h:329: OSCertHandles* cert_handles);
> Please use the X509Certificate and CertificateList types, if
> possible.
>
>
>
http://codereview.chromium.org/6793026/diff/9002/net/base/x509_certificate_ns...
> File net/base/x509_certificate_nss.cc (right):
>
>
>
http://codereview.chromium.org/6793026/diff/9002/net/base/x509_certificate_ns...
> net/base/x509_certificate_nss.cc:955:
> CERT_GetCertChainFromCert(cert_handle, PR_Now(), certUsageSSLServer);
> The certificate chain is readily available in the Verify()
> method.  It is best to get the certificate chain in Verify().
>
>
>
http://codereview.chromium.org/6793026/diff/9002/net/base/x509_certificate_op...
> File net/base/x509_certificate_openssl.cc (right):
>
>
>
http://codereview.chromium.org/6793026/diff/9002/net/base/x509_certificate_op...
> net/base/x509_certificate_openssl.cc:338: // TODO(bulach): how to get
> the chain out of a certificate?
> I don't know.  Usually a certificate chain is a byproduct
> of certificate verification.  But my quick search on this
> topic didn't find anything.  Sorry.
>
>
> http://codereview.chromium.org/6793026/
>

[wtc, 2011-04-08 03:39:59]

On 2011/04/07 01:05:44, cevans wrote:
>
> I'm not sure what you mean by "It is best to get the certificate chain in
> Verify()." -- are you saying that all certificate chain iteration should be
> performed within the Verify() call?

Sorry I wasn't clear.

What I meant is that the X509Certificate::Verify() method is the
best place to get the certificate chain from, because Verify()
already has the certificate chain (except for OpenSSL).  It just
doesn't return the certificate chain in CertVerifyResult.

So one can imagine adding the certificate chain to the
CertVerifyResult structure, and change Verify() to return the
certificate chain, perhaps controlled by a flag, in CertVerifyResult.

Then, the caller of Verify() can iterate over the certificate
chain at the right moment.  We don't need to have Verify() do that.

[wtc, 2011-04-08 03:43:33]

One more thing:

SSL servers are supposed to send us their certificate chains in the
SSL Certificate message, so we don't need to build the certificate
chains.  This doesn't work in practice for two reasons:

1. Some servers are misconfigured and only send the leaf certificates.

2. Some servers send extra "cross certificates" to allow the browser
to chain up to multiple roots.  I am not sure if your scheme can deal
with extra certificates.

[Ryan Sleevi, 2011-04-09 03:41:52]

On 2011/04/08 03:43:33, wtc wrote:
> One more thing:
> 
> SSL servers are supposed to send us their certificate chains in the
> SSL Certificate message, so we don't need to build the certificate
> chains.  This doesn't work in practice for two reasons:
> 
> 1. Some servers are misconfigured and only send the leaf certificates.
> 
> 2. Some servers send extra "cross certificates" to allow the browser
> to chain up to multiple roots.  I am not sure if your scheme can deal
> with extra certificates.

wtc: Have you seen servers that send additional certificates beyond the chain?
My experience with cross-certification (eg: Federal Bridge PKI, Verisign), one
of two options is used, depending on whether or not the platforms of the clients
are known. I can also think of several embedded devices and libraries deployed
that will treat extra certs beyond the root as a fatal, non-recoverable error,
so I was surprised you mentioned it.

For a cross-certified CA path EE->A->B and E->A->B'->C, where B is the
self-signed root certificate and B' is the cross-certified version (same
subject+key as A's issuer, but signed by C and possibly with name constraints),
I've seen the following:

EE->A (client chain building is left to fill in either B or B'->C)
EE->A->B (clients which only trust C may have trouble, depending on their RFC
3280/5280 compliance. This is generally a 'bad' configuration)
EE->A->B'[->C] (server supplies B' for clients which may only have C. Clients
which have C and trust C can build a chain to it. Clients which only trust B are
expected to substitute B for B' during chain building, so that they build
EE->A->B as the trust)

Beyond the FBPKI, I also see this with Verisign certificates. There are two
versions of their EV root certificate - one self-signed, one signed by their
(old) root. Customers are encouraged to go EE->A->EV', as the modern clients
that support EV also support substituting EV' with the self-signed EV in their
trust stores.

The effect of mentioning all of this is because returning the chain from
X509Certificate::Verify() is something I had already implemented, so that the
chain-as-verified is the one that could be displayed to the user, rather than
the chain-as-sent. This would also allow for certificates fetched via AIA to be
included in the results consistently.

Because the chain returned to Verify() may differ (and significantly) from the
chain in the X509Certificate passed in, the codereview was contingent on two
others ( http://codereview.chromium.org/4645001/ - cache the entire chain in the
response cache - and http://codereview.chromium.org/2944008/ - change
X509Certificate to cache by OS handle, not by chain). The latter is relevant
here because, presently, X509Certificate cannot safely be used to transport a
unique chain of certificates, since two X509Certificates with different chains
will result in the longer chain clobbering the shorter one and the same
X509Certificate being returned.

The original CL (for returning from X509Certificate::Verify()) is at
http://codereview.chromium.org/3146034/ - but because of the above two bugs,
hasn't landed.

The other potential area of concern is how different platforms perform chain
building. Windows builds multiple paths in parallel - meaning it may know both
about EE->A->B and EE->A->B'->C. It then applies a series of scoring rules to
determine the "best" chain. As well, it can optionally provide all of the chains
built (either just the high-scoring, or optionally including the low scoring as
well). 

NSS also supports exploring multiple paths during trust evaluation (when using
libpkix, as we are), except it's results only include the path it verified, and
it does not apply any heuristics for scoring. In the Verisign EV case above, if
NSS knows about both chains, it may return different results depending on
whether or not EV OIDs were passed in as a constraint if libpkix was specified,
or an entirely different chain if libpkix was used for verify but pre-libpkix is
used to get the path back. 

For Mac, it's chain building is not even at an RFC 3280 level, and bridge PKI is
a big weakness ( see http://crbug.com/69278 for a bug that would potentially
mitigate some of it )

agl, cevans: While unfortunately the above CLs may not be reviewable in time for
M12 branch, doubly so with wtc gone on vacation, they may provide a ready-made
implementation of some of the plumbing, or at least provide a warning for paths
which I explored (returning a new X509Certificate with a new chain, for example)
which will not presently work.

[cevans, 2011-04-11 21:57:16]

On Tue, Apr 5, 2011 at 8:48 PM, Chris Evans <cevans@google.com> wrote:

> On Mon, Apr 4, 2011 at 4:46 PM, <agl@chromium.org> wrote:
>
>> 1) you're hashing only the leaf certificate, which is probably not what
>> sites
>> want to do.
>
>
> The fingerprint match is actually permitted at any level of a validated
> chain -- leaf, intermediate or root. This gives flexibility, which is a
> goal. We do also see specific use-cases for matching at the leaf level (such
> as permitting self-signed certs, leading to a SSH-like trust model and
> setup, but that's definitely for a much, much later discussion and
> changelist).
>
> They'll either want to hash the last public key (which survives a
>> certificate rollover) or they'll want to pin to a CA certificate, in which
>> case
>> you have to hash the public key.
>>
>
> Ah yes, I do promise I was paying attention in that other thread about what
> different fields we might want to match on. I added a big TODO. I do want to
> keep the match of full fingerprint even as we add other match options later.
>
>
>> 2) You're checking the fingerprint at the URLRequest level. This can go
>> wrong
>> because we can open a socket too foo.example.com (which gives a cert for
>> *.example.com) and then have a request for bar.example.com. If bar shares
>> an IP
>> address with foo, we'll reuse the socket.
>
>
> Just so I'm clear -- the sharing condition is a matching IP address and a
> hostname which matches the certificate wildcard associated with the existing
> SSL socket?
> And there's no new SSL handshake?
>
> However, it might be the case that
>> foo.example.com has a different, SNI triggered, certificate. Thus the
>> requests
>> will fail in a hard to diagnose manner.
>>
>
> I think I see. We might apply the foo.example.com "pin" to the *.
> example.com cert. Let me look into it.
>
> Actually, doesn't this mean that a site with the above set up would already
> have a bug: we would show the wrong cert for bar.example.com in the cert
> viewer UI, or possibly even fail to show the EV indicator due to not
> handshaking with the intended EV certificate?
>

One final question here -- isn't there also a largely theoretical attack on
the bar.example.com session, if bar.example.com used a stronger certificate
than *.example.com ?

I'm not worried about it, just trying to see if my understanding is correct.


Cheers
Chris


>
>
>> 3) Upper case hex strings: bad for readability if it's getting exposed in
>> the
>> HSTS UI.
>>
>
> Fixed.
>
>
>> I'm also not sure about whether your X509 certificate API is correct. You
>> might
>> find that it's better to add a function IsSPKIHashInChain rather than
>> GetChain
>> with the OS X and Windows APIs.
>>
>
> Looks like you're right. I haven't checked, but I think this would have
> become very apparent due to compile failure on Win, Mac trybots when I got
> that far.
> The "generic" code I moved from chrome/common/net to net/base looks like it
> might have only been called on Linux. I thought it was libnss all the way
> down but obviously not :)
>
>
>>
>>
>>
http://codereview.chromium.org/6793026/diff/3007/chrome/browser/resources/net...
>> File chrome/browser/resources/net_internals/index.html (right):
>>
>>
>>
http://codereview.chromium.org/6793026/diff/3007/chrome/browser/resources/net...
>> chrome/browser/resources/net_internals/index.html:390: Include
>> subdomains: <input type="checkbox" id=hstsCheckInput />
>> Add a <br/> at the end of this line?
>
>
> Fixed.
>
> Cheers
> Chris
>
>
>>
>>
>> http://codereview.chromium.org/6793026/
>>
>
>

[agl, 2011-04-12 15:34:15]

On Mon, Apr 11, 2011 at 5:57 PM, Chris Evans <cevans@google.com> wrote:
> One final question here -- isn't there also a largely theoretical attack on
> the bar.example.com session, if bar.example.com used a stronger certificate
> than *.example.com ?
> I'm not worried about it, just trying to see if my understanding is correct.

If there's a weak certificate for *.example.com, and an attacker can
crack it, then they can MITM connections to bar.example.com no matter
what. The only difference might be if bar.example.com did forward
security and *.example.com didn't.


AGL

[wtc, 2011-04-20 17:24:25]

On 2011/04/09 03:41:52, Ryan Sleevi wrote:
> 
> wtc: Have you seen servers that send additional certificates beyond the chain?

One example is the VeriSign EV cert chain EE->A->EV (where 'EV'
is a cert of the new EV root cross-signed by the old root) that
you mentioned.

Another example is described in
http://code.google.com/p/chromium/issues/detail?id=43538#c3

The reason servers can get away with these cert chains is that
most certificate libraries throw the certs provided in the server's
Certificate message into their pool of certs and rebuild the cert
chain.