Account for the OS returning an empty certificate chain.

Account for the OS returning an empty certificate chain.

OS X sometimes returns an empty certificate chain on unrecoverable certificate
chain. This was leaving the verified certificate NULL and causing crashes.

BUG=472291
Committed: https://crrev.com/a6173cd8139cac600957a6430afa7ab5ef12b550
Cr-Commit-Position: refs/heads/master@{#326683}

[davidben, 2015-04-18 00:23:40]

davidben@chromium.org changed reviewers:
+ rsleevi@chromium.org

[davidben, 2015-04-18 00:23:40]

I'm not sure this is the best fix or if we should bail early (out of the loop
entirely and skip everything else) if the OS gives an unrecoverable error. But
this does fix the crash at least.

https://codereview.chromium.org/1094983002/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/1094983002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_mac.cc:475: }
This isn't necessary, but I was a little worried about the empty chain getting
returned when it wasn't supposed it. It seemed slightly poor.

[Ryan Sleevi, 2015-04-18 00:54:58]

unit test?

https://codereview.chromium.org/1094983002/diff/60001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/1094983002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_mac.cc:472: tmp_trust_result ==
kSecTrustResultRecoverableTrustFailure)) {
Why only these status codes?

And, from your description, |tmp_trust_result| is
kSecTrustResultFatalTrustError, which wouldn't be caught by either of these
three.

https://codereview.chromium.org/1094983002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_mac.cc:593: temp_verify_result.has_md5 ||
temp_verify_result.has_sha1);
The result of this is that a chain that fails (for whatever reason) has higher
priority over a weak, but untrusted chain

This is because weak_chain would be |false| for this chain, but would be true
for the others.

Should weak_chain factor in the CFArrayGetCount() of |temp_chain| ?

[davidben, 2015-04-22 21:57:58]

(Will add a test once I figure out what's causing OS X to hate this chain. I
don't really want to add a test that asserts some input gives an unrecoverable
error when I don't know why it does that.)

https://codereview.chromium.org/1094983002/diff/60001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/1094983002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_mac.cc:472: tmp_trust_result ==
kSecTrustResultRecoverableTrustFailure)) {
On 2015/04/18 00:54:58, Ryan Sleevi wrote:
> Why only these status codes?
> 
> And, from your description, |tmp_trust_result| is
> kSecTrustResultFatalTrustError, which wouldn't be caught by either of these
> three.

You've got it backwards. These are the status codes where we DO expect a chain.

This block was just a sanity check. It's not really needed. That's not the fix.
The fix is to never clear verified_cert up at the top, which is where the
regression came from.

https://codereview.chromium.org/1094983002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_mac.cc:593: temp_verify_result.has_md5 ||
temp_verify_result.has_sha1);
On 2015/04/18 00:54:58, Ryan Sleevi wrote:
> The result of this is that a chain that fails (for whatever reason) has higher
> priority over a weak, but untrusted chain
> 
> This is because weak_chain would be |false| for this chain, but would be true
> for the others.
> 
> Should weak_chain factor in the CFArrayGetCount() of |temp_chain| ?

Hrm? A chain that fails will have untrusted = false. That gives it lower
priority over everything.

In fact, that sanity check up at the top is to prevent precisely what you're
talking about from happening. A chain that's trusted (or untrusted but
recoverable) should never be empty.

[davidben, 2015-04-23 00:09:52]

Added a test and tidied up the code to hopefully be more understandable. Notably
I made GetCertChainInfo have a non-empty chain as a precondition since that
makes a bit more sense. The callers now check for this.

I also removed the leaf_is_weak output and comment because that wasn't actually
being used. It was redundant with the is_md4, etc., checks and wasn't being used
in ranking. (Might be a useful optimization to actually use it, but *shrug*.
This way we don't replicate the list of weak hashes which is nice.)

[Ryan Sleevi, 2015-04-23 00:30:50]

Yes, it was being used.

If the leaf was weak, we didn't call the chain weak (I mean, it *was* weak, we
just didn't *call* it weak), and instead only iterated until we found a trusted
chain.

If the leaf was strong, but the chain was weak, then we iterated until we found
a trusted non-weak chain, or at least a trusted chain.

So don't delete that code. If there's a bug, it's a bug.

https://codereview.chromium.org/1094983002/diff/80001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_mac.cc (left):

https://codereview.chromium.org/1094983002/diff/80001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_mac.cc:589: // because no amount of path discovery
will fix a weak leaf.
This is a bug that we _aren't_ exiting this loop after the first pass when
leaf_is_weak, so I'd rather *not* delete all this code, and instead fix that
bug.

https://codereview.chromium.org/1094983002/diff/80001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/1094983002/diff/80001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_mac.cc:591: if (!untrusted && !weak_chain)
This is the bug.

It should have been
if (!untrusted && (leaf_is_weak || !weak_chain))


The logic was expressed somewhat inverted, in that we didn't want to call it a
"weak chain" if the leaf was also weak, because there's no amount we can do to
fix it.

[davidben, 2015-04-23 01:00:28]

On 2015/04/23 00:30:50, Ryan Sleevi wrote:
> Yes, it was being used.
> 
> If the leaf was weak, we didn't call the chain weak (I mean, it *was* weak, we
> just didn't *call* it weak), and instead only iterated until we found a
trusted
> chain.
> 
> If the leaf was strong, but the chain was weak, then we iterated until we
found
> a trusted non-weak chain, or at least a trusted chain.
> 
> So don't delete that code. If there's a bug, it's a bug.
> 
>
https://codereview.chromium.org/1094983002/diff/80001/net/cert/cert_verify_pr...
> File net/cert/cert_verify_proc_mac.cc (left):
> 
>
https://codereview.chromium.org/1094983002/diff/80001/net/cert/cert_verify_pr...
> net/cert/cert_verify_proc_mac.cc:589: // because no amount of path discovery
> will fix a weak leaf.
> This is a bug that we _aren't_ exiting this loop after the first pass when
> leaf_is_weak, so I'd rather *not* delete all this code, and instead fix that
> bug.
> 
>
https://codereview.chromium.org/1094983002/diff/80001/net/cert/cert_verify_pr...
> File net/cert/cert_verify_proc_mac.cc (right):
> 
>
https://codereview.chromium.org/1094983002/diff/80001/net/cert/cert_verify_pr...
> net/cert/cert_verify_proc_mac.cc:591: if (!untrusted && !weak_chain)
> This is the bug.
> 
> It should have been
> if (!untrusted && (leaf_is_weak || !weak_chain))
> 
> 
> The logic was expressed somewhat inverted, in that we didn't want to call it a
> "weak chain" if the leaf was also weak, because there's no amount we can do to
> fix it.

It's just an optimization, no? I gathered that was the intent, but the code was
acting as if we just never bothered implementing the optimization. I'm not sure
how important it is to bother with it, but I can restore it I suppose. I'll do
it in a separate commit because I think we ought to merge this one.

[davidben, 2015-04-23 01:05:10]

On 2015/04/23 01:00:28, David Benjamin (OOO sick) wrote:
> On 2015/04/23 00:30:50, Ryan Sleevi wrote:
> > Yes, it was being used.
> > 
> > If the leaf was weak, we didn't call the chain weak (I mean, it *was* weak,
we
> > just didn't *call* it weak), and instead only iterated until we found a
> trusted
> > chain.
> > 
> > If the leaf was strong, but the chain was weak, then we iterated until we
> found
> > a trusted non-weak chain, or at least a trusted chain.
> > 
> > So don't delete that code. If there's a bug, it's a bug.
> > 
> >
>
https://codereview.chromium.org/1094983002/diff/80001/net/cert/cert_verify_pr...
> > File net/cert/cert_verify_proc_mac.cc (left):
> > 
> >
>
https://codereview.chromium.org/1094983002/diff/80001/net/cert/cert_verify_pr...
> > net/cert/cert_verify_proc_mac.cc:589: // because no amount of path discovery
> > will fix a weak leaf.
> > This is a bug that we _aren't_ exiting this loop after the first pass when
> > leaf_is_weak, so I'd rather *not* delete all this code, and instead fix that
> > bug.
> > 
> >
>
https://codereview.chromium.org/1094983002/diff/80001/net/cert/cert_verify_pr...
> > File net/cert/cert_verify_proc_mac.cc (right):
> > 
> >
>
https://codereview.chromium.org/1094983002/diff/80001/net/cert/cert_verify_pr...
> > net/cert/cert_verify_proc_mac.cc:591: if (!untrusted && !weak_chain)
> > This is the bug.
> > 
> > It should have been
> > if (!untrusted && (leaf_is_weak || !weak_chain))
> > 
> > 
> > The logic was expressed somewhat inverted, in that we didn't want to call it
a
> > "weak chain" if the leaf was also weak, because there's no amount we can do
to
> > fix it.
> 
> It's just an optimization, no? I gathered that was the intent, but the code
was
> acting as if we just never bothered implementing the optimization. I'm not
sure
> how important it is to bother with it, but I can restore it I suppose. I'll do
> it in a separate commit because I think we ought to merge this one.

Actually, the optimization isn't *completely* right, is it? The leaf might be
weak because of SHA-1 while the chain is weak because of MD5. Hopefully that
can't actually happen nowadays, but it's the sort of thing that we'd be exposing
ourselves to, if it matters.

[Ryan Sleevi, 2015-04-23 01:46:53]

On 2015/04/23 01:00:28, David Benjamin (OOO sick) wrote:
> It's just an optimization, no? I gathered that was the intent, but the code
was
> acting as if we just never bothered implementing the optimization. 

It is an optimization, but I'm not sure why you think we didn't bother
implementing it. The logic statement may be confusing, but it is implemented.
(Namely, if the leaf is weak, weak_chain is always false, and so the only factor
is trusted vs untrusted. If the leaf is strong, weak_chain is dependent, and is
the second factor)

> I'm not sure
> how important it is to bother with it, but I can restore it I suppose. I'll do
> it in a separate commit because I think we ought to merge this one.

I'm not sure I understand this

[Ryan Sleevi, 2015-04-23 01:48:57]

On 2015/04/23 01:05:10, David Benjamin (OOO sick) wrote:
> Actually, the optimization isn't *completely* right, is it? The leaf might be
> weak because of SHA-1 while the chain is weak because of MD5. Hopefully that
> can't actually happen nowadays, but it's the sort of thing that we'd be
exposing
> ourselves to, if it matters.

Doesn't happen, doesn't matter. Or at least, making *that* scenario work was
never a goal of the CL.

(Yes, it could come up because we don't sort possible chains by the hash alg, 3
of which we'll fail the connection vs 1 we warn about, but the other 3 we've
always failed on and will continue to fail on, and are effectively gone; so just
the 1 matters)

[davidben, 2015-04-23 13:40:09]

On 2015/04/23 01:46:53, Ryan Sleevi wrote:
> On 2015/04/23 01:00:28, David Benjamin (OOO sick) wrote:
> > It's just an optimization, no? I gathered that was the intent, but the code
> was
> > acting as if we just never bothered implementing the optimization. 
> 
> It is an optimization, but I'm not sure why you think we didn't bother
> implementing it. The logic statement may be confusing, but it is implemented.
> (Namely, if the leaf is weak, weak_chain is always false, and so the only
factor
> is trusted vs untrusted. If the leaf is strong, weak_chain is dependent, and
is
> the second factor)

Oh! I'm sorry, I can't read. I thought that said leaf_is_weak || (....). Please
ignore all those comments as I'm clearly confused. Uploaded a new version that
leaves that code alone. (Also fixes some typos... the joys of building in
release mode because your Air doesn't have enough disk for debug...)

[Ryan Sleevi, 2015-04-23 14:05:41]

LGTM for timezone changes mod nits, but I mean, pay careful attention :)

https://codereview.chromium.org/1094983002/diff/100001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/1094983002/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_mac.cc:579: // If the chain is trusted or has
recoverable errors, it cannot be empty.
I find this comment actually makes the preceeding else far more confusing; it
also follows the anti-pattern of putting error handling last.

if (CFArrayGetCount(temp_chain) == 0) {
  // If the chain is empty, it must not be trusted or have recoverable errors;
  DCHECK(untrusted);
  DCHECK_NE(kSecTrustResultRecoverableTrustFailure, temp_trust_result);
} else {
  ...
}

https://codereview.chromium.org/1094983002/diff/100001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/1094983002/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_unittest.cc:1604:
GetTestCertsDirectory().AppendASCII("root_ca_cert.pem"));
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/test_root...

ScopedTestRoot test_root(ImportCertFromFile(GetTestCertsDirectory(),
"root_ca_cert.pem").get());


[Really, ScopedTestRoot should take a "const scoped_refptr<X509Certificate>&" so
that we don't have to do the .get() dance; in case it scares you to see the
.get(), the temporary lasts for the duration of the full statement (aka the full
execution of the ctor), which will handle grabbing the ref to keep things alive]

https://codereview.chromium.org/1094983002/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_unittest.cc:1609: scoped_refptr<X509Certificate>
cert(cert_list[0]);
Either ImportCertFromFile or
https://code.google.com/p/chromium/codesearch#chromium/src/net/test/cert_test...

[davidben, 2015-04-23 18:12:29]

I also had to go fix X509Certificate::GetPublicKeyInfo so PTAL.

https://codereview.chromium.org/1094983002/diff/100001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/1094983002/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_mac.cc:579: // If the chain is trusted or has
recoverable errors, it cannot be empty.
On 2015/04/23 14:05:41, Ryan Sleevi wrote:
> I find this comment actually makes the preceeding else far more confusing; it
> also follows the anti-pattern of putting error handling last.
> 
> if (CFArrayGetCount(temp_chain) == 0) {
>   // If the chain is empty, it must not be trusted or have recoverable errors;
>   DCHECK(untrusted);
>   DCHECK_NE(kSecTrustResultRecoverableTrustFailure, temp_trust_result);
> } else {
>   ...
> }

Done.

https://codereview.chromium.org/1094983002/diff/100001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/1094983002/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_unittest.cc:1604:
GetTestCertsDirectory().AppendASCII("root_ca_cert.pem"));
On 2015/04/23 14:05:41, Ryan Sleevi wrote:
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/test_root...
> 
> ScopedTestRoot test_root(ImportCertFromFile(GetTestCertsDirectory(),
> "root_ca_cert.pem").get());
> 
> 
> [Really, ScopedTestRoot should take a "const scoped_refptr<X509Certificate>&"
so
> that we don't have to do the .get() dance; in case it scares you to see the
> .get(), the temporary lasts for the duration of the full statement (aka the
full
> execution of the ctor), which will handle grabbing the ref to keep things
alive]

I just copy-and-pasted that part of the test. :-P Done.

https://codereview.chromium.org/1094983002/diff/100001/net/cert/cert_verify_p...
net/cert/cert_verify_proc_unittest.cc:1609: scoped_refptr<X509Certificate>
cert(cert_list[0]);
On 2015/04/23 14:05:41, Ryan Sleevi wrote:
> Either ImportCertFromFile or
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/test/cert_test...

Done.

[Ryan Sleevi, 2015-04-23 21:26:57]

lgtm

[davidben, 2015-04-23 22:50:22]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-04-23 22:50:22]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1094983002/#ps160001
(title: "size test cert to a multiple of 8, otherwise key size in bits is
weird")

[commit-bot: I haz the power, 2015-04-23 22:50:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1094983002/160001

[commit-bot: I haz the power, 2015-04-23 22:55:16]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-04-23 22:56:12]

Patchset 9 (id:??) landed as
https://crrev.com/a6173cd8139cac600957a6430afa7ab5ef12b550
Cr-Commit-Position: refs/heads/master@{#326683}