Work around GTE CyberTrust/Baltimore CyberTrust cross-signing issues

Work around GTE CyberTrust/Baltimore CyberTrust cross-signing issues

OS X's lack of robust support for cross-signed certificates, combined with the
impending removal of the legacy GTE CyberTrust 1024-bit root in favour of
the 2048-bit Baltimore CyberTrust Root, will soon cause issues for sites that
need to use the cross-signed intermediate. Fix up the chain on the fly when
dealing with such sites.

BUG=236112
TEST=net_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206274

[Ryan Sleevi, 2013-04-27 03:33:51]

wtc: PTAL.

You can run the unittests without my "Retry" code to see the failure (eg: as it
behaves today).

I'm not entirely convinced this is the right approach - see the thread talking
about whether we really do want to match Safari bug-for-bug - but this is at
least a minimal way to avoid double verifications for EVERY failure.

Thoughts?

[wtc, 2013-04-29 19:14:57]

Patch set 2 LGTM.

https://codereview.chromium.org/14492003/diff/2001/net/cert/cert_verify_proc_...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/14492003/diff/2001/net/cert/cert_verify_proc_...
net/cert/cert_verify_proc_mac.cc:347: // |verified_chain|, and |chain_info| with
the verification results.

It would be useful to point out that on failure, this function
does not modify the output arguments. We rely on this property
on lines 510-513.

https://codereview.chromium.org/14492003/diff/2001/net/cert/cert_verify_proc_...
net/cert/cert_verify_proc_mac.cc:508: CFRangeMake(0, slice_point));

Another option is to only remove the cross-signed certs
from the cert_array, i.e., only slice_point is removed.
Would that work?

https://codereview.chromium.org/14492003/diff/2001/net/cert/test_root_certs_m...
File net/cert/test_root_certs_mac.cc (right):

https://codereview.chromium.org/14492003/diff/2001/net/cert/test_root_certs_m...
net/cert/test_root_certs_mac.cc:86: allow_system_trust_ = true;

Is it convenient to initialize allow_system_trust_ in the
constructor? Otherwise tools such as Coverity may report
an uninitialized class member.

https://codereview.chromium.org/14492003/diff/2001/net/data/ssl/certificates/...
File net/data/ssl/certificates/cybertrust_baltimore_cross_certified_1.pem
(right):

https://codereview.chromium.org/14492003/diff/2001/net/data/ssl/certificates/...
net/data/ssl/certificates/cybertrust_baltimore_cross_certified_1.pem:1:
Certificate:

Please describe these certificates in the README file in
net/data/ssl/certificates.

[Ryan Sleevi, 2013-04-29 21:28:23]

https://codereview.chromium.org/14492003/diff/2001/net/cert/cert_verify_proc_...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/14492003/diff/2001/net/cert/cert_verify_proc_...
net/cert/cert_verify_proc_mac.cc:347: // |verified_chain|, and |chain_info| with
the verification results.
On 2013/04/29 19:14:57, wtc wrote:
> 
> It would be useful to point out that on failure, this function
> does not modify the output arguments. We rely on this property
> on lines 510-513.

Good point, will do.

https://codereview.chromium.org/14492003/diff/2001/net/cert/cert_verify_proc_...
net/cert/cert_verify_proc_mac.cc:508: CFRangeMake(0, slice_point));
On 2013/04/29 19:14:57, wtc wrote:
> 
> Another option is to only remove the cross-signed certs
> from the cert_array, i.e., only slice_point is removed.
> Would that work?

Well, the only cert(s) that should follow slice_point would be the root (that
shouldn't be sent).

I'm trying to understand what the advantage here would be?

https://codereview.chromium.org/14492003/diff/2001/net/cert/test_root_certs_m...
File net/cert/test_root_certs_mac.cc (right):

https://codereview.chromium.org/14492003/diff/2001/net/cert/test_root_certs_m...
net/cert/test_root_certs_mac.cc:86: allow_system_trust_ = true;
On 2013/04/29 19:14:57, wtc wrote:
> 
> Is it convenient to initialize allow_system_trust_ in the
> constructor? Otherwise tools such as Coverity may report
> an uninitialized class member.

No. We use |Init()| to handle the per-OS initialization of members.

This is the same as how empty_ is treated, and it doesn't trigger any warnings.
Init() is called by the CTOR, FWIW.

[wtc, 2013-04-29 22:53:42]

https://codereview.chromium.org/14492003/diff/2001/net/cert/cert_verify_proc_...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/14492003/diff/2001/net/cert/cert_verify_proc_...
net/cert/cert_verify_proc_mac.cc:508: CFRangeMake(0, slice_point));

On 2013/04/29 21:28:23, Ryan Sleevi wrote:
> 
> Well, the only cert(s) that should follow slice_point would be the root (that
> shouldn't be sent).
> 
> I'm trying to understand what the advantage here would be?

You are right. I didn't think this through. I thought that
there might be useful certs after the cross-signed certs.

https://codereview.chromium.org/14492003/diff/2001/net/cert/test_root_certs_m...
File net/cert/test_root_certs_mac.cc (right):

https://codereview.chromium.org/14492003/diff/2001/net/cert/test_root_certs_m...
net/cert/test_root_certs_mac.cc:86: allow_system_trust_ = true;

On 2013/04/29 21:28:23, Ryan Sleevi wrote:
>
> Init() is called by the CTOR, FWIW.

I see. I didn't know that (I didn't check test_root_certs.cc)
when I reviewed the CL. Sorry about my confusion.

[Ryan Sleevi, 2013-05-01 00:06:17]

wtc: Do you mind looking at the diff between PS2 & 4 for TestRootCerts, since it
had to change (due to SecTrustSetAnchorCertificatesOnly being unimplemented on
OS X 10.6)

Handy diff link -
https://codereview.chromium.org/14492003/diff2/2001:14001/net/cert/test_root_...

[wtc, 2013-05-01 00:53:26]

Patch set 4 LGTM. Thanks.

https://codereview.chromium.org/14492003/diff/14001/net/cert/test_root_certs_...
File net/cert/test_root_certs_mac.cc (right):

https://codereview.chromium.org/14492003/diff/14001/net/cert/test_root_certs_...
net/cert/test_root_certs_mac.cc:91: // creating a copy of the system roots and
merging with temporary_roots_.

Nit: change "Do so by emulating" to "Emulate"?

Using "by" twice in the same sentence is a little verbose.

[commit-bot: I haz the power, 2013-06-13 23:18:47]

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

[commit-bot: I haz the power, 2013-06-14 02:48:31]

Change committed as 206274