Try #2: CertVerifyProcMac: Add Keychain re-ordering hack, check CRLsets in path pruning loop.

Try #2: CertVerifyProcMac: Add Keychain re-ordering hack, check CRLsets in path pruning loop.

This also removes the native hostname checking workarounds.

BUG=570909, 588789, 621684, 647241
Committed: https://crrev.com/af868e7814f899b9be9d2d3f8231d02b9b4d5f64
Cr-Commit-Position: refs/heads/master@{#420782}

[mattm, 2016-09-22 19:35:52]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 19:36:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2016-09-22 19:36:27]

Description was changed from

==========
Try #2: CertVerifyProcMac: Add Keychain re-ordering hack, check CRLsets in path
pruning loop.

This also removes the native hostname checking workarounds.

BUG=570909,588789,621684
==========

to

==========
Try #2: CertVerifyProcMac: Add Keychain re-ordering hack, check CRLsets in path
pruning loop.

This also removes the native hostname checking workarounds.

BUG=570909,588789,621684,647241
==========

[mattm, 2016-09-22 19:42:54]

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

[mattm, 2016-09-22 19:42:55]

This fixes the error on Sierra in the previous version of the CL due to creating
a ssl options struct with empty hostname. It matches what the SecurityTool
verify_cert code does if you don't specify a hostname. Do you know if there are
any other side effects of not passing the ssl_options struct?

[Ryan Sleevi, 2016-09-22 20:53:55]

On 2016/09/22 19:42:55, mattm wrote:
> This fixes the error on Sierra in the previous version of the CL due to
creating
> a ssl options struct with empty hostname. It matches what the SecurityTool
> verify_cert code does if you don't specify a hostname. Do you know if there
are
> any other side effects of not passing the ssl_options struct?

Ah, it's all paging back in now (sorry, 3 years of not touching this delicate
snowflake = I forget)

We supplied the hostname because Keychain uses it as a key in determining
per-host exception policies for trust settings (e.g. marking in Keychain to
"Always trust this cert, but only for www.google.com"). This is what happens
when you add an exception to the Trust Settings dialog spawned by Safari. I
believe I added this code in response to a user complaining that Chrome wasn't
respecting Safari's exceptions.

Given that we have our own exception persistence layer (at a 7day window, up in
the //content layer and persisted by prefs), we should be fine. 

Out of curiousity, have you explored making sure that .ServerName == nullptr
when .size() == 0? I'm wondering if the combination of serving a pointer (but w/
len 0) is causing an issue. If you've got a 10.12 box handy, that might be worth
testing just to see.

Checking the earlier OS X releases,
https://opensource.apple.com/source/libsecurity_apple_x509_tp/libsecurity_app...
should be fine if we omit the structure - it should still perform the
id-kp-serverAuth EKU check so long as we're initializing an SSL trust policy,
even if we don't supply an SSLOptions. That's the only thing I would think might
be affected. We'll have to see when 10.12 sources are released (if they are)
what changed here.

[commit-bot: I haz the power, 2016-09-22 21:07:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 21:07:53]

Dry run: This issue passed the CQ dry run.

[mattm, 2016-09-22 22:05:25]

On 2016/09/22 20:53:55, Ryan Sleevi (slow) wrote:
> On 2016/09/22 19:42:55, mattm wrote:
> > This fixes the error on Sierra in the previous version of the CL due to
> creating
> > a ssl options struct with empty hostname. It matches what the SecurityTool
> > verify_cert code does if you don't specify a hostname. Do you know if there
> are
> > any other side effects of not passing the ssl_options struct?
> 
> Ah, it's all paging back in now (sorry, 3 years of not touching this delicate
> snowflake = I forget)
> 
> We supplied the hostname because Keychain uses it as a key in determining
> per-host exception policies for trust settings (e.g. marking in Keychain to
> "Always trust this cert, but only for http://www.google.com%22). This is what
happens
> when you add an exception to the Trust Settings dialog spawned by Safari. I
> believe I added this code in response to a user complaining that Chrome wasn't
> respecting Safari's exceptions.
> 
> Given that we have our own exception persistence layer (at a 7day window, up
in
> the //content layer and persisted by prefs), we should be fine. 
> 
> Out of curiousity, have you explored making sure that .ServerName == nullptr
> when .size() == 0? I'm wondering if the combination of serving a pointer (but
w/
> len 0) is causing an issue. If you've got a 10.12 box handy, that might be
worth
> testing just to see.

The memset 0 + the !hostname.empty() check in the old version should ensure that
name was nullptr and len is 0.. but I doublechecked to be sure. Still fails if I
explicitly set ServerName=nullptr and len=0.

> 
> Checking the earlier OS X releases,
>
https://opensource.apple.com/source/libsecurity_apple_x509_tp/libsecurity_app...
> should be fine if we omit the structure - it should still perform the
> id-kp-serverAuth EKU check so long as we're initializing an SSL trust policy,
> even if we don't supply an SSLOptions. That's the only thing I would think
might
> be affected. We'll have to see when 10.12 sources are released (if they are)
> what changed here.

Cool.

[Ryan Sleevi, 2016-09-23 20:45:09]

lgtm

[mattm, 2016-09-23 21:23:34]

The CQ bit was checked by mattm@chromium.org

[commit-bot: I haz the power, 2016-09-23 21:23:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 23:25:37]

Description was changed from

==========
Try #2: CertVerifyProcMac: Add Keychain re-ordering hack, check CRLsets in path
pruning loop.

This also removes the native hostname checking workarounds.

BUG=570909,588789,621684,647241
==========

to

==========
Try #2: CertVerifyProcMac: Add Keychain re-ordering hack, check CRLsets in path
pruning loop.

This also removes the native hostname checking workarounds.

BUG=570909,588789,621684,647241
==========

[commit-bot: I haz the power, 2016-09-23 23:25:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-23 23:27:12]

Description was changed from

==========
Try #2: CertVerifyProcMac: Add Keychain re-ordering hack, check CRLsets in path
pruning loop.

This also removes the native hostname checking workarounds.

BUG=570909,588789,621684,647241
==========

to

==========
Try #2: CertVerifyProcMac: Add Keychain re-ordering hack, check CRLsets in path
pruning loop.

This also removes the native hostname checking workarounds.

BUG=570909,588789,621684,647241

Committed: https://crrev.com/af868e7814f899b9be9d2d3f8231d02b9b4d5f64
Cr-Commit-Position: refs/heads/master@{#420782}
==========

[commit-bot: I haz the power, 2016-09-23 23:27:13]

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