Ensure X509Certificate::OSCertHandles are safe to be used on both UI and IO threads on Win

Ensure X509Certificate::OSCertHandles are safe to be used on both UI, IO, and Worker threads on Win.

Mirror the behaviour of SChannel by creating a new in-memory HCERTSTORE containing the certificate and its intermediate CA certificates whenever it is necessary to pass in a PCCERT_CONTEXT to a Windows API that may need to access the PCCERT_CONTEXT->hCertStore - such as certificate chain verification or display.

This also paves the way for removing the GlobalCertStore on Windows, which was necessary in order to link certificates with their intermediates for these same APIs.

BUG=47648
TEST=net_unittests:X509CertificateTest.* should cover this. Additionally, on a fresh profile, navigate to different HTTPS sites. From the Page Info Bubble, select Certificate Information, and in the Windows Certificate Viewer, click "Certification Path" and confirm the entire certificate chain is displayed. This is a variation of testing for http://crbug.com/45706, which should not regress.

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

[Ryan Sleevi, 2011-07-20 01:49:20]

wtc: Split from the caching changes.

To be clear, everything I've seen from the MSFT store/CSPs are that things are
thread-safe/properly locked with respect to operations. The (possibly only) area
I believe is problematic is in iterating the store while modifications are
happening. This is done thread-safe with respect to the internal linked list,
but if modifications are happening, it can result in certificates being skipped
during Verify() / display.

When testing with SChannel, every time the certificate context was queried, a
new CERT_STORE_PROV_MEMORY was created, with
CERT_STORE_DEFER_CLOSE_UNTIL_LAST_FREE_FLAG set, and the chain certificate(s)
were then added. I believe we should match that behaviour, as that's the most
likely behaviour existing software will be coded against.

[Ryan Sleevi, 2011-07-26 23:09:14]

wtc: Ping on this?

[Ryan Sleevi, 2011-07-29 21:54:07]

On 2011/07/26 23:09:14, Ryan Sleevi wrote:
> wtc: Ping on this?

On 2011/07/26 23:09:14, Ryan Sleevi wrote:
> wtc: Ping on this?

wtc: Reping.

[wtc, 2011-10-04 00:26:34]

LGTM on Patch Set 1.

I am sorry about the very late review.

Your first comment (describing what you found about the
thread safety of PCCERT_CONTEXT and how SChannel uses it)
is very informative.  I suggest you put it in the CL's commit
message or even inside the source code.

I suggest some changes below, mostly to improve the comments.
I also have some questions in my comments.

http://codereview.chromium.org/7324039/diff/1/chrome/browser/ui/cocoa/certifi...
File chrome/browser/ui/cocoa/certificate_viewer.mm (right):

http://codereview.chromium.org/7324039/diff/1/chrome/browser/ui/cocoa/certifi...
chrome/browser/ui/cocoa/certificate_viewer.mm:12: #include "base/mac/mac_util.h"
I believe we can include "base/mac/foundation_util.h" instead.

http://codereview.chromium.org/7324039/diff/1/chrome/browser/ui/views/certifi...
File chrome/browser/ui/views/certificate_viewer_win.cc (right):

http://codereview.chromium.org/7324039/diff/1/chrome/browser/ui/views/certifi...
chrome/browser/ui/views/certificate_viewer_win.cc:29: HCERTSTORE cert_store =
const_cast<const HCERTSTORE>(cert_list->hCertStore);
Is this const_cast necessary?  Also, we should not have 'const'
inside const_cast.

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

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:64: // An OSCertListHandle is a handle to the
underlying crypto library that
Add "the object in" before "the underlying crypto library".

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:66: // marked as an identity certificate and the
remaining certificates marked
I suggest changing "an identity certificate" to
"a primary certificate" (or "the primary certificate"?).
The meaning of "identity certificate" is not clear here.

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:71: // It should be noted that depending on the
underlying cryptographic
Nit: for brevity, change "It should be noted that" to
"Note: " or "Note that", or just remove it.

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:72: // library, an OSCertHandle or OSCertListHandle
may not be thread-safe.
Please add a comment to motivate OSCertListHandle.  I think
it is for thread safety.  Since OSCertListHandle may not be
thread-safe, we create an OSCertListHandle for each thread
that needs one.  Do I understand this correctly?

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:76: // used for the certificate containing just the
subset of related
Add "store" after "certificate".

Change "related" to "supplementary".

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:83: typedef struct x509_st* OSCertHandle;
Change "struct x509_st" to X509.

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:94: typedef void* OSCertListHandle;
Nit: it would be nice to use the same style of typedef
when OSCertListHandle is the same as OSCertHandle.

On line 78 and line 94, we duplicate the typedef.
On line 90, we define OSCertListHandle as OSCertHandle.

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:262: // associated intermediates, or NULL on
failure. Ownership is transferred
Nit: intermediates => intermediate certificates
                 or   intermediate CA certificates

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:378: static void
FreeOSCertListHandle(OSCertListHandle cert_list);
cert_list => cert_list_handle

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

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:769: }
Remove curly braces.

A comment like lines 783-786 that emphasizes cert_handle_ must
be first would be useful.  Perhaps we can just move lines
783-786 here.  Alternatively we can document this fact in
x509_certificate.h when we define the OSCertListHandle type.

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:1086: void
X509Certificate::FreeOSCertListHandle(OSCertListHandle identity) {
identity => cert_list_handle

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

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:770: }
Change VerifyInternal to use CreateOSCertListHandle().

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:981: void
X509Certificate::FreeOSCertListHandle(OSCertListHandle cert_list) {
cert_list => cert_list_handle

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate_opens...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate_opens...
net/base/x509_certificate_openssl.cc:311: void
X509Certificate::FreeOSCertListHandle(OSCertListHandle cert_list) {
cert_list => cert_list_handle

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate_opens...
net/base/x509_certificate_openssl.cc:351:
X509Certificate::CreateOSCertListHandle() const {
Change VerifyInternal to use CreateOSCertListHandle().

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

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate_win.c...
net/base/x509_certificate_win.cc:635: // Create a temporary certificate store to
hold |cert_handle_| and any
temporary => in-memory

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate_win.c...
net/base/x509_certificate_win.cc:636: // associated intermediates. The store
will be referenced in the returned
intermediates => intermediate certificates
            or   intermediate CA certificates

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate_win.c...
net/base/x509_certificate_win.cc:649: // the identity is used for client auth,
it may prompt the user again.
Nit: identity => certificate handle?

Do we ever use the return value of CreateOSCertListHandle()
for client auth?  I think we only call CreateOSCertListHandle()
in VerifyInternal() and the certificate viewer dialog, so
I don't think the scenario this comment describes will ever
happen.

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate_win.c...
net/base/x509_certificate_win.cc:753: FreeOSCertListHandle(cert_list);
Do we have a scoped type for PCCERTCONTEXT?  We could use it
here for |cert_list|.

[Ryan Sleevi, 2011-10-04 03:38:07]

wtc: PTAL.

Note one "significant" change between patchset 1 and patchset 2 was the removal
of a dedicated header for ScopedPCCERT_CHAIN_CONTEXT. Considering that this
implementation already has local scoped-wrappers in its anonymous namespace,
that more were being added (ScopedPCCERT_CONTEXT), and that there are no
existing users of scoped_cert_chain_context, this seemed like an appropriate
task and not warranting a separate CL. If you disagree, I can pull it out.

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

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:66: // marked as an identity certificate and the
remaining certificates marked
On 2011/10/04 00:26:34, wtc wrote:
> I suggest changing "an identity certificate" to
> "a primary certificate" (or "the primary certificate"?).
> The meaning of "identity certificate" is not clear here.

Does the explanation on line 53-54 provide the necessary context?

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

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:770: }
On 2011/10/04 00:26:34, wtc wrote:
> Change VerifyInternal to use CreateOSCertListHandle().

I don't think this is desirable for NSS, since then it would have to be scoped
to un-dup.

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

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate_win.c...
net/base/x509_certificate_win.cc:649: // the identity is used for client auth,
it may prompt the user again.
On 2011/10/04 00:26:34, wtc wrote:
> Nit: identity => certificate handle?
> 
> Do we ever use the return value of CreateOSCertListHandle()
> for client auth?  I think we only call CreateOSCertListHandle()
> in VerifyInternal() and the certificate viewer dialog, so
> I don't think the scenario this comment describes will ever
> happen.

Now that certificate prompting has been moved into native UI, this probably
doesn't matter as much. The native UI will access to HCERSTOREs of incoming
certs in order to build chains (used to prioritize the order in the UI).

I feel like leaving this subtle note documented is important, although I agree,
in the current code, this shouldn't be hit. Would you prefer I remove it
entirely?

[wtc, 2011-10-04 18:00:51]

LGTM on Patch Set 3.

High-level comments:

You can ignore my complaints on the comments in the source
code except for one: it is important to explain the difference
between OSCertHandle and OSCertListHandle and when one needs
to use OSCertListHandle.

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

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:66: // marked as an identity certificate and the
remaining certificates marked
On 2011/10/04 03:38:07, Ryan Sleevi wrote:
> 
> Does the explanation on line 53-54 provide the necessary context?

This is fine.  My complaint about this comment is that
every certificate we deal with here is an identity certificate,
so we need a way to distinguish the cert represented by
os_cert_handle_ and the other certs used for chain building.
This is why I suggested "primary" vs. "supplementary".

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

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:770: }
On 2011/10/04 03:38:07, Ryan Sleevi wrote:
>
> I don't think this is desirable for NSS, since then it would have to be scoped
> to un-dup.

The reason I suggested having VerifyInternal use
CreateOSCertListHandle on all platforms is that the
consistency makes it easier to explain when one should
use os_cert_handle() and when one should use CreateOSCertListHandle().

With your proposal, we will need to explain why
CreateOSCertListHandle() is necessary only for Mac (which
needs an array of SecCertificateRef) and Windows (for
thread safety of certificate store enumeration).  It still
works, but it makes it harder for a future maintainer to
understand this code.

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

http://codereview.chromium.org/7324039/diff/1/net/base/x509_certificate_win.c...
net/base/x509_certificate_win.cc:649: // the identity is used for client auth,
it may prompt the user again.
On 2011/10/04 03:38:07, Ryan Sleevi wrote:
>
> I feel like leaving this subtle note documented is important, although I
agree,
> in the current code, this shouldn't be hit. Would you prefer I remove it
> entirely?

You can keep this comment.  I asked the question to verify
my understanding of your code.  It did confuse me a little
bit, but not much.

http://codereview.chromium.org/7324039/diff/10002/chrome/browser/ui/views/cer...
File chrome/browser/ui/views/certificate_viewer_win.cc (right):

http://codereview.chromium.org/7324039/diff/10002/chrome/browser/ui/views/cer...
chrome/browser/ui/views/certificate_viewer_win.cc:30: view_info.rghStores =
&const_cast<HCERTSTORE>(cert_list->hCertStore);
This const_cast looks strange because const_cast is usually
applied to a pointer.  This would look better to me:
  view_info.rghStores = const_cast<HCERTSTORE*>(&cert_list->hCertStore);

Using a local variable, as your old patch set did, would avoid
the const_cast:
  HCERTSTORE cert_store = cert_list->hCertStore;
  ...
  view_info.rghStores = &cert_store;

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

http://codereview.chromium.org/7324039/diff/10002/net/base/x509_certificate.h...
net/base/x509_certificate.h:74: // OSCertListHandle should use
CreateOSCertListHandle() or only allow it to
In "only allow it to be used on a single thread", it's not
clear what "it" refers to.

The goal of this paragraph is to explain to people when they
can use os_cert_handle(), and when they must use CreateOSCertListHandle().
Please make sure the distinction between the two types is
addressed.  (I'll let you judge it.)

http://codereview.chromium.org/7324039/diff/10002/net/base/x509_certificate_w...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/7324039/diff/10002/net/base/x509_certificate_w...
net/base/x509_certificate_win.cc:66: 
Nit: delete one blank line.

http://codereview.chromium.org/7324039/diff/10002/net/base/x509_certificate_w...
net/base/x509_certificate_win.cc:833: ScopedPCCERT_CONTEXT
cert_list(CreateOSCertListHandle());
I don't see where the ScopedPCCERT_CONTEXT type is defined.
Is this a typo of ScopedCertContext?

[Ryan Sleevi, 2011-10-10 02:20:50]

wtc: It appears that the bulk of the feedback is related to the fact that
OSCertListHandles are used for different things depending on the platform:

Windows: For thread-safety (for certain functions)
OS X: As a convenience wrapper for getting CFArray
OpenSSL/NSS: Not necessary

My concern, when originally implementing, is that I wanted to avoid having
platform-specific functionality #ifdef'd into X509Certificate (eg:
Windows/OpenSSL's cert_store()); instead, keep the interface consistent.
However, as you pointed out, even if the interface is consistent, the usage
wasn't, so this was still serving as a point of confusion.

I agree that this perhaps creates too much overloading/confusion. I've published
a new patchset that contains no functional changes to the code - it's the same
structurally as what you reviewed. However, it splits off the
thread-safety-on-Windows and the CFArray-on-OS-X into separate functions. This
also greatly expands on the comments to better clarify when these functions
should be used.

My hope is that you'll find this is a bit more obvious in both intent and
implementation. What do you think?

[wtc, 2011-10-16 14:55:49]

Patch Set 5 LGTM.  Thanks.

High-level comments

1. I think it's better to make CreateOSCertChainForCert
platform-specific methods of X509Certificate, only defined
for Mac and Windows, with platform-specific function
prototypes.

2. In comments that serve as formal documentation, please
don't omit "certificate" when refering to an intermediate
certificate.  (I would even say "intermediate CA certificate",
but I seem to be the only one.)

All of my review comments below are suggestions to help
make the source code comments clearer.

http://codereview.chromium.org/7324039/diff/22001/chrome/browser/ui/views/cer...
File chrome/browser/ui/views/certificate_viewer_win.cc (right):

http://codereview.chromium.org/7324039/diff/22001/chrome/browser/ui/views/cer...
chrome/browser/ui/views/certificate_viewer_win.cc:28: // Search the cert store
that 'cert' is in when building the cert chain.

Nit: this comment sounds weird since the code now uses
'cert_list', not 'cert'.  Also, the cert store is now
specifically constructed for this operation.

We should update this comment (e.g., "Search the cert
store that contains the cert and its intermediate CAs
when building the cert chain").  But I think we can also
simply remove the comment.

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

http://codereview.chromium.org/7324039/diff/22001/net/base/x509_certificate.h...
net/base/x509_certificate.h:57: // An OSCertHandle is a handle to a single
certificate object in the

Nit: remove "single".  Now that OSCertListHandle is gone,
the emphasis on "a single certificate" is no longer necessary.

http://codereview.chromium.org/7324039/diff/22001/net/base/x509_certificate.h...
net/base/x509_certificate.h:349: // Returns the current OSCertHandle.

Nit: "the current OSCertHandle" is not clear.  How about
"the OSCertHandle of the current object" or "the
OSCertHandle of this object"?

http://codereview.chromium.org/7324039/diff/22001/net/base/x509_util_mac.h
File net/base/x509_util_mac.h (right):

http://codereview.chromium.org/7324039/diff/22001/net/base/x509_util_mac.h#ne...
net/base/x509_util_mac.h:21: // optional intermediates.

Nit: any additional certificates contained as optional intermediates
=> intermediate certificates

You can add "any" to indicate that intermediate certificates
may not exist.

I.e., make this more concise, and use "intermediate CA certificates" or
"intermediate certificates" instead of
just "intermediates".

http://codereview.chromium.org/7324039/diff/22001/net/base/x509_util_win.cc
File net/base/x509_util_win.cc (right):

http://codereview.chromium.org/7324039/diff/22001/net/base/x509_util_win.cc#n...
net/base/x509_util_win.cc:28: // OSCertListHandle is freed.

Change the two occurrences of OSCertListHandle to
PCCERT_CONTEXT in this paragraph.

http://codereview.chromium.org/7324039/diff/22001/net/base/x509_util_win.cc#n...
net/base/x509_util_win.cc:39: // that the identity is used for client auth, it
may prompt the user again.

Nit: the identity => the returned PCCERT_CONTEXT

http://codereview.chromium.org/7324039/diff/22001/net/base/x509_util_win.cc#n...
net/base/x509_util_win.cc:56: // the store will be freed when |scoped_cert| is
freed.

|scoped_cert| => |primary_cert|

http://codereview.chromium.org/7324039/diff/22001/net/base/x509_util_win.h
File net/base/x509_util_win.h (right):

http://codereview.chromium.org/7324039/diff/22001/net/base/x509_util_win.h#ne...
net/base/x509_util_win.h:19: // NULL on failure.

Nit: intermediates => intermediate certificates

In the rest of this comment block, you may use "intermediates"
now that you have defined it in the first paragraph.

Please add a heading "Remarks", "Notes", or "Rationale"
after this paragraph to indicate that the subsequent
paragraphs merely provide background info rather than
describing how to use this function.  The only important
info from the subsequent paragraphs regarding usage is
"This function is only needed when the HCERTSTORE of the os_cert_handle()
will be accessed".  It may be a good idea to repeat that
in the first paragraph.

In any case, I just wanted to make the first paragraph
contain complete info for using this function correctly,
even if in a terse way, and mark the rest of this comment
block as providing additional details.  Please do what you
think is right to accomplish these goals.

http://codereview.chromium.org/7324039/diff/22001/net/base/x509_util_win.h#ne...
net/base/x509_util_win.h:24: // current implementation on Windows, all
X509Certificate::OSCertHandles

Nit: current implementation => current X509Certificate implementation

http://codereview.chromium.org/7324039/diff/22001/net/base/x509_util_win.h#ne...
net/base/x509_util_win.h:42: // returned PCCERT_CONTEXT *SHOULD NOT* be stored
in an X509Certificate, as

Nit: SHOULD => MUST?

[Ryan Sleevi, 2011-10-28 03:28:46]

wtc: I've made the minor updates to comments where you suggested, and I've moved
it back into the X509Certificate.

You've already LGTM'd this twice, so I don't think there is a need for review,
but I'll wait a few days for this to land - particularly if the CertVerifier fix
is forthcoming. If you're fine with this landing and potentially regressing
there until you land your fix, I'll go ahead and commit sooner.

[wtc, 2011-10-31 19:29:43]

Patch Set 12 LGTM.

I'm sorry I missed your question the other day.  I didn't
mean to delay your checkin, but I agree it's good to check
this in after my X509Certificate::chain_fingerprint() CL.
Thanks!

[commit-bot: I haz the power, 2011-11-01 00:47:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/7324039/46002

[commit-bot: I haz the power, 2011-11-01 01:57:56]

Change committed as 108056