Record when certificates signed with md[2,4,5] are encountered when using OpenSSL

Record when certificates signed with md[2,4,5] are encountered when using OpenSSL

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

[Ryan Sleevi, 2011-10-22 02:04:16]

joth: PTAL

[joth, 2011-10-23 17:10:26]

LGTM

Thanks,

[commit-bot: I haz the power, 2011-10-23 17:46:01]

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

[commit-bot: I haz the power, 2011-10-23 17:46:03]

Presubmit check for 8368015-1 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: net/base/x509_certificate_openssl.cc

[Ryan Sleevi, 2011-10-23 17:53:12]

wtc: For your rubberstamping.

[joth, 2011-10-24 09:01:55]

One thought: are there any tests that cover these flags? I just noticed that the
patch isn't enabling any and wondered if it should be.

[Ryan Sleevi, 2011-10-24 09:02:56]

On 2011/10/24 09:01:55, joth wrote:
> One thought: are there any tests that cover these flags? I just noticed that
the
> patch isn't enabling any and wondered if it should be.

Not yet. Will be addressing that in http://codereview.chromium.org/8374020/

[wtc, 2011-10-25 18:09:35]

LGTM.

[palmer, 2011-10-25 19:44:48]

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

http://codereview.chromium.org/8368015/diff/1/net/base/x509_certificate_opens...
net/base/x509_certificate_openssl.cc:484: verify_result->has_md4 = true;
Even if we never expect to see it, perhaps we should also check for MD4 CAs and
set true in that case?

[wtc, 2011-10-25 20:37:31]

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

http://codereview.chromium.org/8368015/diff/1/net/base/x509_certificate_opens...
net/base/x509_certificate_openssl.cc:484: verify_result->has_md4 = true;
On 2011/10/25 19:44:48, Chris P. wrote:
> Even if we never expect to see it, perhaps we should also check for MD4 CAs
and
> set true in that case?

Thank you for the suggestion.  I should explain why there
is no has_md4_ca.

The reason there are has_md2_ca and has_md5_ca is that
they allow us to disallow MD2 or MD5 CA certificates
first, before disallowing MD2 or MD5 server certificates.

MD4 is more broken than MD2 and MD5 and probably was only
supported by Windows.  Therefore we can directly ban all MD4
certificates.  A two-phase banning isn't necessary.

[palmer, 2011-10-25 20:52:29]

> MD4 is more broken than MD2 and MD5 and probably was only
> supported by Windows.  Therefore we can directly ban all MD4
> certificates.  A two-phase banning isn't necessary.

Excellent, thanks.

[Ryan Sleevi, 2011-11-01 08:01:55]

joth: Would you mind taking another look at this, as I made some non-trivial
changes.

Presently, if the certificate fails to verify, the information about the
(failed) chain isn't captured into CertVerifyResult. This shifts the logic for
building ->verified_cert and the ->has_md* variables to after chain building,
but before return.

On a successful chain, a second loop through the certificates
agl's SPKI extraction is done. I've kept it there to minimize the risk of it
being exposed to any malformed ASN.1, which OpenSSL is more forgiving of.

Everything looks good locally and I've pushed a try out to linux_redux

[joth, 2011-11-01 09:31:45]

LGTM with 2 suggestions.

And thanks once again!

http://codereview.chromium.org/8368015/diff/13003/net/base/x509_certificate_o...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/8368015/diff/13003/net/base/x509_certificate_o...
net/base/x509_certificate_openssl.cc:302: void GetCertChainInfo(X509_STORE_CTX*
store_ctx,
Not sure if more stuff is likely to migrate into here, but maybe we could make
the name more explicit about which info it gets? (also, I'm not sure if the verb
here should be Get so much as Set? Or more generically, Process.  c.f.
AppendPublicKeyHashes in the other platform versions of this file)

SetVerifiedCertAndMdFlagFields ?

http://codereview.chromium.org/8368015/diff/13003/net/base/x509_certificate_o...
net/base/x509_certificate_openssl.cc:524: 
Maybe it would be as well to extract the next loop into AppendPublicKeyHashes()
like on the other platforms? That way it's a bit easier to spot the high-level
steps that occur before vs after the early return for the error-cases.

[Ryan Sleevi, 2011-11-03 01:52:58]

http://codereview.chromium.org/8368015/diff/13003/net/base/x509_certificate_o...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/8368015/diff/13003/net/base/x509_certificate_o...
net/base/x509_certificate_openssl.cc:302: void GetCertChainInfo(X509_STORE_CTX*
store_ctx,
On 2011/11/01 09:31:45, joth wrote:
> Not sure if more stuff is likely to migrate into here, but maybe we could make
> the name more explicit about which info it gets? (also, I'm not sure if the
verb
> here should be Get so much as Set? Or more generically, Process.  c.f.
> AppendPublicKeyHashes in the other platform versions of this file)
> 
> SetVerifiedCertAndMdFlagFields ?

We're currently using this name for [_nss, _win, _mac], so I've left it the same
for now. I don't feel particularly strongly one way or the other re: naming,
other than we have been expanding it over time (such as this CL :p)

http://codereview.chromium.org/8368015/diff/13003/net/base/x509_certificate_o...
net/base/x509_certificate_openssl.cc:524: 
On 2011/11/01 09:31:45, joth wrote:
> Maybe it would be as well to extract the next loop into
AppendPublicKeyHashes()
> like on the other platforms? That way it's a bit easier to spot the high-level
> steps that occur before vs after the early return for the error-cases.
> 

Done.

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

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

[commit-bot: I haz the power, 2011-11-03 03:44:30]

Change committed as 108425