Make OS X path "building" more buildy, less breaky

Make OS X path "building" more buildy, less breaky

For the reasons described within the code itself, OS X can be a bit tetchy
when it comes to PKI path building. Rather than blow up in users' face with
an error, or warn them about weak signatures when strong signatures exist,
take a performance hit and do something similar to Safari, which is to
assume the OS APIs are broken/won't do the right thing, and try to fix up
the inputs prior to giving to the Security.framework.

In this case, it means trying to verify the cert as supplied (the existing
behaviour), and if that fails / gives something undesirable, begin cutting
off certs given to the OS and retrying until it gives something better or
there are no more certs to amputate.

This causes an (unmeasured) perf hit, but only for situations that would
fail for users today, or would yell at them tomorrow, so overall, it's
a worthy tradeoff.

BUG=438653, 434914, 440267
TEST= https://github.com works. https://cacert.omniroot.com works. Unit tests are happy.

Committed: https://crrev.com/a3fa5418ffa25d5064423f0bc78e6fce2d43f83b
Cr-Commit-Position: refs/heads/master@{#314641}

[Ryan Sleevi, 2015-01-31 02:19:54]

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

[Ryan Sleevi, 2015-01-31 02:19:55]

David: Could you do a wtc-style review on this and carefully double check my
logic. I had to think carefully here, and I'm still nervous I missed something
glaringly obvious.

I discussed this with Adam, and tried to put all of the context in the code to
explain the complexity. This generalizes the fix from
https://code.google.com/p/chromium/issues/detail?id=236112 , which you can see
is now manifesting for other roots for 10.9/10.10, and also tries its damnedest
to find a !SHA-1 path when possible.

Eric, FYI since I thought you might be interested

[Ryan Sleevi, 2015-01-31 02:23:03]

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
File net/cert/cert_verify_proc_mac.cc (left):

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:461: // cross-certified certificate and remove
it from certificate chain processing.
Sadly, overgeneralizing is the "right" answer here :(

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:185: bool* leaf_is_weak) {
This is just so I can shortcircuit the loop below. If the leaf is weak, no
amount of lopping certs off will change the result that the overall chain is
weak.

[davidben, 2015-02-02 22:13:55]

Most of the comments are of the "I don't quite understand how this chopping
affects this situation" form.

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:185: bool* leaf_is_weak) {
On 2015/01/31 02:23:03, Ryan Sleevi wrote:
> This is just so I can shortcircuit the loop below. If the leaf is weak, no
> amount of lopping certs off will change the result that the overall chain is
> weak.

("This" is referring to leaf_is_week and not the resets at the bottom, right?)

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:527: //  
https://blog.digicert.com/expired-intermediate-certificate/
So, this was an example of DigiCert being now in the trust store, but previously
they had an intermediate, now expired, signed by some other key? I could be
misremembering, but I think the sites in question would send

   Leaf -> Something -> DigiCert (with DigiCert omitted on the wire because it's
a self-signed root)

But OS X would try to build something like

  Leaf -> Something -> DigiCert(expired) -> SomethingElse

so it used the DigiCert(expired) signed by SomethingElse in its store rather
than the self-signed DigiCert root also in the store.

(Is that correct?)

In that case, how does chopping off the end help there? Is it that once you chop
down to the leaf, OS X would redundantly fetch Something and then use different
logic to end up at the right one?

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:533: //   chain.
I'm not sure I understand how this case relates to the loop. Is it that the
server sends[*] A -> B -> C -> D where C is signed by D with SHA-1, and OS X
doesn't know to stop? Or is the server still sending the A -> B -> C chain and
OS X is adding an intermediate it saw somewhere else? If the former, I would
have thought the solution would be for the server to send a different chain. If
the latter, why does chopping bits off the chain help?

[*] Assume "sends" here accounts for you wanting to omit the self-signed root,
so "sending" A -> B -> C -> D means you send only three certs, sign_B(A),
sign_C(B), sign_D(C).

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:546: // the peer's certificate remains.
Safari's strategy is to chop off all but the leaf, right? Are there known cases
where AIA chasing from the leaf fails while this does?

I'm not totally clear on how chopping certs off the end interacts with AIA. Is
the general behavior that OS X AIA chases from the end of the supplied chain and
never backtracks?

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:552: CFArrayGetCount(cert_array) > 0) {
Think it's worth UMA to measure how usage in the wild of this crazy looks?  Not
sure what you'd most usefully measure though.

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:562: return rv;
A network error in AIA chasing will cause this to fail even if we had a chain
from a previous iteration, right? That seems not ideal if the old chain just had
SHA-1 but otherwise was still fine, but that might be more trouble than is
worth---the SHA-1 case has a time limit and all other cases would have failed
anyway.

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:569: bool untrusted = (temp_trust_result !=
kSecTrustResultUnspecified &&
[Confirmed that we consider Proceed and Unspecified as the same elsewhere. (Is
that what Safari does too?)]

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:576: (candidate_weak && !weak_chain)))) {
This condition was slightly hard to follow. Not sure if an English language
comment would help though.

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:585: if (!untrusted && !weak_chain)
Isn't this redundant with the while loop condition? It just saves one CFArray
operation.

If our new thing is trusted and not weak then, we should have set that to the
candidate. And so candidate_untrusted and candidate_weak are false, which breaks
out of the loop. Or, in logic, that condition turns into:

  if (!trust_ref || (true && (candidate_untrusted || (candidate_weak && true))))
  if (!trust_ref || candidate_untrusted || candidate_weak)

So either candidate_untrusted and candidate_weak are already both false (in
which case we would have broken out of the loop already anyway), or we'll set
them to untrusted/weak_chain and they'll become both false. So we'll break out
of the loop.

[Ryan Sleevi, 2015-02-02 22:36:01]

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:185: bool* leaf_is_weak) {
On 2015/02/02 22:13:55, David Benjamin wrote:
> On 2015/01/31 02:23:03, Ryan Sleevi wrote:
> > This is just so I can shortcircuit the loop below. If the leaf is weak, no
> > amount of lopping certs off will change the result that the overall chain is
> > weak.
> 
> ("This" is referring to leaf_is_week and not the resets at the bottom, right?)

Right, sorry. Yes, adding leaf is weak was to make the shortcircuiting easier.

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:527: //  
https://blog.digicert.com/expired-intermediate-certificate/
On 2015/02/02 22:13:55, David Benjamin wrote:
> So, this was an example of DigiCert being now in the trust store, but
previously
> they had an intermediate, now expired, signed by some other key? I could be
> misremembering, but I think the sites in question would send
> 
>    Leaf -> Something -> DigiCert (with DigiCert omitted on the wire because
it's
> a self-signed root)
> 
> But OS X would try to build something like
> 
>   Leaf -> Something -> DigiCert(expired) -> SomethingElse
> 
> so it used the DigiCert(expired) signed by SomethingElse in its store rather
> than the self-signed DigiCert root also in the store.
> 
> (Is that correct?)
> 
> In that case, how does chopping off the end help there? Is it that once you
chop
> down to the leaf, OS X would redundantly fetch Something and then use
different
> logic to end up at the right one?

If I recall correctly (and I sent out a ping to my contacts at DigiCert), the
chain is something like

Leaf -> Something(A) -> DigiCert(expired) -> DigiCert [old?] root
Leaf -> Something(B) -> DigiCert(other intermediate) -> DigiCert [new?] root

The lopping here forces a fetch of Something via Leaf's AIA, which picks up the
new path.

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:533: //   chain.
On 2015/02/02 22:13:54, David Benjamin wrote:
> I'm not sure I understand how this case relates to the loop. Is it that the
> server sends[*] A -> B -> C -> D where C is signed by D with SHA-1, and OS X
> doesn't know to stop? Or is the server still sending the A -> B -> C chain and
> OS X is adding an intermediate it saw somewhere else? If the former, I would
> have thought the solution would be for the server to send a different chain.
If
> the latter, why does chopping bits off the chain help?
> 
> [*] Assume "sends" here accounts for you wanting to omit the self-signed root,
> so "sending" A -> B -> C -> D means you send only three certs, sign_B(A),
> sign_C(B), sign_D(C).

if I understood the question correctly -
If OS X trusts both C and D, and the server sends sign_D(C), then it will build
the path to D, even if it could have stopped with C.

This results in the signature for sign_D(C) affecting the security calculation.
By chopping of sign_D(C) from the inputs, it looks in the trust store when
evaluating sign_C(B), finds C, and stops there.

Now, the reason why you need to send sign_D(C) is for legacy/non-AIA devices
that trust D - e.g. Android, Firefox. If you just sent A -> B and they only
trust D, they would fail to build a path. However, they're also smart enough
that if you're on a new Android/Firefox, and you send sign_D(C), they'll ignore
it in favour of sign_C(C) [aka the new root]

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:546: // the peer's certificate remains.
On 2015/02/02 22:13:55, David Benjamin wrote:
> Safari's strategy is to chop off all but the leaf, right? Are there known
cases
> where AIA chasing from the leaf fails while this does?

Yup. Our unittest for GTE CyberTrust is an example where the leaf doesn't have
an AIA.

> 
> I'm not totally clear on how chopping certs off the end interacts with AIA. Is
> the general behavior that OS X AIA chases from the end of the supplied chain
and
> never backtracks?

Correct. OS X also has a little logic, in that it will attempt to lop off any
expired certs explicitly sent by the peer.

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:552: CFArrayGetCount(cert_array) > 0) {
On 2015/02/02 22:13:54, David Benjamin wrote:
> Think it's worth UMA to measure how usage in the wild of this crazy looks? 
Not
> sure what you'd most usefully measure though.

Eh, once we have our own chain building, we could look to avoid this crazy. As
it stands, the Apple folks know my gripe about this behaviour since 10.4, so I'm
not sure what we'd do different.

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:562: return rv;
On 2015/02/02 22:13:55, David Benjamin wrote:
> A network error in AIA chasing will cause this to fail even if we had a chain
> from a previous iteration, right?

No. rv would == OK, temp_trust_result == kSecTrustResultUnspecified, and
temp_chain_info would have status bits of AIA failure.

> That seems not ideal if the old chain just had
> SHA-1 but otherwise was still fine, but that might be more trouble than is
> worth---the SHA-1 case has a time limit and all other cases would have failed
> anyway.

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:569: bool untrusted = (temp_trust_result !=
kSecTrustResultUnspecified &&
On 2015/02/02 22:13:55, David Benjamin wrote:
> [Confirmed that we consider Proceed and Unspecified as the same elsewhere. (Is
> that what Safari does too?)]

https://developer.apple.com/library/mac/qa/qa1360/_index.html

kSecTrustResultUnspecified = no user explicit trust settings set, but it's
trusted
kSecTrustResultProceed = explicit user trust settings to always trust

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:585: if (!untrusted && !weak_chain)
On 2015/02/02 22:13:54, David Benjamin wrote:
> Isn't this redundant with the while loop condition? It just saves one CFArray
> operation.
> 
> If our new thing is trusted and not weak then, we should have set that to the
> candidate. And so candidate_untrusted and candidate_weak are false, which
breaks
> out of the loop. Or, in logic, that condition turns into:
> 
>   if (!trust_ref || (true && (candidate_untrusted || (candidate_weak &&
true))))
>   if (!trust_ref || candidate_untrusted || candidate_weak)
> 
> So either candidate_untrusted and candidate_weak are already both false (in
> which case we would have broken out of the loop already anyway), or we'll set
> them to untrusted/weak_chain and they'll become both false. So we'll break out
> of the loop.

Yeah. Just saves a CFArray operation. I originally had much more explicit
conditionals (with duplicated code), then coalesced to avoid the redundancy.

[davidben, 2015-02-03 22:39:53]

lgtm with comment.

I'm still not sure how this helps the DigiCert expired intermediate case
specifically, but we can continue that out-of-band.

I do recall Safari being able to view some of the affected sites (but not all)
when playing with it briefly a while back, so evidently whatever it was, some
kind of chopping did help.

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:533: //   chain.
On 2015/02/02 22:36:01, Ryan Sleevi wrote:
> On 2015/02/02 22:13:54, David Benjamin wrote:
> > I'm not sure I understand how this case relates to the loop. Is it that the
> > server sends[*] A -> B -> C -> D where C is signed by D with SHA-1, and OS X
> > doesn't know to stop? Or is the server still sending the A -> B -> C chain
and
> > OS X is adding an intermediate it saw somewhere else? If the former, I would
> > have thought the solution would be for the server to send a different chain.
> If
> > the latter, why does chopping bits off the chain help?
> > 
> > [*] Assume "sends" here accounts for you wanting to omit the self-signed
root,
> > so "sending" A -> B -> C -> D means you send only three certs, sign_B(A),
> > sign_C(B), sign_D(C).
> 
> if I understood the question correctly -
> If OS X trusts both C and D, and the server sends sign_D(C), then it will
build
> the path to D, even if it could have stopped with C.
> 
> This results in the signature for sign_D(C) affecting the security
calculation.
> By chopping of sign_D(C) from the inputs, it looks in the trust store when
> evaluating sign_C(B), finds C, and stops there.
> 
> Now, the reason why you need to send sign_D(C) is for legacy/non-AIA devices
> that trust D - e.g. Android, Firefox. If you just sent A -> B and they only
> trust D, they would fail to build a path. However, they're also smart enough
> that if you're on a new Android/Firefox, and you send sign_D(C), they'll
ignore
> it in favour of sign_C(C) [aka the new root]

Okay. So this is another variant of the first example, but with SHA-1 instead of
1024-bit roots? They couldn't get a new sign_D(C) issued that used SHA-256
instead?

https://codereview.chromium.org/886223002/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:585: if (!untrusted && !weak_chain)
Maybe remove the condition from the top, if you really want to save that
CFArrayRemoveValueAtIndex call? :-) It's confusing to have the same thing
checked twice (especially two equivalent spellings of the same thing).

[Ryan Sleevi, 2015-02-04 20:43:28]

New patchsets have been uploaded after l-g-t-m from davidben@chromium.org

[Ryan Sleevi, 2015-02-04 20:44:04]

David: Do these comment updates address your concerns?

[davidben, 2015-02-04 20:48:11]

lgtm

[Ryan Sleevi, 2015-02-04 20:59:55]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2015-02-04 21:00:33]

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

[commit-bot: I haz the power, 2015-02-04 21:37:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-04 21:39:19]

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