Distrust new WoSign/StartCom certificates

Distrust new WoSign/StartCom certificates

As announced at
https://security.googleblog.com/2016/10/distrusting-wosign-and-startcom.html

BUG=661003
R=davidben@chromium.org
TEST=https://notbefore-after-21st-test.samspin.net/ does not load

Committed: https://crrev.com/600f3a8217ed36654e9c10a778ca2de1480cb36a
Cr-Commit-Position: refs/heads/master@{#432755}

[Ryan Sleevi, 2016-11-16 09:19:46]

David: Please take a look.

The only unit test I plan to add is a synthetic before Oct-21 and after Oct-21
dummy cert to the cert_verify_proc_whitelist. I couldn't think of any other
tests to add, because I specifically wanted to avoid depending on CertVerifyProc
(as OS-level changes will take effect)

Manual testing is at https://notbefore-after-21st-test.samspin.net/

[Ryan Sleevi, 2016-11-16 09:19:57]

Description was changed from

==========
Distrust new WoSign/StartCom certificates

As announced at
https://security.googleblog.com/2016/10/distrusting-wosign-and-startcom.html

BUG=661003
R=davidben@chromium.org
==========

to

==========
Distrust new WoSign/StartCom certificates

As announced at
https://security.googleblog.com/2016/10/distrusting-wosign-and-startcom.html

BUG=661003
R=davidben@chromium.org
TEST=https://notbefore-after-21st-test.samspin.net/ does not load
==========

[davidben, 2016-11-16 09:52:31]

https://codereview.chromium.org/2509613002/diff/1/net/cert/cert_verify_proc_w...
File net/cert/cert_verify_proc_whitelist.cc (right):

https://codereview.chromium.org/2509613002/diff/1/net/cert/cert_verify_proc_w...
net/cert/cert_verify_proc_whitelist.cc:19: const uint8_t
g_wosign_keys[][crypto::kSHA256Length] = {
Nit: kWoSignKeys?

https://codereview.chromium.org/2509613002/diff/1/net/cert/cert_verify_proc_w...
net/cert/cert_verify_proc_whitelist.cc:1939: base::Time::UnixEpoch() +
base::TimeDelta::FromSeconds(1477008000);
[confirmed]

https://codereview.chromium.org/2509613002/diff/1/net/cert/cert_verify_proc_w...
net/cert/cert_verify_proc_whitelist.cc:1948: (cert.valid_start().is_null() ||
cert.valid_start().is_max() ||
Why the explicit is_max check? valid_start's documentation suggests is_null is a
special value, but doesn't say anything about is_max.

https://codereview.chromium.org/2509613002/diff/1/net/cert/cert_verify_proc_w...
net/cert/cert_verify_proc_whitelist.cc:1950: return false;
return true? This function returns true for bad certificates.

https://codereview.chromium.org/2509613002/diff/1/net/cert/cert_verify_proc_w...
net/cert/cert_verify_proc_whitelist.cc:1966: return false;
Not that it matters since a chain presumably won't include both WoSign/StartCom
and CNNIC, but should this be break so it doesn't short-circuit the
WoSign/StartCom check on other hashes?

[Ryan Sleevi, 2016-11-16 11:07:51]

Updated

https://codereview.chromium.org/2509613002/diff/1/net/cert/cert_verify_proc_w...
File net/cert/cert_verify_proc_whitelist.cc (right):

https://codereview.chromium.org/2509613002/diff/1/net/cert/cert_verify_proc_w...
net/cert/cert_verify_proc_whitelist.cc:1948: (cert.valid_start().is_null() ||
cert.valid_start().is_max() ||
On 2016/11/16 09:52:31, davidben (at IETF until 11-19) wrote:
> Why the explicit is_max check? valid_start's documentation suggests is_null is
a
> special value, but doesn't say anything about is_max.

It reflects the other date-based checks re: certificates. If a date was > the
max representation (e.g. 2038 certs), it gets treated as in scope of the policy.
For something like last_wosign_cert it's not necessary, because that's < 2038.

Here, I'm torn between cargo culting and pragmatism, and can easily be swayed by
saying it's an unrealistic cost (even if the compiler would be able to fold in
the check, expand 1938-1939, and de-dup the comparison)

https://codereview.chromium.org/2509613002/diff/1/net/cert/cert_verify_proc_w...
net/cert/cert_verify_proc_whitelist.cc:1950: return false;
On 2016/11/16 09:52:30, davidben (at IETF until 11-19) wrote:
> return true? This function returns true for bad certificates.

Should have waited until I had tests in place :) Yes, excellent catch on a
critical bug :)

https://codereview.chromium.org/2509613002/diff/1/net/cert/cert_verify_proc_w...
net/cert/cert_verify_proc_whitelist.cc:1966: return false;
On 2016/11/16 09:52:30, davidben (at IETF until 11-19) wrote:
> Not that it matters since a chain presumably won't include both
WoSign/StartCom
> and CNNIC, but should this be break so it doesn't short-circuit the
> WoSign/StartCom check on other hashes?

It was largely a concern about performance and the need to repeatedly scan the
whitelist. I went ahead and filed http://crbug.com/665816 to track this, if only
because it seems we (... read, me) keeps adding O(N^2) algorithms because N<10,
but I'm concerned in aggregate I might be adding more cuts.

[Ryan Sleevi, 2016-11-16 11:14:58]

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

[commit-bot: I haz the power, 2016-11-16 11:15:22]

Dry run: 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-11-16 11:18:30]

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

[commit-bot: I haz the power, 2016-11-16 11:18:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ryan Sleevi, 2016-11-16 11:22:52]

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

[commit-bot: I haz the power, 2016-11-16 11:23:18]

Dry run: 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-11-16 11:26:25]

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

[commit-bot: I haz the power, 2016-11-16 11:26:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ryan Sleevi, 2016-11-16 11:54:35]

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

[commit-bot: I haz the power, 2016-11-16 11:55:01]

Dry run: 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-11-16 12:29:35]

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

[commit-bot: I haz the power, 2016-11-16 12:29:36]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Ryan Sleevi, 2016-11-16 12:54:44]

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

[commit-bot: I haz the power, 2016-11-16 12:55:04]

Dry run: 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-11-16 14:46:56]

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

[commit-bot: I haz the power, 2016-11-16 14:46:56]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[davidben, 2016-11-16 15:56:40]

lgtm

https://codereview.chromium.org/2509613002/diff/1/net/cert/cert_verify_proc_w...
File net/cert/cert_verify_proc_whitelist.cc (right):

https://codereview.chromium.org/2509613002/diff/1/net/cert/cert_verify_proc_w...
net/cert/cert_verify_proc_whitelist.cc:1948: (cert.valid_start().is_null() ||
cert.valid_start().is_max() ||
On 2016/11/16 11:07:51, Ryan Sleevi (at IETF til 11-20 wrote:
> On 2016/11/16 09:52:31, davidben (at IETF until 11-19) wrote:
> > Why the explicit is_max check? valid_start's documentation suggests is_null
is
> a
> > special value, but doesn't say anything about is_max.
> 
> It reflects the other date-based checks re: certificates. If a date was > the
> max representation (e.g. 2038 certs), it gets treated as in scope of the
policy.
> For something like last_wosign_cert it's not necessary, because that's < 2038.
> 
> Here, I'm torn between cargo culting and pragmatism, and can easily be swayed
by
> saying it's an unrealistic cost (even if the compiler would be able to fold in
> the check, expand 1938-1939, and de-dup the comparison)

Oh, I was less worried about cost and just about clarity. Although I'm still not
sure I follow why, semantically, we'd want this. Wouldn't the point of using a
saturating value like max be that we never need to explicitly check it?

Anyway, I don't actually care. It just struck me as odd when I was reading it.

[Ryan Sleevi, 2016-11-16 22:17:49]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2016-11-16 22:19:30]

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-11-17 01:35:39]

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

[commit-bot: I haz the power, 2016-11-17 01:35:40]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Ryan Sleevi, 2016-11-17 01:38:21]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2016-11-17 01:39:22]

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-11-17 05:10:07]

Description was changed from

==========
Distrust new WoSign/StartCom certificates

As announced at
https://security.googleblog.com/2016/10/distrusting-wosign-and-startcom.html

BUG=661003
R=davidben@chromium.org
TEST=https://notbefore-after-21st-test.samspin.net/ does not load
==========

to

==========
Distrust new WoSign/StartCom certificates

As announced at
https://security.googleblog.com/2016/10/distrusting-wosign-and-startcom.html

BUG=661003
R=davidben@chromium.org
TEST=https://notbefore-after-21st-test.samspin.net/ does not load
==========

[commit-bot: I haz the power, 2016-11-17 05:10:08]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-17 05:15:17]

Description was changed from

==========
Distrust new WoSign/StartCom certificates

As announced at
https://security.googleblog.com/2016/10/distrusting-wosign-and-startcom.html

BUG=661003
R=davidben@chromium.org
TEST=https://notbefore-after-21st-test.samspin.net/ does not load
==========

to

==========
Distrust new WoSign/StartCom certificates

As announced at
https://security.googleblog.com/2016/10/distrusting-wosign-and-startcom.html

BUG=661003
R=davidben@chromium.org
TEST=https://notbefore-after-21st-test.samspin.net/ does not load

Committed: https://crrev.com/600f3a8217ed36654e9c10a778ca2de1480cb36a
Cr-Commit-Position: refs/heads/master@{#432755}
==========

[commit-bot: I haz the power, 2016-11-17 05:15:18]

Patchset 5 (id:??) landed as
https://crrev.com/600f3a8217ed36654e9c10a778ca2de1480cb36a
Cr-Commit-Position: refs/heads/master@{#432755}