Refactor X509Certificate caching to cache the OS handle, rather than the X509Certificate

Cache the underlying OS certificate handle within the X509CertificateCache, rather than caching raw X509Certificate pointers.

TEST=X509CertificateTest.Cache, X509CertificateTest.Intermediates
BUG=32623, 47648, 49377, 68448, 70216, 77374, 78038
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=92977

[Ryan Sleevi, 2010-07-11 07:14:09]

Wan-Teh, David,

   This is a precondition to cleaning up the chain building logic across the
platforms, since the list of intermediates sent by the server need to be
reliable and consistent. The current behaviour is entirely dependent upon timing
as to which object you'll get, and since the list of intermediates has meaning
(particularly on OS X), this has to be corrected first.

  Examples (# = X509Certificate pointer, letter = chain)
  1. A
  2. A-B
  3. A
  Then you'll have 3 == 2, rather than 3 == 1

  1. A-B-C
  2. A-D-E
  3. A-B-C
  Then you'll have 1 != 3

In addition, this cleans up the thread-safety for chain-related actions,
specifically on Windows. I'm still auditing the SecTrust code paths to try to
find why SecTrustEvaluate was crashing, and to see if an alternative
implementation by re-creating the SecCertificateRef from the encoded data would
be sufficient, as opposed to the verifier lock (which isn't uniformly used, if
it is necessary). On Windows, the routines that verify, display, or build chains
all access and may modify the PCCERT_CONTEXT's HCERTSTORE, which has no
guarantee of thread-safety when using the same context handle. Factor that in
with SSLClientSocketNSS's static HCERTSTORE, and it's a source of potential
problems, so now a new ephemeral store is created and the handles are duplicated
into it for any routines that may modify the store.

http://codereview.chromium.org/2944008/diff/8001/9011
File net/http/http_response_info.cc (right):

http://codereview.chromium.org/2944008/diff/8001/9011#newcode55
net/http/http_response_info.cc:55: RESPONSE_INFO_HAS_CERT_CHAIN = 1 << 17,
This was the biggest question I had - whether this was the right way to do it.

I wasn't sure how upgrades are supposed to interact with the HTTP cache. I would
rather it be implicit by RESPONSE_INFO_HAS_CERT, but because
Persist/CreateFromPickle have no way to know whether it was "pre-chain" or
"post-chain", I had to introduce this flag.

[Ryan Sleevi, 2010-07-18 01:32:23]

Wan-Teh,

  I believe this may offer an alternate solution to the race condition problem
of http://crbug.com/49377 , while offering a workable path forward for the other
bugs mentioned.

  I've combined the solution for 49377 with this CL, since the solution (caching
OS cert handles, rather than X509Certificate pointers) fundamentally breaks the
|source| argument of X509Certificate - the same way that causing it to cache by
|intermediates| did. Further, since Cacheing the OSCertHandles may create
thread-safety problems (PCCERT_CONTEXT's not being fully thread-safe, possible
verifier issues on Mac), and since this CL already directly addressed those
problems, it seemed even more appropriate to combine.

  As a recap (and not because I'm trying to get you addicted to coffee), the
race described in #49377 stemmed from one thread following a pattern of
Thread A:
Decrement
Lock
  Remove
Release

Thread B:
Lock
  Find
Release
Increment

The original CL (http://codereview.chromium.org/2836055/show ) tried to resolve
this by making Thread B be able to resurrect any dead pointers Find() returned,
if it beat Thread A to the lock.

  By changing the cache to use the OS certificates, this fundamentally changes
the pattern to:

Thread A:
Lock
  Decrement
  Remove?
Release

Thread B
Lock
  Find
  Increment
Release

  As such, everything is nice and safely guarded by the cache lock.

  The performance implications of this, compared to the in-trunk implication, is
two-fold:
  1) Certificate fingerprints are calculated against all of the certificates
during construction and destruction (as it was in Patchset 1 of this CL)
  2) Because X509Certificate* are no longer shared (just their OSCertHandle),
the subject, issuer, and validity periods are re-processed for every instance of
a certificate.

I'm not sure how concerned I should be about #2. If it is a legitimate
performance issue, I do have a solution, but I was trying to keep complexity
minimal (The solution would be storing the parsed elements along with the
certificate in the Cache::Entry, presumably as a scoped_refptr<> to some
X509Certificate::ParsedInformation structure)

Certificates are still safe to store in scoped_refptr<>s, and the performance
penalty of calculating all of the fingerprints is only felt when the last
reference is gone. It just means that if you CreateFromBytes(A), and then
CreateFromBytes(A) again, you'll get different X509Certificate*, but which both
return the same os_cert_handle().

[wtc, 2010-07-20 23:25:56]

Ryan,

Thanks a lot for writing this CL to fix several long-standing bugs!

One problem with a large CL is that it intimidates the code reviewer.
I hope you can break it into smaller pieces.  If it's too hard to do that,
don't worry about it; I'll drink more coffee.

I've only looked at the changes to x509_certificate* files, somewhat
superficially.  Here are some comments.

http://codereview.chromium.org/2944008/diff/41001/35007
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/2944008/diff/41001/35007#newcode43
net/base/x509_certificate.h:43: // An OSIdentityHandle combines the OSCertHandle
with any supplied
I don't like the OSIdentityHandle name.  Why don't you call it a "cert chain"
or "cert list"?  "Identity" is too ambiguous.

Typo on the next line: It's use => Its use

http://codereview.chromium.org/2944008/diff/41001/35007#newcode95
net/base/x509_certificate.h:95: bool has_certificate_chain);
Nit: could you add an enum type for the has_certificate_chain parameter?
This will make the code more readable (than just "true" or "false").

http://codereview.chromium.org/2944008/diff/41001/35007#newcode150
net/base/x509_certificate.h:150: OSIdentityHandle GetThreadSafeOSIdentity()
const;
Nit: since our code should be thread-safe by default, it doesn't seem
necessary for the function name to stress the fact that it's thread-safe.

http://codereview.chromium.org/2944008/diff/41001/35009
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/2944008/diff/41001/35009#newcode665
net/base/x509_certificate_nss.cc:665: // The intermediates are made available
through the cert database. If/when
This comment is wrong.  The intermediate CA certs are not in the NSS cert
database.  They are in memory only.  We don't have a name for that thing in NSS.
(It's probably called the "default trust domain" or "default crypto context",
but I'm not sure.  Nelson Bolyard calls it a "bucket" of certs.)

http://codereview.chromium.org/2944008/diff/41001/35011
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/2944008/diff/41001/35011#newcode663
net/base/x509_certificate_win.cc:663: OSIdentityHandle identity =
GetThreadSafeOSIdentity();
Are you sure we need to do this on Windows?  I've never seen this in
Microsoft's sample code.

[Ryan Sleevi, 2010-07-21 01:46:26]

Understandably, I don't wish to drop large CLs anymore than I want to read. With
this one, I was trying to be focused on a specific issue - thread-safety of
X509Certificate - though even that may not be specific enough.

If I were to break them up into smaller pieces, the effect would be something
would be broken until reassembled (verification, caching, or display, depending
on which goes first), which is why I grouped them under this one CL.

By changing the cache, it's no longer reasonable to swap a cached
X509Certificate* (with no intermediates) with a network X509Certificate* (with
intermediates), because the cache just contains individual certificates. 

This makes the source parameter useless, which makes the HTTP caching method
(caching just the EE, rather than the EE+certs) useless, since the cert will
never be replaced once deserialized. Omitting this change would break
verification/display for cached entries, since they'd be missing certificates.

This, in turn, causes problems with the verify/display code, which needs a way
to get all of the certificates to verify/display, and in a thread-safe manner.
Omitting this change invalidates the purpose of the original change, which was
thread-safety.

http://codereview.chromium.org/2944008/diff/41001/35007
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/2944008/diff/41001/35007#newcode43
net/base/x509_certificate.h:43: // An OSIdentityHandle combines the OSCertHandle
with any supplied
On 2010/07/20 23:25:56, wtc wrote:
> I don't like the OSIdentityHandle name.  Why don't you call it a "cert chain"
> or "cert list"?  "Identity" is too ambiguous.
> 
> Typo on the next line: It's use => Its use

"CertificateList" was added to the intermediates, and it implies a std::vector
of X509Certificates (in scoped_refptr). Does OSCertList still make sense then?

"OSChainHandle"/"OSCertChain" works for Mac (CFArrayRef), but on NSS it would
imply a CERTCertList/CERTCertificateList, while on Windows it would imply a
PCCERT_CHAIN_CONTEXT

"OSVerifyHandle" perhaps gets the closest, but the handle is also used by the
display routines (because, under the hood, both OS X and Windows use the verify
routines as part of the display routines).

What about OSCertBucket ;)

http://codereview.chromium.org/2944008/diff/41001/35007#newcode150
net/base/x509_certificate.h:150: OSIdentityHandle GetThreadSafeOSIdentity()
const;
On 2010/07/20 23:25:56, wtc wrote:
> Nit: since our code should be thread-safe by default, it doesn't seem
> necessary for the function name to stress the fact that it's thread-safe.

It was moreso to stress that the OS may not be - on OS X, the verifier crashes
(which may or may not be related to the cache resurrection), and on Windows -
the HCERTSTORE issue

http://codereview.chromium.org/2944008/diff/41001/35009
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/2944008/diff/41001/35009#newcode665
net/base/x509_certificate_nss.cc:665: // The intermediates are made available
through the cert database. If/when
On 2010/07/20 23:25:56, wtc wrote:
> This comment is wrong.  The intermediate CA certs are not in the NSS cert
> database.  They are in memory only.  We don't have a name for that thing in
NSS.
> (It's probably called the "default trust domain" or "default crypto context",
> but I'm not sure.  Nelson Bolyard calls it a "bucket" of certs.)

I'll update the comment accordingly - I was treating "cert database" to refer to
the structure (CERTCertDBHandle - of which only the STAN_GetDefaultTrustDomain()
is ever used, from what I could tell) rather than the on-disk representation.

http://codereview.chromium.org/2944008/diff/41001/35011
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/2944008/diff/41001/35011#newcode663
net/base/x509_certificate_win.cc:663: OSIdentityHandle identity =
GetThreadSafeOSIdentity();
On 2010/07/20 23:25:56, wtc wrote:
> Are you sure we need to do this on Windows?  I've never seen this in
> Microsoft's sample code.

A PCCERT_CONTEXT is not intrinsically thread-safe - because of it's HCERTSTORE
member - though the other members are.

There are two issues with the HCERTSTORE: Enumeration and properties.

On the properties side, it's possible to store an HCRYPTPROV with a certificate
as the CERT_KEY_PROV_HANDLE_PROP_ID (as well as the "ncrypt or legacy"
property). An HCRYPTPROV has no guarantee to be thread-safe (especially with
respect to creating/destroying containers, but enumeration can get wonky
depending on the CSP).

In addition, a cert store itself may open an HCRYPTPROV handle (for verification
purposes). While this handle /should/ be thread-safe, again, no API contract
guarantees this.

See
http://blogs.msdn.com/b/alejacma/archive/2008/06/30/threading-issues-with-cry...

This mainly comes in play because the CERT_KEY_PROV_HANDLE_PROP_ID is
checked/accessed when showing a certificate, as it informs the little icon "You
have a private key for this certificate".

On the enumeration side, CertEnumCertificatesInStore (and the related family)
have no guarantees explicitly spelled out. This is one of the black-areas of
CryptoAPI ( http://msdn.microsoft.com/en-us/library/bb204775(VS.85).aspx ). In
Wine, it most definitely is not safe (
http://source.winehq.org/source/dlls/crypt32/context.c , for example ) - if you
remove a certificate while another thread is enumerating, and that certificate
is the pPrevContext of the enumerating thread, enumeration will terminate early.

This function is the basis for the filter functions (eg:
CertFindCertificateInStore), which in turn is the basis for the chain functions
(CertGetCertificateChain). You can watch these calls with the CryptoAPI tracer
script.

The net result is that modifying the HCERTSTORE from another thread can have
side-effects on the ability of CryptoAPI to build/verify a certificate chain.

The reason it doesn't show up in the MSFT samples is you are hard-pressed to
find a multi-threaded sample which uses CryptoAPI. Even the SampleStore of the
Win 7 SDK just forwards back into CryptoAPI - hiding any threading details.
You've also got
http://us.generation-nt.com/cryptunprotectdata-failing-invalid-data-help-3099...

As it relates to .NET APIs, none of the abstractions for PCCERT_CONTEXT
(X509Certificate, X509Certificate2) or HCERTSTORE are listed as thread-safe.
Presumably, if thread-safety was implicit with the native types, it would have
been trivial to add to the .NET abstractions.

For Windows, the problem is resolved simply by creating a throw-away HCERTSTORE
and adding the certificates to it. I would have preferred to use certificate
links on Windows, to avoid the memory overhead, but that forces a tie-in back to
the original HCERTSTORE, which brings up the safety issues again.

[Ryan Sleevi, 2010-08-15 13:10:16]

I've rolled to ToT and cleaned up the areas that Wan-Teh mentioned, as well as
added more context to some of the comments, so that it will be preserved beyond
just the comments here.

I pulled out the few line change for http://codereview.chromium.org/3170019/show
so that it stands on its own, but as mentioned in the updated notes, it is a
precondition. Because of the cache changes, without that change in first, the
Windows path will return 'weird' results (eg: not what the user selected).

Wan-Teh, please let me know what you think of the altered naming
(OSCertListHandle). While I'm not thrilled with it's connotations in NSS (eg:
CERTList or CERTCertificateList, rather than the CERTCertificate it is), the
problems that necessitate the typedef don't exist on NSS, so I'm willing to
compromise. I avoided calling it OSChainHandle for two reasons. First, on
Windows, it most definitely doesn't fit (suggests PCCERT_CHAIN_CONTEXT, rather
than PCCERT_CONTEXT). The second was that it isn't necessarily a "valid" chain -
just a bundle of certificates that may be needed (possibly in addition with
others) to build and verify a chain. While admittedly, when originating from a
SSL/TLS server, it's supposed to be an ordered/hierarchal list of CAs, because
an X509Certificate is the primary transport method for certificates from other
sources (eg: x-x509-user-cert import), it's not uniformly guaranteed.

If you see any places/things you'd rather pulled out, please let me know, but I
believe this represents the absolute least number of changes that can accomplish
changing/fixing the X509Certificate caching mechanism, while not causing any
regressions in viewing/verification.

[wtc, 2010-08-17 23:45:14]

rsleevi: thanks a lot for improving this CL.

I will review it later this week.  Still swamped by code reviews...

Just wanted to tell you first that you don't need to break this CL into smaller
pieces.

[Ryan Sleevi, 2010-10-10 02:21:06]

On 2010/08/17 23:45:14, wtc wrote:
> rsleevi: thanks a lot for improving this CL.
> 
> I will review it later this week.  Still swamped by code reviews...
> 
> Just wanted to tell you first that you don't need to break this CL into
smaller
> pieces.

Wan-Teh, I've rebased to trunk, updating for the ongoing OpenSSL work. Since
this CL is directly related to the ability to save additional certificates
received as a result of <keygen> (37142), do you think you might have a chance
to review it, or suggest someone who can?

[bulach, 2010-10-20 14:50:11]

thanks Ryan!

I'm not yet too familiar with this code, so my review is mostly about style and
general nits rather than the actual architecture / implementation. it'd be great
to have wtc's final word on it.

I'll look at the other patches of the series now, but here are some comments:

http://codereview.chromium.org/2944008/diff/101002/102004
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/2944008/diff/101002/102004#newcode52
net/base/cert_database_nss_unittest.cc:52: X509Certificate::OSCertHandles()));
nit: fit on previous line?

http://codereview.chromium.org/2944008/diff/101002/102006
File net/base/ssl_config_service.cc (right):

http://codereview.chromium.org/2944008/diff/101002/102006#newcode38
net/base/ssl_config_service.cc:38: }
nit: no need for { }

http://codereview.chromium.org/2944008/diff/101002/102007
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/2944008/diff/101002/102007#newcode75
net/base/x509_certificate.cc:75: // simultaneously incrementing the reference
count within the cache.
nit: it may be obvious, but perhaps add a note saying that the key used for
lookups is the fingerprint and not |cert_handle| itself?

http://codereview.chromium.org/2944008/diff/101002/102007#newcode131
net/base/x509_certificate.cc:131: std::pair<CertMap::iterator, bool> ret_pos =
cache_.insert(cache_value);
nit: could either go for:
DCHECK(!ret_pos.second);

or remove the local ret_pos:
pos = cache_.insert(cache_value).first;

http://codereview.chromium.org/2944008/diff/101002/102007#newcode143
net/base/x509_certificate.cc:143: // replaced by a cached object.
nit: perhaps s/by a cached object/by the cached object we just found/.

http://codereview.chromium.org/2944008/diff/101002/102007#newcode148
net/base/x509_certificate.cc:148: pos->second.ref_count++;
nit: ++pos->second.ref_count;

http://codereview.chromium.org/2944008/diff/101002/102007#newcode154
net/base/x509_certificate.cc:154: // only Remove() what was obtained via
Insert().
needs some updating, perhaps:
// Remove |cert_handle| from the cache using its fingerprint as lookup key.
// The caller must have called InsertOrUpdate() prior to Remove().

http://codereview.chromium.org/2944008/diff/101002/102007#newcode171
net/base/x509_certificate.cc:171: pos->second.ref_count--;
nit: 
if (--pos->second.ref_count == 0)

http://codereview.chromium.org/2944008/diff/101002/102007#newcode173
net/base/x509_certificate.cc:173: // The last reference to |fingerprint| has
been removed, so release
nit: |cert_handle|

http://codereview.chromium.org/2944008/diff/101002/102007#newcode235
net/base/x509_certificate.cc:235: 
should the intermediates be mandatory?

if (!ok)
  return false;

http://codereview.chromium.org/2944008/diff/101002/102008
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/2944008/diff/101002/102008#newcode127
net/base/x509_certificate.h:127: SINGLE_CERTIFICATE,
nit: normally, enum values are defined as ENUMTYPENAME_VALUE,

so these would be:
PICKLETYPE_SINGLE_CERTIFICATE,
PICKLETYPE_CERTIFICATE_CHAIN,

http://codereview.chromium.org/2944008/diff/101002/102008#newcode132
net/base/x509_certificate.h:132: CERTIFICATE_CHAIN
nit: add a ,

http://codereview.chromium.org/2944008/diff/101002/102008#newcode218
net/base/x509_certificate.h:218: // each thread may obtain their own handle, and
safely use that.
nit:
// Callers are responsible to FreeOSCertListHandle() with the
// returned value.

http://codereview.chromium.org/2944008/diff/101002/102012
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/2944008/diff/101002/102012#newcode488
net/base/x509_certificate_unittest.cc:488: // intermediates are equal, the
cached certificate should be returned.
nit: see below, I think we can move the "Because.." sentence a bit further to
make it clearer.

http://codereview.chromium.org/2944008/diff/101002/102012#newcode494
net/base/x509_certificate_unittest.cc:494: 
nit: for completeness, could add:
// New handle.
EXPECT_NE(cert1.get(), cert2.get());
// But since intermediates are equal, they share the same cached certificate.
EXPECT_EQ(cert1->os_cert_handle(), cert2->os_cert_handle());

http://codereview.chromium.org/2944008/diff/101002/102012#newcode539
net/base/x509_certificate_unittest.cc:539: scoped_refptr<X509Certificate> cert2
=
nit: may be clearer with s/cert1/cert/, s/cert2/cert_from_pickle/

http://codereview.chromium.org/2944008/diff/101002/102013
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/2944008/diff/101002/102013#newcode561
net/base/x509_certificate_win.cc:561: // fail?
:)
yeah, same question up above. is there any use for impartial intermediates?
perhaps we could be 'strict' on all platforms for start, and relax if necessary
later?

http://codereview.chromium.org/2944008/diff/101002/102013#newcode568
net/base/x509_certificate_win.cc:568: // Note: This does not actually close the
store if |identity_cert| is
s/identity_cert/identity_ca_certs_/
also, rather than "is valid" perhaps "is not empty" ?
how about the cert_handle_ added up above, would that have any impact on the
store?

http://codereview.chromium.org/2944008/diff/101002/102013#newcode889
net/base/x509_certificate_win.cc:889: reinterpret_cast<const void **>(
nit: s/void **/void**/

http://codereview.chromium.org/2944008/diff/101002/102013#newcode913
net/base/x509_certificate_win.cc:913: 
nit: could add DCHECK_EQ(length, buffer.size());

http://codereview.chromium.org/2944008/diff/101002/102014
File net/http/http_response_info.cc (right):

http://codereview.chromium.org/2944008/diff/101002/102014#newcode144
net/http/http_response_info.cc:144: type = X509Certificate::CERTIFICATE_CHAIN;
nit: maybe,
X509Certificate::PickleType pickle_type =
    (flags & RESPONSE_INFO_HAS_CERT_CHAIN) ?
    X509Certificate::PICKLETYPE_CERTIFICATE_CHAIN :
    X509Certificate::PICKLETYPE_SINGLE_CERTIFICATE;

http://codereview.chromium.org/2944008/diff/101002/102016
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/2944008/diff/101002/102016#newcode395
net/socket/ssl_client_socket_nss.cc:395: X509Certificate::OSCertHandle
CreateOSCert(CERTCertificate* cert) {
nit: maybe CreateOSCertFromNSSCert?

[Ryan Sleevi, 2010-10-23 05:56:40]

@bulach:

Thanks for taking a look at this! I've updated quite a bit of the comments based
on your feedback, hopefully making things clearer. I also took another style
pass at it, as I've certainly learned more about the Style Guide in the past 3
months.

@wtc:
I added http://crbug.com/60256 to notes on this CL, as it's incidentally
resolved. Because the HTTP cache now stores the entire chain, it's available
even when the resource is cached entirely.

http://codereview.chromium.org/2944008/diff/101002/102013
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/2944008/diff/101002/102013#newcode561
net/base/x509_certificate_win.cc:561: // fail?
On 2010/10/20 14:50:11, bulach wrote:
> :)
> yeah, same question up above. is there any use for impartial intermediates?
> perhaps we could be 'strict' on all platforms for start, and relax if
necessary
> later?

For serialization/deserialization to/from the cache, I've changed it to make it
fail. However, for here, it's a bit mixed about whether it should be a /hard/
error. It generally is something that can be recovered from (due to the
prevalence of AIA information and, on Windows, automatic root cert downloading),
so if an intermediate fails to add, it likely can still be verified
successfully.

The side-effects of failure would be
  1) Possibly displaying an incomplete chain (this already can happen - see
http://crbug.com/60256 )
  2) Possibly marking a certificate as chaining to an unknown/untrusted root.

The probability of failure itself is extremely low here, and I've only seen it
happen when people have had buggy CryptoAPI plugins that have caused all sorts
of other failures (eg: flagging certificates as revoked when they aren't), which
themselves are even more rare.

I'd hate to CHECK/DCHECK here, because of the general recoverability, but does
that explanation help?

http://codereview.chromium.org/2944008/diff/101002/102013#newcode913
net/base/x509_certificate_win.cc:913: 
On 2010/10/20 14:50:11, bulach wrote:
> nit: could add DCHECK_EQ(length, buffer.size());

This was intentional, and a common pattern for the Windows code. When calling
any of the CryptoAPI functions with NULL for the data pointer, and a valid
DWORD* for the length, they will request as much as needed - OR MORE.

When you call again, this time passing a valid pointer, CryptoAPI will update
length to the size actually written. It's not necessary here to resize the
vector back down (which may reallocate), since the data is just written into the
pickle (using length, not .size()), and then discarded.

I'm not sure whether it's appropriate to comment, because it's so common (see
the second bolded note on
http://msdn.microsoft.com/en-us/library/aa387677(VS.85).aspx )

[wtc, 2010-10-25 23:56:43]

On 2010/10/23 05:56:40, rsleevi wrote:
>
> @wtc:
> I added http://crbug.com/60256 to notes on this CL, as it's incidentally
> resolved. Because the HTTP cache now stores the entire chain, it's available
> even when the resource is cached entirely.

I think it's better to split the HTTP cache storing the entire chains
into a separate CL.  Is it convenient to do that?

[Ryan Sleevi, 2010-10-26 00:02:32]

On 2010/10/25 23:56:43, wtc wrote:
> On 2010/10/23 05:56:40, rsleevi wrote:
> >
> > @wtc:
> > I added http://crbug.com/60256 to notes on this CL, as it's incidentally
> > resolved. Because the HTTP cache now stores the entire chain, it's available
> > even when the resource is cached entirely.
> 
> I think it's better to split the HTTP cache storing the entire chains
> into a separate CL.  Is it convenient to do that?

When I initially did so, when first submitting this CL, several tests broke
because they were relying on the SOURCE_FROM_NETWORK to eject the
LONE_CERT_IMPORT from the X509Certificate::Cache and the actual server
certificate replacing the cached entry.

I'll take another pass at it, and see if the trybots still complain.

[wtc, 2010-10-26 00:33:38]

On 2010/10/26 00:02:32, rsleevi wrote:
>
> When I initially did so, when first submitting this CL, several tests broke
> because they were relying on the SOURCE_FROM_NETWORK to eject the
> LONE_CERT_IMPORT from the X509Certificate::Cache and the actual server
> certificate replacing the cached entry.

I see.  Those unit tests should be removed or updated as part of
the CL that stores entire cert chains in the HTTP cache.

[Ryan Sleevi, 2011-01-13 04:32:27]

wtc: I've re-based and re-uploaded this CL, with the cache logic split out per
your request. If you're too loaded, I'd like to ask agl to see if he's available
to review in your stead.

[agl, 2011-04-11 16:50:49]

This CL seems to have died, but still seems useful.

Ryan: did you want to revive it?

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

http://codereview.chromium.org/2944008/diff/210001/net/base/x509_certificate....
net/base/x509_certificate.h:51: // X509Certificate represents an X.509
certificate, which is comprised a
s/a/of a/ (not sure about that grammar so ignore if you liked it previously.)

http://codereview.chromium.org/2944008/diff/210001/net/base/x509_certificate....
net/base/x509_certificate.h:58: // crypo library. We assume that OSCertHandle is
a pointer type on all
typo: crypto

[Ryan Sleevi, 2011-04-12 00:15:09]

agl: I've been waiting (but perhaps not nagging) on wtc's review, given that it
touches a lot of the platform specific implementation. If you're comfortable
reviewing this (and it's dependent CL, http://codereview.chromium.org/4645001/
), that would be great.

I've updated both of these CLs to the current ToT

[agl, 2011-04-12 14:54:57]

It's already been several rounds with wtc, I think this is good to go. LGTM.

[wtc, 2011-04-20 18:06:49]

On 2011/04/12 14:54:57, agl wrote:
> It's already been several rounds with wtc, I think this is good to go. LGTM.

I'd like to review this CL again.  I am not convinced that we need
to cache the OS cert handle instead of the X509Certificate object.
The incorrect reference count handling of the X509Certificate cache
can be fixed in a different way, by having the
X509Certificate::Create*() functions return scoped_refptr<X509Certificate>
instead of X509Certificate*.

[Ryan Sleevi, 2011-04-20 22:31:56]

On 2011/04/20 18:06:49, wtc wrote:
> On 2011/04/12 14:54:57, agl wrote:
> > It's already been several rounds with wtc, I think this is good to go. LGTM.
> 
> I'd like to review this CL again.  I am not convinced that we need
> to cache the OS cert handle instead of the X509Certificate object.
> The incorrect reference count handling of the X509Certificate cache
> can be fixed in a different way, by having the
> X509Certificate::Create*() functions return scoped_refptr<X509Certificate>
> instead of X509Certificate*.

wtc: I'll hold off committing until you have a chance to review. I should
mention that the the decision here is blocking work on a number of bugs, since
they are all affected by however the root issue(s) are resolved here.

I apologize for the wall of text below, but since it's been over 9 months since
we first discussed this issue and this CL was worked up, I wanted to make sure
as much of the original context (and the context since discovered) is available
when considering this CL. 



The race is within the Cache itself, not the higher-order Create* functions. An
X509Certificate* stored in the cache can be invalidated before it is removed
from the cache. This is because the X509Certificate* isn't removed from the
cache until the dtor is already being invoked and the last call to Release() has
been made. To avoid that, AddRef()/Release() would need to be changed so that
the object is first removed from the cache (thread-safe with respect to the
cache) and then decrement the ref count. If the ref count is == 0, then the dtor
should be invoked. Conversely, when coming from the cache, the ref count should
be incremented while the cache lock is still held. We explored this in a
previous CL, but the solution was lacking at the time.

However, I think there is a compelling argument for the CL as implemented,
beyond just the thread-safety/raciness of the Cache. As it stands today, an
X509Certificate represents several different unique logical entities:

1) A single certificate with basic accessors for UI purposes (subject, issuer,
validity) - eg: the infobubble, which really only wants that information to be
available in a cross-platform agnostic way.
2) A specific certificate chain sent by an SSL server - eg, ssl_client_socket_*
- containing a certificate chain and one or more issuer certificates (via
GetIntermediateCerts()), specifically chosen by an SSL peer.
3) A constructed, verified certificate chain - eg, X509Certificate::Verify()
results. It shares the same end-entity certificate as #2, but represents the
certificate chain that was actually validated for a particular purpose (EV, SSL,
client auth, CA, etc). The intermediate certificates may differ substantially
from #2.
4) A "bundle" of certificates - such as by application/x-x509-user-cert
handling. Zero or more certificate chains may be represented (that is,
GetIntermediateCerts() has no strict hierarchal relationship like in #2/#3)
5) A simple wrapper for a /specific/ native OS certificate handle- eg:
ssl_server_socket, CreateSelfSigned().

The problem with caching at the X509Certificate level is that it makes it
hard/impossible to reliably preserve the chain for the different purposes of #2,
#3, and #4, since there can be only one X509Certificate* for a given
OSCertHandle, and the chain is part of that X509Certificate*. There are
absolutely times where a server may wish to influence how #3 or #5 are
generated/returned by altering how #2 is supplied (cross-certification and EV
come to mind). Likewise, because there currently can't be any uniqueness for #2,
two unrelated SSL servers can have a direct effect on the verification results,
including EV status, depending on which was loaded first.

An alternative is to make X509Certificate* simply a wrapper for a /single/ OS
certificate handle and basic information about it (#1/#5). If any additional
certificates needed to be conveyed, they would be wrapped in their own
X509Certificate*s. All the code that expects "more than one" certificate would
be updated to use CertificateList - display code, verification code, client auth
code.

Such would be a radical refactoring of the code, and in previous discussions,
was not desired. Hence the above approach which, by changing from caching
X509Certificate* to OSCertHandle, preserves the memory savings (although
reliably only for Mac/OpenSSL, since as-implemented on Windows/NSS, they're
already OS-ref-counted), while allowing X509Certificate* to maintain it's
current meaning as "transport object for a bunch of hopefully-related
certificates"

For what it's worth, I fully support the "X509Certificate is a single
certificate" idea, with CertificateList being used to represent a chain or
"blob" of certificates. The overhead with that approach though is it means that
Subject/Issuer/Validity dates (and later, potentially public keys) all end up
parsed for every X509Certificate, when the UI (#1) generally only cares about it
for the EE certificate.

I don't think we want to be caching by X509Certificate*, although I can see the
value in caching the subject/issuer/validity. I think caching by the OS handle
is somewhat dangerous on Windows and Mac, and imposes certain unnecessary and
potentially undesirable limitations. Neither OS guarantees that the OSCertHandle
will be thread-safe, while the X509Certificate* IS supposed to be thread-safe.
Further, an OSCertHandle created from an in-memory store or from raw data is not
the same object as an OSCertHandle created from the MY cert store / Keychain,
under the hood. Though they represent the same certificate, both Windows and Mac
have meta-information associated with the OSCertHandle (handle to a private
key/keychain, for example) that may not be preserved when copying between
stores, nor may it be thread-safe either.

[Ryan Sleevi, 2011-06-21 04:26:21]

wtc: Ping

[Ryan Sleevi, 2011-07-07 07:03:56]

wtc: I've updated this to ToT and split out some more changes that can/should
otherwise stand alone (Thread-safe use of the OSCertHandle on Windows and
handling client/server intermediates for SSLClientSocketWin)

Please disregard the two changes in chrome/browser/ui, and the TODO(rsleevi) in
ssl_client_socket_nss.cc, as I need to work some more rebase magic to get these
commits sorted out. However, all three are cosmetic changes, so they should be
easy to disregard.

[Chris Evans, 2011-07-14 22:42:49]

wtc: another ping?

On 2011/07/07 07:03:56, Ryan Sleevi wrote:
> wtc: I've updated this to ToT and split out some more changes that can/should
> otherwise stand alone (Thread-safe use of the OSCertHandle on Windows and
> handling client/server intermediates for SSLClientSocketWin)
> 
> Please disregard the two changes in chrome/browser/ui, and the TODO(rsleevi)
in
> ssl_client_socket_nss.cc, as I need to work some more rebase magic to get
these
> commits sorted out. However, all three are cosmetic changes, so they should be
> easy to disregard.

[wtc, 2011-07-16 00:54:54]

rsleevi: I have read the whole CL.  I summarized that it
contains three main changes:

1. Cache OS certificate handles instead of X509Certificate
objects.

2. Remove the Source field of X509Certificate.

3. For OS_WIN, remove our own in-memory certificate store.
Just add certificates to the NULL store.

I want to send you an important comment on #3 first.

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (left):

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate....
net/base/x509_certificate.cc:191: return ok ? cert_handle : NULL;
IMPORTANT: Removing X509Certificate::cert_store() may
reintroduce bug 45706.

In r48650 I made essentially the same change:
http://codereview.chromium.org/2322008

But it introduced bug 45706.  I couldn't track down the
bug, so I reverted it in r48953:
http://codereview.chromium.org/2605007

Note that the way we get the server's certificate chain
has changed since then.  We used to call the NSS function
CERT_GetCertChainFromCert.  Now we call the new NSS function
SSL_PeerCertificateChain.  It is possible that bug 45706
was caused by the use of CERT_GetCertChainFromCert.

So please test this carefully, including the test case in
bug 45706.  I would love this to work!

[Ryan Sleevi, 2011-07-16 01:24:22]

On 2011/07/16 00:54:54, wtc wrote:
> rsleevi: I have read the whole CL.  I summarized that it
> contains three main changes:
> 
> 1. Cache OS certificate handles instead of X509Certificate
> objects.
> 
> 2. Remove the Source field of X509Certificate.

Correct. The behaviour of the Source field was directly tied to the requirements
of the HTTP Cache and the usage of the X509CertificateCache - the Source was
used to eject lower quality certificates from the cache, in order to replace
them with higher quality certificates (namely, those X509Certificate*'s that
have their entire chain). This was introduced because of http://crbug.com/7065

First, this is no longer necessary because of http://crrev.com/82214, which
allows "from (disk) cache" certificates to store their chain. So there is no
longer a need to distinguish between Sources, since all Sources are effectively
equal.

Second, because the X509CertificateCache only contains individual certificates,
not complete certificate chains (via storing the X509Certificate*), the Source
argument no longer distinguishes chains vs individual certificates.

So this was removed as both out-dated (with r82214) and (with this CL)
superseded functionality.

> 3. For OS_WIN, remove our own in-memory certificate store.
> Just add certificates to the NULL store.
> 
> I want to send you an important comment on #3 first.

Correct. I originally packaged http://codereview.chromium.org/7324039/ into this
codereview, but in the process of splitting things out, I pulled this out. I
haven't published that second CL, since I saw/knew you were swamped already, and
that CL did not strictly tie to the U-A-F case, at least as far as security
impact goes.

This was done, in part, to avoid a shutdown ordering issue in shutting down the
cert_store() and shutting down the X509CertificateCache, which would otherwise
contain live pointers to certificates stored within the cert_store(). This
applies for both client certificates (SSLClientSocketWin) and server
certificates (X509CertificateCache). 

The other reason this was done was because originally, the two changes were
integrated, although I believe this behaviour still belongs in scope for this
CL.

I haven't poked around the Cert* routines to confirm, but in looking further at
this change, I believe the old behaviour (CertCloseStore() called by a
LazyInstance in X509CertificateWin, registered with ChromeMain's AtExitHandler)
may have had a stale reference race, similar to the one seen in NSS. With the
new behaviour, this should be avoided.

The original GlobalCertStore (X509CertificateWin) uses
DefaultLazyInstanceTraits, rather than LeakyLazyInstanceTraits. As a result,
when the AtExit handler is run, the GlobalCertStore will be torn down. This will
result in CertCloseStore() being called. What I'm uncertain is if this will
invalidate the existing PCCERT_CONTEXT's (or make them dangerous to use, perhaps
by holding onto a pointer to an invalid cert store), or if it just prevents them
from writing to the backing store. This question is in part due to the fact that
CertOpenStore has an explicit flag, CERT_STORE_DEFER_CLOSE_UNTIL_LAST_FREE_FLAG,
to indicate that it's OK to keep a store open following a CertCloseStore() if
there are still PCCERT_CONTEXT's open (such as the one held by the CertVerifier
thread).

This would seem in scope with the possibility of U-A-F, but on Windows only. The
new code uses a LeakyLazyInstanceTraits, so the result is akin to NSS, which is
to leak the handles.

If your concern is that of risk, I think an alternative would be to make
GlobalCertStore leaky (or deferred) with this CL, and then eradicate it with CL
7324039. 

> 
>
http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate.cc
> File net/base/x509_certificate.cc (left):
> 
>
http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate....
> net/base/x509_certificate.cc:191: return ok ? cert_handle : NULL;
> IMPORTANT: Removing X509Certificate::cert_store() may
> reintroduce bug 45706.
> 
> In r48650 I made essentially the same change:
> http://codereview.chromium.org/2322008
> 
> But it introduced bug 45706.  I couldn't track down the
> bug, so I reverted it in r48953:
> http://codereview.chromium.org/2605007
> 
> Note that the way we get the server's certificate chain
> has changed since then.  We used to call the NSS function
> CERT_GetCertChainFromCert.  Now we call the new NSS function
> SSL_PeerCertificateChain.  It is possible that bug 45706
> was caused by the use of CERT_GetCertChainFromCert.
> 
> So please test this carefully, including the test case in
> bug 45706.  I would love this to work!

[wtc, 2011-07-17 01:55:32]

LGTM.

High-Level Comments (both of which you can ignore)

1. Ideally CreateOSCertHandleFromBytes and
CreateOSCertHandlesFromBytes (note the 's' in 'Handles')
should consult the cache first before importing the certs
into the underlying crypto libraries.  But that could be
a lot of work, and I believe a good crypto library caches
certificates internally anyway.

2. The new cache doesn't solve the problem that all
OS certificate handles are not equal.  This problem has
to do with the "meta data" in OS certificate handles,
such as the key provider info in Windows CryptoAPI,
or the PK11SlotInfo *slot member in NSS.

3. We will use more memory because the subject_ and
issuer_ fields are stored in every X509Certificate object.
In the long run we should make X509Certificate contain
essentially the OS cert handles and get the subject,
issuer fields from the OS cert handles.  The parsing of
the fields will be done on demand.

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (left):

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate....
net/base/x509_certificate.cc:75: // You must acquire this lock before using any
private data of this object.
It doesn't seem necessary to wrap this line.

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

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate....
net/base/x509_certificate.cc:67: class X509CertificateCache {
This class should be renamed OSCertHandleCache.
You can do the renaming in a separate CL.

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate....
net/base/x509_certificate.cc:90: Entry() : cert_handle(NULL), ref_count(0) {}
Add a blank line below.

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate....
net/base/x509_certificate.cc:96: size_t ref_count;
Define ref_count as an 'int'.  This is what RefCountedBase
does.

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate....
net/base/x509_certificate.cc:125: "Only insert certs with real fingerprints.";
Delete this DCHECK.  This DCHECK was only meaningful in the
old code because some X509Certificate objects are fake
certificates and don't have fingerprints.

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate....
net/base/x509_certificate.cc:144: // Two certificates don't match, likely due to
a SHA1 hash collision.
Remove "likely".

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate....
net/base/x509_certificate.cc:154: DHISTOGRAM_COUNTS("X509CertificateReuseCount",
1);
Move this DHISTOGRAM_COUNTS call outside the critical section.

You can do this at the end of this function:
  if (old_handle) {
    X509Certificate::FreeOSCertHandle(old_handle);
    DHISTOGRAM_COUNTS("X509CertificateReuseCount", 1);
  }

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate....
net/base/x509_certificate.cc:157: *cert_handle =
X509Certificate::DupOSCertHandle(pos->second.cert_handle);
Note how you increment pos->second.ref_count and the
ref-count of pos->second.cert_handle together.  This point
will be important in my comment on Remove() below.

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate....
net/base/x509_certificate.cc:180: return;  // A cache collision where the
winning cert is still around.
In the comments on lines 175 and 180, change
"cache collision" to "hash collision".

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate....
net/base/x509_certificate.cc:187:
X509Certificate::FreeOSCertHandle(pos->second.cert_handle);
Here, you decrement pos->second.ref_count every time, but
only decrement the ref-count of pos->second.cert_handle when
pos->second.ref_count becomes 0.  So pos->second.cert_handle
has a reference leak.

This can be fixed in two ways.

1. Move this X509Certificate::FreeOSCertHandle() call outside
the if statement, to line 181.

2. In InsertOrUpdate(), call X509Certificate::DupOSCertHandle()
only when an Entry is newly created, on line 136.
This means we acquire an OSCertHandle reference for
Entry::cert_handle.

The second solution is my preference.

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate....
net/base/x509_certificate.cc:226: X509Certificate* cert = new
X509Certificate(cert_handle, intermediates);
The local variable 'cert' was necessary in the old code.
In the new code, just do
  return new X509Certificate(cert_handle, intermediates);

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate_...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate_...
net/base/x509_certificate_mac.cc:1150: X509Certificate::OSCertHandles()));
Move this to the previous line.

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate_...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate_...
net/base/x509_certificate_unittest.cc:664: // it returns a cached or new
X509Certificate object.
The second sentence of this paragraph should be removed or
updated.

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate_...
net/base/x509_certificate_unittest.cc:668: // come from the network.
Delete this paragraph.

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate_...
net/base/x509_certificate_unittest.cc:680: // Add the same certificate, but as a
new handle.
Note: NSS should return the same handle here.

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate_...
net/base/x509_certificate_unittest.cc:692:
cert2->GetIntermediateCertificates()));
Since cert1 and cert2 don't have intermediate CA certificates,
this check (lines 691-692) is not effective.

Note: HasIntermediateCertificates uses IsSameOSCert rather
than OS cert handle value equality, so that weakens the
check for the new X509CertificateCache even if cert1 and
cert2 have intermediate CA certificates.

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate_...
net/base/x509_certificate_unittest.cc:741: EXPECT_NE(cert.get(),
cert_from_pickle.get());
Consider removing this check, irrelevant in the new code. 
This check was linked to the "Faking SOURCE_LONE_CERT_IMPORT
..." comment in the old code.

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate_...
net/base/x509_certificate_unittest.cc:817: EXPECT_NE(cert1, cert2);
Delete this comment and check, just like you deleted the
comment and check on lines 854-855 of the old code.

Perhaps this entire unit test should be deleted.

http://codereview.chromium.org/2944008/diff/229001/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/2944008/diff/229001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:2058:
chain_context->rgpChain[0]->rgpElement[i]->pCertContext,
Move this to the previous line.

http://codereview.chromium.org/2944008/diff/229001/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/2944008/diff/229001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_openssl.cc:873: intermediates);
This may fit on the previous line.

[Ryan Sleevi, 2011-07-17 03:52:22]

wtc: Thanks for reviewing. Since you raised a point about a possible bug/leak, I
wanted to make sure I sufficiently addressed it for you. It should just be a
matter of making sure the documentation and my comments below are sufficient.

Regarding your high level comments, I agree with both of them and they're
something I'd like to (eventually) address. It's been over a year since the
issue was first noticed and this CL was first written, and the code and paths
have changed somewhat, as well as a good deal of new code (eg: OpenSSL, server
sockets) that have brought in new considerations.

For 1 - Windows & NSS cache, OS X & OpenSSL don't.

For 2/3 - I think one possible way to address your concerns would be to change
the cache from one that caches the OS handle to one that caches the certificate
information. This will avoid needing to reparse the entire certificate and all
it's extensions (OS X), needing to hit the registry repeatedly to look up ASN.1
providers (Win), or using the same handle for two certificates which might have
different private key references (Windows, NSS). This solution is probably in
sufficient for OpenSSL, due to how the DER data is cached.

It'd probably be best to file bugs for them, to be able to collaborate on what
the appropriate direction is.

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

http://codereview.chromium.org/2944008/diff/229001/net/base/x509_certificate....
net/base/x509_certificate.cc:187:
X509Certificate::FreeOSCertHandle(pos->second.cert_handle);
On 2011/07/17 01:55:32, wtc wrote:
> Here, you decrement pos->second.ref_count every time, but
> only decrement the ref-count of pos->second.cert_handle when
> pos->second.ref_count becomes 0.  So pos->second.cert_handle
> has a reference leak.
> 
> This can be fixed in two ways.
> 
> 1. Move this X509Certificate::FreeOSCertHandle() call outside
> the if statement, to line 181.
> 
> 2. In InsertOrUpdate(), call X509Certificate::DupOSCertHandle()
> only when an Entry is newly created, on line 136.
> This means we acquire an OSCertHandle reference for
> Entry::cert_handle.
> 
> The second solution is my preference.

I don't believe there is a leak, and have made an attempt to update the
documentation to that effect. I want to make sure you're happy with the
explanation being sufficiently clear, so I'll hold off committing until you're
OK with the verbiage.

Each caller to InsertOrUpdate is guaranteed to "own" their own OS handle -
meaning they're responsible for calling FreeOSCertHandle() on it as well. If the
entry is new to the cache, the cache will add a reference that the Cache owns as
well (so two handles - one for the cache, one for the caller).

If the entry is an existing element, then the caller's original handle will be
freed, and it will be substituted with a duplicate of the element in the cache.

When Remove() is called, the caller owns a handle and the Cache, if it's found,
also owns a handle. This call removes the Cache's handle, so that the only owner
left (of the OS Cert handle) is the caller, who then subsequently frees it
(lines 587/591). So the adds and removes are balanced.

[wtc, 2011-07-17 14:39:54]

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

http://codereview.chromium.org/2944008/diff/236003/net/base/x509_certificate....
net/base/x509_certificate.cc:617: }
rsleevi: I hope I didn't misunderstand your code.  Here is
my proof that there is a reference leak.

Looking at the X509Certificate constructor and destructor,
we verify that:
1. The cache->InsertOrUpdate and cache->Remove calls are
balanced.
2. The DupOSCertHandle and FreeOSCertHandle calls (in the
X509Certificate constructor and destructor) are balanced.

Therefore, the DupOSCertHandle and FreeOSCertHandle calls
inside cache->InsertOrUpdate and cache->Remove must be
balanced.  But there are more DupOSCertHandle calls in
cache->InsertOrUpdate than FreeOSCertHandle calls in
cache->Remove.

[wtc, 2011-07-17 15:11:49]

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

http://codereview.chromium.org/2944008/diff/236003/net/base/x509_certificate....
net/base/x509_certificate.cc:617: }
On 2011/07/17 14:39:54, wtc wrote:
>
> Therefore, the DupOSCertHandle and FreeOSCertHandle calls
> inside cache->InsertOrUpdate and cache->Remove must be
> balanced.  But there are more DupOSCertHandle calls in
> cache->InsertOrUpdate than FreeOSCertHandle calls in
> cache->Remove.

rsleevi: I see the flaw in my proof.  The last sentence
of my proof is wrong.  Sorry about my confusion.

[wtc, 2011-07-17 15:26:19]

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

http://codereview.chromium.org/2944008/diff/236003/net/base/x509_certificate....
net/base/x509_certificate.cc:172: }
I suggest the following:

  X509Certificate::OSCertHandle old_handle = NULL;
  {
    base::AutoLock lock(lock_);
    CertMap::iterator pos = cache_.find(fingerprint);
    if (pos == cache_.end()) {
      Entry cache_entry;
      ...
      cache_entry.cert_handle = X509Certificate::DupOSCertHandle(*cert_handle);
      ...
      pos = cache_.insert(cache_value).first;
    } else {
      ...
      old_handle = *cert_handle;
      *cert_handle = pos->second.cert_handle;
    }
    ++pos->second.ref_count;
  }

  if (old_handle) {
    X509Certificate::FreeOSCertHandle(old_handle);
    X509Certificate::DupOSCertHandle(*cert_handle);
    ...
  }

[Ryan Sleevi, 2011-07-17 23:43:30]

wtc: I'm glad you saw where they balanced. I've omitted making your suggested
change, because I think it may expose a very, very technically complex and
improbable timing bug, in the name of paranoia.

I'll look to commit this within the next 24 hours, baring any further changes.

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

http://codereview.chromium.org/2944008/diff/236003/net/base/x509_certificate....
net/base/x509_certificate.cc:172: }
I don't believe it's safe to do the Dup of the existing entry outside of the
lock, due to the edge case of a hash collision. 

Steps:
A and B represent two colliding certificates when hashed with SHA.1. A.1 / B.1
refer to the first instance of those certificates.

Create X509Certificate(A.1). A is added to the cache.
Create X509Certificate(B.1). B is not added to the cache (because of A).
Create X509Certificate(B.2). B is not added to the cache (because of A).
Free X509Certificate(A.1). A is removed from the cache.
Create X509Certificate(B.3). B is added to the cache.
----
Begin Raciness
----
Thread 1:
1) Create X509Certificate(B.4). B is incremented in the cache and the lock
released.
2) Suspend thread 1
5) Wake thread 1. Attempt to dupe the handle, which is now stale/invalid.
----
Thread 2:
3) Free X509Certificate(B.1). The lock is obtained, the ref count is decremented
(B.4 incremented it).
4) Free X509Certificate(B.2). The lock is obtained, the ref count is decremented
(B.3 created it). The ref count is now 0, the handle is freed.
----


For that reason, albeit subtle and an extreme edge case, I've left it as
originally written. The fact that freeing B.1/B.2 can release the reference
created/incremented by B.3/B.4 is not a problem, as the counts are still
balanced and the X509Certificate's each retain ownership of their handle, so
there are no leaks because of it.

While the scenario described above relies on a SHA1 collision of two
certificates WITH signatures, A and B, which we presume is extremely unlikely
and highly technically complex, in the event of any further SHA-1 weakening it's
probably better to leave as is.

[wtc, 2011-07-18 22:20:26]

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

http://codereview.chromium.org/2944008/diff/236003/net/base/x509_certificate....
net/base/x509_certificate.cc:172: }
rsleevi: you're right.  My proposed code relies on
Entry::ref_count being >= 1 before returning from
InsertOrUpdate.  With hash collision of certificates this
is not true.

It is surprising that an X509Certificate object that did not
increment Entry::ref_count can decrement Entry::ref_count
because of hash collision, i.e., a certificate can be the
losing cert in InsertOrUpdate but be the winning cert in Remove,
and vice versa.

I think your explanation that this will still work out
OK is correct.