Return the constructed certificate chain in X509Certificate::Verify()

Return the constructed certificate chain in X509Certificate::Verify()

BUG=65540
TEST=net_unittests --gtest_filter=X509CertificateTest.VerifyReturn*


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

[Ryan Sleevi, 2011-04-17 23:35:18]

agl: Now that a unique X509Certificate can be returned with the same end-entity
but a unique chain (at least, once http://codereview.chromium.org/2944008/
lands), X509Certificate::Verify() can be used to return the chain-as-verified,
rather than the chain-as-input.

This should also make it easier to decouple some of the browser-side policy
decisions (HSTS pinning, CAA, etc) from the PKIX-side of
X509Certificate::Verify() if desired, and as mentioned in
http://codereview.chromium.org/6793026

Because the OSCertHandles themselves are ref-counted by the various underlying
libraries, and by the new X509Certificate cache, the only overhead incurred from
always returning the chain should just be that the subject/issuer names and
validity dates of the end-entity certificate are parsed two additional times.
First, at the beginning of Verify() when the input chain is cloned in case of
processing error, and second, at the end of Verify(), when the returned chain is
cloned. Overall, this should be very minimal processing and memory overhead.

This plumbs the returned chain from Verify() through to the SSLInfo. This
/should/ cause the chain to surface to the UI, since the SSLInfo ultimately
feeds the UI, but that part isn't being tested yet as part of this CL. It's
still possible that the original/input chain is displayed - I haven't
aggressively tested the permutations yet, or written the UI unit tests.

SSLHostInfo was left alone - it continues to contain the chain as sent by the
server, rather than the chain as verified. However, the HttpResponseInfo, and
ergo the HttpCache, will contain the full chain as verified. This is consistent
with the fact that the certificate status reflects the status at the time it was
persisted, rather than the status at the time it was viewed, and will allow the
full chain to be displayed in the UI even if the user removes/uninstalls one or
more intermediates or roots. Also, this means a further increase in HttpCache
size, of a likely 1-3K per cached SSL resource, due to the additional/root
certificates being stored that might otherwise be available in the system
certificate stores.

wtc, cevans: FYI because of the related CL and comments.

[agl, 2011-04-18 13:50:27]

LGTM (I'll be unavailable for most of the rest of the day.)

Note that SSLHostInfo should contain the chain as sent by the server, so we
shouldn't try and update it in light of this change.

As a nit: the first two added tests contained very similar code. That's somewhat
endemic in test code, but I suspect that they could call a common function that
takes a single bool argument. But don't worry about it if that doesn't work for
some reason that I haven't thought of.

http://codereview.chromium.org/6874039/diff/6004/net/base/cert_verify_result.cc
File net/base/cert_verify_result.cc (right):

http://codereview.chromium.org/6874039/diff/6004/net/base/cert_verify_result....
net/base/cert_verify_result.cc:6: 
No need for this blank line.

http://codereview.chromium.org/6874039/diff/6004/net/base/cert_verify_result.h
File net/base/cert_verify_result.h (right):

http://codereview.chromium.org/6874039/diff/6004/net/base/cert_verify_result....
net/base/cert_verify_result.h:29: // be substantially different.
I think it's worth noting that, in the event of a verification failure, this
will contain the chain as supplied by the server.

http://codereview.chromium.org/6874039/diff/6004/net/data/ssl/certificates/x5...
File net/data/ssl/certificates/x509_verify_results.chain.pem (right):

http://codereview.chromium.org/6874039/diff/6004/net/data/ssl/certificates/x5...
net/data/ssl/certificates/x509_verify_results.chain.pem:51: 
nuke blank line.

[wtc, 2011-04-20 23:39:29]

rsleevi: does this mean you can mark
http://codereview.chromium.org/3146034/show
obsolete now?

http://codereview.chromium.org/6874039/diff/6004/net/base/cert_verify_result.h
File net/base/cert_verify_result.h (right):

http://codereview.chromium.org/6874039/diff/6004/net/base/cert_verify_result....
net/base/cert_verify_result.h:30: scoped_refptr<X509Certificate> verified_cert;
It's better to return a CertificateList to avoid the
confusion.

[Ryan Sleevi, 2011-04-20 23:44:34]

Yes, I've marked that CL obsolete now.

http://codereview.chromium.org/6874039/diff/6004/net/base/cert_verify_result.h
File net/base/cert_verify_result.h (right):

http://codereview.chromium.org/6874039/diff/6004/net/base/cert_verify_result....
net/base/cert_verify_result.h:30: scoped_refptr<X509Certificate> verified_cert;
On 2011/04/20 23:39:30, wtc wrote:
> It's better to return a CertificateList to avoid the
> confusion.

As implemented, this means that the subject, issuer, and validity dates for
every intermediate and root must be parsed, even though they are never accessed
by Chromium on Windows/Mac, and the parsed results are ignored in the Linux/CrOS
certificate viewer in favour of accessing NSS directly.

This is in line with the comments on http://codereview.chromium.org/2944008/ ,
so I'll defer any changes until we discuss the issues raised there.

[Ryan Sleevi, 2011-07-23 00:19:49]

agl, wtc: Since it's been 3 months since this was reviewed, I felt it better to
re-ping and ensure the LGTM stands and hasn't changed.

[agl, 2011-07-25 14:11:56]

I had nits that haven't been updated but, if it merges without huge changes then
a LGTM still stands.

[Ryan Sleevi, 2011-07-25 23:51:19]

sergeyu: In looking at your http://crrev.com/93153, I get the feeling this may
break one or more remoting/sandboxed tests.

My thought was to change it such that verified_cert can be NULL if run in the
sandbox. The alternative, switching to std::vector<std::string>, strikes me as
something that may end up to be too memory intensive to be tenable.

This is not an optional flag on the Verify() call because this information (the
verified chain) is information that should be accessible for every call to
Verify().

Is there a way to test this without access to Google-internal sources,
preferably if possible to write a unit test?

[wtc, 2011-07-26 00:16:35]

rsleevi: please make the following suggested changes.  Thanks.

Some of the suggested changes apply to all implementations
(_nss, _mac, _openssl, and _win).

http://codereview.chromium.org/6874039/diff/6004/net/base/cert_verify_result.h
File net/base/cert_verify_result.h (right):

http://codereview.chromium.org/6874039/diff/6004/net/base/cert_verify_result....
net/base/cert_verify_result.h:26: // The certificate and chain that were
constructed during verification.
Nit: were => was
because only the chain was constructed.

http://codereview.chromium.org/6874039/diff/6004/net/base/cert_verify_result....
net/base/cert_verify_result.h:34: // |verified_certificate|, rather than the
originally supplied certificate
Typo: verified_certificate => verified_cert

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

http://codereview.chromium.org/6874039/diff/6004/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:804: CreateFromHandle(cert_handle_,
GetIntermediateCertificates());
Use intermediate_ca_certs_ instead of GetIntermediateCertificates().

http://codereview.chromium.org/6874039/diff/6004/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:904: std::vector<SecCertificateRef>
verified_chain;
Nit: it is not obvious that verified_chain does not contain
verified_cert.

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

http://codereview.chromium.org/6874039/diff/6004/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:180: return;
Why do you want to allow cert_list to be NULL?
This is not necessary if you don't move the GetCertChainInfo
call.

Have you verified that cvout[cvout_cert_list_index].value.pointer.chain
can be used when PKIXVerifyCert fails?  It is uncommon
for a function to return outputs on failure, so I'm not
sure if PKIXVerifyCert does that.

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

http://codereview.chromium.org/6874039/diff/6004/net/base/x509_certificate_wi...
net/base/x509_certificate_win.cc:299: return;
Why don't you test chain_context->cChain == 0 here?
Can cChain be negative?

http://codereview.chromium.org/6874039/diff/6004/net/data/ssl/certificates/RE...
File net/data/ssl/certificates/README (right):

http://codereview.chromium.org/6874039/diff/6004/net/data/ssl/certificates/RE...
net/data/ssl/certificates/README:44: the correct ordered, filtered certificate
chain is returned during
Nit: correct => correctly

http://codereview.chromium.org/6874039/diff/6004/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_mac.cc (left):

http://codereview.chromium.org/6874039/diff/6004/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_mac.cc:705: if (!server_cert_) {
Does this mean we can set server_cert_ to NULL as soon as
we have verified it?

[Sergey Ulanov, 2011-07-26 00:41:18]

Unfortunately currently we don't have any tests that test SSL code in sandbox.

Looking at your change, I don't think it can break anything - we don't call
GetSSLInfo() in sandbox.

On 2011/07/25 23:51:19, Ryan Sleevi wrote:
> sergeyu: In looking at your http://crrev.com/93153, I get the feeling this may
> break one or more remoting/sandboxed tests.
> 
> My thought was to change it such that verified_cert can be NULL if run in the
> sandbox. The alternative, switching to std::vector<std::string>, strikes me as
> something that may end up to be too memory intensive to be tenable.
> 
> This is not an optional flag on the Verify() call because this information
(the
> verified chain) is information that should be accessible for every call to
> Verify().
> 
> Is there a way to test this without access to Google-internal sources,
> preferably if possible to write a unit test?

[Ryan Sleevi, 2011-07-26 00:44:15]

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

http://codereview.chromium.org/6874039/diff/6004/net/base/x509_certificate_wi...
net/base/x509_certificate_win.cc:299: return;
On 2011/07/26 00:16:35, wtc wrote:
> Why don't you test chain_context->cChain == 0 here?
> Can cChain be negative?

No, it's a DWORD, so it's unsigned. Updated to be explicit about the check for
== 0. And it's a check for pathologically bad/broken crypto providers, rather
than potentially dereferencing an invalid address.

http://codereview.chromium.org/6874039/diff/6004/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_mac.cc (left):

http://codereview.chromium.org/6874039/diff/6004/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_mac.cc:705: if (!server_cert_) {
On 2011/07/26 00:16:35, wtc wrote:
> Does this mean we can set server_cert_ to NULL as soon as
> we have verified it?

Not without further updates. The SSLClientSocket* currently compare
|server_cert_| with the new/incoming certificate on a renegotiation, to see if
it can skip re-verification.

However, your question has highlighted what may be a bug - I'll need to go back
and look carefully at this before committing/commenting.

The bug I'm looking at is at line 1245. We end up not updating |server_cert_| on
a renegotiation handshake if the end-entity certificates are the same. This
prevents a server from being able to change it's intermediates on the fly during
a renegotiation. I'm not sure how important this is, but it is supported by
Apache, so I believe it may be a bug.

If were to fix this bug to compare the entire old and new chains, then I don't
think we could safely swap using |->verified_cert| for |server_cert_| in that
check. This is because the |->verified_cert| chain may differ greatly from the
|server_cert_| chain, which would cause the chain mismatches to (potentially)
force a reverification.

I'm also thinking that perhaps SSLHostInfo should be tracking both the verified
chain and the original (server) chain. The original server chain is useful for
situations such as client certificate authentication (where the intermediates
sent by the server may be necessary in order to build a successful client auth
chain - http://crbug.com/47656 , http://crbug.com/47658), while the verified
chain would be used for display purposes. That's what I need to look into
further.

[wtc, 2011-07-26 01:37:54]

http://codereview.chromium.org/6874039/diff/6004/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_mac.cc (left):

http://codereview.chromium.org/6874039/diff/6004/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_mac.cc:705: if (!server_cert_) {
On 2011/07/26 00:44:15, Ryan Sleevi wrote:
>
> I'm also thinking that perhaps SSLHostInfo should be tracking both the
verified
> chain and the original (server) chain. The original server chain is useful for
> situations such as client certificate authentication (where the intermediates
> sent by the server may be necessary in order to build a successful client auth
> chain - http://crbug.com/47656 , http://crbug.com/47658),

Those two bugs should be marked WontFix.  I remember we
discussed this issue before.  The certificates in the
server's Certificate message don't need to be used for
building the client certificate chain.  NSS uses them
to build the client certificate chain unintentionally.
It is an implementation quirk, not a feature.

NSS also uses the intermediate CA certificates sent by
server A to build the certificate chain for server B.
We should not need to emulate this behavior on Mac and
Windows.

[Ryan Sleevi, 2011-07-26 01:44:50]

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

http://codereview.chromium.org/6874039/diff/6004/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:180: return;
On 2011/07/26 00:16:35, wtc wrote:
> Why do you want to allow cert_list to be NULL?
> This is not necessary if you don't move the GetCertChainInfo
> call.
> 
> Have you verified that cvout[cvout_cert_list_index].value.pointer.chain
> can be used when PKIXVerifyCert fails?  It is uncommon
> for a function to return outputs on failure, so I'm not
> sure if PKIXVerifyCert does that.

Thanks for catching this. In digging down further into libpkix, I see that the
chain is not made available if there are any exceptions processing/validating.

This is incredibly unfortunate, because, combined with the error log not
containing any/all of the errors, it makes it nigh impossible to reliably
display the chain that was built with errors.

This should probably be fixed upstream, but like the error log, I don't think
this would be a trivial change to libpkix, based on how it builds/evaluates
chains.

For this reason, I removed the check on line 214-215, since it represents an
impossible condition. I left the check in for lines 218/219, but removed the
comment, because I believe it's reasonable to expect that NSS may change this
behaviour in the future, and a root cert may not always be available.

[Ryan Sleevi, 2011-07-26 01:57:16]

http://codereview.chromium.org/6874039/diff/6004/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_mac.cc (left):

http://codereview.chromium.org/6874039/diff/6004/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_mac.cc:705: if (!server_cert_) {
On 2011/07/26 01:37:54, wtc wrote:
> On 2011/07/26 00:44:15, Ryan Sleevi wrote:
> >
> > I'm also thinking that perhaps SSLHostInfo should be tracking both the
> verified
> > chain and the original (server) chain. The original server chain is useful
for
> > situations such as client certificate authentication (where the
intermediates
> > sent by the server may be necessary in order to build a successful client
auth
> > chain - http://crbug.com/47656 , http://crbug.com/47658),
> 
> Those two bugs should be marked WontFix.  I remember we
> discussed this issue before.  The certificates in the
> server's Certificate message don't need to be used for
> building the client certificate chain.  NSS uses them
> to build the client certificate chain unintentionally.
> It is an implementation quirk, not a feature.
> 
> NSS also uses the intermediate CA certificates sent by
> server A to build the certificate chain for server B.
> We should not need to emulate this behavior on Mac and
> Windows.

I agree that we shouldn't emulate the server cert behaviour on Mac/Windows
(although, as an aside, we are currently doing it on Windows, due the
|cert_store()|), but I'm not sure I agree with the WontFix for the client cert.
It's not just Firefox that has the client cert behaviour.

We can discuss why on those issues, to see if there is a case to be made, but
for the time being I've restored the original behaviour, which is to set
|ssl_info->cert| = |server_cert_|.

The return of the verified chain in GetSSLInfo() can come in a later CL.

[wtc, 2011-07-26 19:32:29]

LGTM.

http://codereview.chromium.org/6874039/diff/23001/net/base/cert_verify_result.cc
File net/base/cert_verify_result.cc (right):

http://codereview.chromium.org/6874039/diff/23001/net/base/cert_verify_result...
net/base/cert_verify_result.cc:6: #include "net/base/x509_certificate.h"
The blank line to separate the primary header from the rest
of the headers is shown in the examples in the Style Guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

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

http://codereview.chromium.org/6874039/diff/23001/net/base/x509_certificate.c...
net/base/x509_certificate.cc:596: CreateFromHandle(cert_handle_,
intermediate_ca_certs_);
Since X509Certificate is reference-counted, I think you
can just say
  verify_result->verified_cert = this;

It doesn't seem necessary to construct a copy of the
current X509Certificate object.

http://codereview.chromium.org/6874039/diff/23001/net/base/x509_certificate_m...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6874039/diff/23001/net/base/x509_certificate_m...
net/base/x509_certificate_mac.cc:852: std::vector<SecCertificateRef>
verified_intermediates;
Let's change this back to verified_chain, to be consistent
with the other x509_certificate_xxx.cc files.  A little
imprecision is fine by me.

http://codereview.chromium.org/6874039/diff/23001/net/base/x509_certificate_n...
File net/base/x509_certificate_nss.cc (left):

http://codereview.chromium.org/6874039/diff/23001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:179: DCHECK(cert_list);
Can you resurrect this DCHECK?

http://codereview.chromium.org/6874039/diff/23001/net/base/x509_certificate_u...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/6874039/diff/23001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:825: // that is, if any unrecoverable
errors occur, the default chain returned is
It's not clear what "unrecoverable errors" means here.  I'd
like remove the word "unrecoverable".

http://codereview.chromium.org/6874039/diff/23001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:874: TEST(X509CertificateTest,
VerifyReturnChainProperlyOrders) {
ProperlyOrders => ProperlyOrdered ?

http://codereview.chromium.org/6874039/diff/23001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:916: // or part of the certificate/chain
being verified.
Nit: remove '/'?

http://codereview.chromium.org/6874039/diff/23001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_mac.cc (left):

http://codereview.chromium.org/6874039/diff/23001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_mac.cc:726: NOTREACHED();
Did you mean to remove this NOTREACHED()?

[Ryan Sleevi, 2011-07-26 23:28:31]

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

http://codereview.chromium.org/6874039/diff/23001/net/base/x509_certificate.c...
net/base/x509_certificate.cc:596: CreateFromHandle(cert_handle_,
intermediate_ca_certs_);
On 2011/07/26 19:32:29, wtc wrote:
> Since X509Certificate is reference-counted, I think you
> can just say
>   verify_result->verified_cert = this;
> 
> It doesn't seem necessary to construct a copy of the
> current X509Certificate object.

This came up on discussion on IRC yesterday, as I was exploring doing this.
Though RefCountedThreadSafe::AddRef() and Release() are const, because this is
in a const method, I have a (const X509Certificate*) |this|.

Obviously, I can get away with it by doing const_cast<X509Certificate*>(this),
but that seemed a violation of the API, particularly with where sergeyu was
heading towards refactoring.

That said, because all of X509Certificate is initialized at ctor and there are
no non-const methods, I've added the const_cast<>, as this will save some small
amount of memory in the returned chain on error.

http://codereview.chromium.org/6874039/diff/23001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_mac.cc (left):

http://codereview.chromium.org/6874039/diff/23001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_mac.cc:726: NOTREACHED();
On 2011/07/26 19:32:29, wtc wrote:
> Did you mean to remove this NOTREACHED()?

Yes, for consistency with the other SSLClientSocket* implementations.

[commit-bot: I haz the power, 2011-07-30 08:47:49]

Change committed as 94832