Record when certificates signed with md[2,4,5] are encountered on OS X.

Record when certificates signed with md[2,4,5] are encountered on OS X.

R=wtc@chromium.org
BUG=101123
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108308

[Ryan Sleevi, 2011-10-22 02:23:21]

wtc: PTAL. Note also http://crbug.com/101231 - not sure if that's critical to
fix before this.

[wtc, 2011-10-25 18:24:44]

LGTM.  Thanks!

It's hard to predict if http://crbug.com/101231 will affect
the performance of this code.  I guess we should not delay
the fixing of http://crbug.com/101231 too long.

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

http://codereview.chromium.org/8374019/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:301: i < count; ++i) {

Does this not fit on the previous line?

http://codereview.chromium.org/8374019/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:325: break;

If we get here, it means the Mac OS X certificate library
malfunctioned, right?  So this is defensive programming?
Would be nice to add a comment to make this clear.

[palmer, 2011-10-25 19:53:50]

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

http://codereview.chromium.org/8374019/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:315: if
(!CSSMOIDEqual(&fields.fields[field].FieldOid,
"fields fields field field oid" is baffling. Are there better names we could
use?

CSSMFields cssmFields;
...
WhateverTheTypeIs fields = cssmFields.fields;
...

for (size_t i = 0; i < cssmFields.num_of_fields; ++i) {
    if (!CSSMOIDEqual(&fields[i].FieldOid, ...
...
...fields[i].FieldValue.Data)...
et c.

It's also a micro-optimization to hoist all those accesses of cssmFields out of
the loop. (Unless the compiler does it anyway, in which case the code might as
well also be more readable to humans.)

http://codereview.chromium.org/8374019/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:334: } else if (CSSMOIDEqual(alg_oid,
&CSSMOID_MD5WithRSA)) {
As in the other CL, we should keep track of MD4 CAs even though we don't think
we'll ever see any.

[Ryan Sleevi, 2011-11-01 06:45:39]

And this is why tests help :-)

Updated to reflect your comments. Also updated the tests due to MD4 being
explicitly distinctly non-supported. MD2, while disabled by policy, still
returns the chain, similar to the Windows behaviour.

For those playing the home game and want to watch where Apple disables
algorithms,
libsecurity_apple_x509_tp/lib/tpPolicies.cpp::iSignCheckSignatureAlgorithm . To
match their sig-alg checking logic, I've updated the CSSMOIDEquals to handle
both MDx and MDxWithRSA variants. While the former is arguably invalid, OS X
(like CryptoAPI) is lenient about this.

[wtc, 2011-11-02 00:28:42]

Patch Set 5 LGTM.

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

http://codereview.chromium.org/8374019/diff/8004/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:330: // Match the behaviour of OS X system
tools and defensively check that

Do OS X system tools really do these defensive checks?

http://codereview.chromium.org/8374019/diff/8004/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:348: CSSMOIDEqual(alg_oid,
&CSSMOID_MD5WithRSA)) {

I'm not convinced that we need to allow CSSMOID_MD2,
CSSMOID_MD4, and CSSMOID_MD5 here.  No need to handle these
corner cases.

http://codereview.chromium.org/8374019/diff/8004/net/base/x509_certificate_un...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/8374019/diff/8004/net/base/x509_certificate_un...
net/base/x509_certificate_unittest.cc:1602: #if !defined(USE_NSS) &&
!defined(OS_MACOSX)  // NSS & OS X don't support MD4.

Nit: I suggest that you just say "MD4 is not supported." in
these comments.

MD4 is rarely used and was probably never mentioned in
PKCS #1.  So it's not surprising that Mac doesn't support it.

http://codereview.chromium.org/8374019/diff/8004/net/base/x509_certificate_un...
net/base/x509_certificate_unittest.cc:1606: #if !defined(USE_NSS)  // NSS
disables MD2 by policy.

Nit: "by default" may be better than "by policy".  I don't
understand what you meant by "disable by policy".

[commit-bot: I haz the power, 2011-11-02 02:34:56]

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

[Ryan Sleevi, 2011-11-02 02:35:15]

Source references below, for the curious.

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

http://codereview.chromium.org/8374019/diff/8004/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:330: // Match the behaviour of OS X system
tools and defensively check that
On 2011/11/02 00:28:43, wtc wrote:
> 
> Do OS X system tools really do these defensive checks?

http://www.opensource.apple.com/source/libsecurity_apple_x509_tp/libsecurity_...

http://codereview.chromium.org/8374019/diff/8004/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:348: CSSMOIDEqual(alg_oid,
&CSSMOID_MD5WithRSA)) {
On 2011/11/02 00:28:43, wtc wrote:
> 
> I'm not convinced that we need to allow CSSMOID_MD2,
> CSSMOID_MD4, and CSSMOID_MD5 here.  No need to handle these
> corner cases.

The concern was if Apple's TP maps them under the hood.

The most recent revision of libsecurity_cssm shows them not doing it in
cssmOidToAlg(), so it's probably fine -
http://www.opensource.apple.com/source/libsecurity_cssm/libsecurity_cssm-5500...

[commit-bot: I haz the power, 2011-11-02 03:58:00]

Try job failure for 8374019-13002 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-02 14:53:01]

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

[commit-bot: I haz the power, 2011-11-02 17:09:27]

Change committed as 108308

[wtc, 2011-11-02 19:26:56]

Patch Set 6 LGTM.

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

http://codereview.chromium.org/8374019/diff/8004/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:330: // Match the behaviour of OS X system
tools and defensively check that

On 2011/11/02 02:35:16, Ryan Sleevi wrote:
> On 2011/11/02 00:28:43, wtc wrote:
> > 
> > Do OS X system tools really do these defensive checks?
> 
>
http://www.opensource.apple.com/source/libsecurity_apple_x509_tp/libsecurity_...

Thank you for the reference.  You're still more defensive
(in the sense of defensive programming) than OS X system
tools because they don't do null pointer checking when the
length check passes :-)

http://codereview.chromium.org/8374019/diff/8004/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:348: CSSMOIDEqual(alg_oid,
&CSSMOID_MD5WithRSA)) {

On 2011/11/02 02:35:16, Ryan Sleevi wrote:
>
> The concern was if Apple's TP maps them under the hood.
> 
> The most recent revision of libsecurity_cssm shows them not doing it in
> cssmOidToAlg(), so it's probably fine -
>
http://www.opensource.apple.com/source/libsecurity_cssm/libsecurity_cssm-5500...

Thank you for checking.

If Apple's TP allows such incorrectly-encoded RSA signatures,
we should override that and set CERT_STATUS_INVALID
on those certificates.  I'm glad we don't need to do
this.