When accessing certificate fields/extensions on OS X, only parse what is needed.

On OS X, when accessing a certificate field (subject, issuer, extension, etc), rather than parsing the entire certificate and converting all known fields to their CSSM equivalents, just parse the desired field.

Additionally, when parsing multiple fields, instead of parsing the certificate multiple times, parse it once and cache the internal parse results. While the cached handle cannot reliably be used across threads, it can reduce the amount of parsing for the common case, where constructing an X509Certificate on OS X requires parsing three fields.

BUG=101231
TEST=net_unittests passes on OS X 10.5, 10.6, and 10.7


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

[Ryan Sleevi, 2011-11-20 07:29:19]

wtc: I'm guessing this goes to you because of its low-level CSSM-ness.

This CL keeps the existing behaviour of reparsing the certificate every time
subjectAltNames are accessed. We can clean this up in a future CL if desired, by
adding a lock and caching the SANs.

On 10.5, some of the necessary locks are handled at the Keychain level, rather
than in the CSSM module (which I suspect is/was the cause for the Verify()
crashes, combined with the X509Certificate::Cache races that were fixed), which
is why the certificate is re-parsed each time fields are accessed. In practice,
this should only affect the SANs - which as I noted above, could be dealt with
differently.

palmer, mattm: FYI.

mattm: Do you have a 10.5 box to test your ECC patch on? If not, I can work on
getting a VM setup.

[mattm, 2011-11-22 02:59:33]

On 2011/11/20 07:29:19, Ryan Sleevi wrote:
> wtc: I'm guessing this goes to you because of its low-level CSSM-ness.
> 
> This CL keeps the existing behaviour of reparsing the certificate every time
> subjectAltNames are accessed. We can clean this up in a future CL if desired,
by
> adding a lock and caching the SANs.
> 
> On 10.5, some of the necessary locks are handled at the Keychain level, rather
> than in the CSSM module (which I suspect is/was the cause for the Verify()
> crashes, combined with the X509Certificate::Cache races that were fixed),
which
> is why the certificate is re-parsed each time fields are accessed. In
practice,
> this should only affect the SANs - which as I noted above, could be dealt with
> differently.
> 
> palmer, mattm: FYI.
> 
> mattm: Do you have a 10.5 box to test your ECC patch on? If not, I can work on
> getting a VM setup.

Thanks Ryan, I only have 10.6.  If you have time to try, that would be great!

[Ryan Sleevi, 2011-11-22 03:25:27]

On 2011/11/22 02:59:33, mattm wrote:
> Thanks Ryan, I only have 10.6.  If you have time to try, that would be great!

Just tested. Passes fine on your patchset 6, with the SnowLeopardOrLater check
commented out.

[wtc, 2011-12-02 01:17:01]

LGTM.  Excellent work.

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

http://codereview.chromium.org/8470012/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:159: if (!field_ || !oid_ || field_->Length <
sizeof(T))

This function only uses field_.  Why do we need to check oid_?

http://codereview.chromium.org/8470012/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:170: oid_ = const_cast<CSSM_OID_PTR>(oid);

This const_cast is not necessary.

http://codereview.chromium.org/8470012/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:187: // API/SPI to "cache" a certificate/CRL.
The exact meaning of a cached

What does "SPI" stand for?  "Service provider interface"?

http://codereview.chromium.org/8470012/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:196: // running on OS X 10.5, as it will fail
to convert some fields reference

Nit: add "that" between "fields reference"?

http://codereview.chromium.org/8470012/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:233: OSStatus GetField(const CSSM_OID*
field_oid,

Perhaps we should not use 'const' when dealing with CSSM_OID
pointers, to avoid the const_casts?

http://codereview.chromium.org/8470012/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:1245: if (status == CSSM_OK &&
key_usage.field()) {

Is the key_usage.field() test your paranoia, defensive programming?
Or is it actually necessary?

[Ryan Sleevi, 2011-12-02 03:14:46]

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

http://codereview.chromium.org/8470012/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:159: if (!field_ || !oid_ || field_->Length <
sizeof(T))
On 2011/12/02 01:17:01, wtc wrote:
> 
> This function only uses field_.  Why do we need to check oid_?

Er, thanks. Like the code on line 170, a left-over from an earlier design.

http://codereview.chromium.org/8470012/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:170: oid_ = const_cast<CSSM_OID_PTR>(oid);
On 2011/12/02 01:17:01, wtc wrote:
> 
> This const_cast is not necessary.

Thanks, fixed.

http://codereview.chromium.org/8470012/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:187: // API/SPI to "cache" a certificate/CRL.
The exact meaning of a cached
On 2011/12/02 01:17:01, wtc wrote:
> 
> What does "SPI" stand for?  "Service provider interface"?

Yes. I was trying to document that though the API says "Cache", the exact
interpretation of what "cache" means is up to the provider (and the provider
used is determined by the GUID of the CSSM_CL_HANDLE - which, in this case, is
the Apple CL)

http://codereview.chromium.org/8470012/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:196: // running on OS X 10.5, as it will fail
to convert some fields reference
On 2011/12/02 01:17:01, wtc wrote:
> 
> Nit: add "that" between "fields reference"?

Done.

http://codereview.chromium.org/8470012/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:233: OSStatus GetField(const CSSM_OID*
field_oid,
On 2011/12/02 01:17:01, wtc wrote:
> 
> Perhaps we should not use 'const' when dealing with CSSM_OID
> pointers, to avoid the const_casts?

This is one of those annoying things. In order to use the OS X defined
CSSM_OIDs, they need to be const CSSM_OID*, or a stack-local CSSM_OID needs to
be allocated and referenced. The "const CSSM_OID" is so we can use
&CSSMOID_Blah, as otherwise it will complain about treating a const-ref as
non-const.

The Apple code just uses C-style casts throughout (eg:
(CSSM_OID_PTR)&CSSMOID_Blah

http://codereview.chromium.org/8470012/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:1245: if (status == CSSM_OK &&
key_usage.field()) {
On 2011/12/02 01:17:01, wtc wrote:
> 
> Is the key_usage.field() test your paranoia, defensive programming?
> Or is it actually necessary?

A month ago I would have said the former, but after investigating the CRL bug,
and from spending more time looking through the Security.framework, I do see
this pattern reflected in the Apple code as well.

[commit-bot: I haz the power, 2011-12-02 03:16:29]

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

[commit-bot: I haz the power, 2011-12-02 04:30:56]

Change committed as 112636