Make Cast certificate verification enforce constraints specified in the trusted root certificate.

Make Cast certificate verification enforce constraints specified in the trusted root certificate.

 * Hardcode the 2 Chromecast root certificates. Previously we included their Subject+SPKI, now it includes the full certificates.

 * Change the net::TrustStore to take full-blown certificates rather than (subjet, spki) tuples.

 * During verification all checks are done on the root certificate except for signature and issuer check.

Committed: https://crrev.com/55f0260050a83ff2ed9b3169bce8812a19d07b1e
Cr-Commit-Position: refs/heads/master@{#388905}

[eroman, 2016-04-15 23:37:33]

[email protected] changed reviewers:
+ [email protected]

[eroman, 2016-04-16 00:08:46]

I have some test failures to investigate, as well as adding test cases that
exercise the new behaviors.

But ready for a high level review in the meantime.

[mattm, 2016-04-16 02:40:29]

https://codereview.chromium.org/1890193003/diff/20001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator.cc (right):

https://codereview.chromium.org/1890193003/diff/20001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.cc:34: //   (1) CN=Cast Root CA
Add "(kCastRootCaDer)" ? Or maybe just combine these comments with the comments
about the includes below.

https://codereview.chromium.org/1890193003/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain.cc (right):

https://codereview.chromium.org/1890193003/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:614: size_t max_path_length =
certs_der.size();
does this still work correctly now that certs_der.size is one greater? (And the
trust anchor would usually, though not guaranteed to be, self-issued.)
Though I'm having trouble thinking of how this would break something.

Not sure if passing anchor as part of the cert chain will lead to any subtle
issues vs handling it separately like in the RFC procedure.

https://codereview.chromium.org/1890193003/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain.h (right):

https://codereview.chromium.org/1890193003/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.h:48: // trusted certificates.
I thought we still wanted to allow having trust anchors without a full cert,
from the conversations with Ryan a while ago.

https://codereview.chromium.org/1890193003/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.h:61: // but shoudl otherwise be
avoided.
should

[eroman, 2016-04-18 20:43:03]

Thanks Matt!

Some initial feedback

Note that I need to make some more updates before asking for another review
pass, so no need to look at the code again just yet.

https://codereview.chromium.org/1890193003/diff/20001/extensions/common/cast/...
File extensions/common/cast/cast_cert_validator.cc (right):

https://codereview.chromium.org/1890193003/diff/20001/extensions/common/cast/...
extensions/common/cast/cast_cert_validator.cc:34: //   (1) CN=Cast Root CA
On 2016/04/16 02:40:29, mattm wrote:
> Add "(kCastRootCaDer)" ? Or maybe just combine these comments with the
comments
> about the includes below.

Done.

https://codereview.chromium.org/1890193003/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain.cc (right):

https://codereview.chromium.org/1890193003/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:614: size_t max_path_length =
certs_der.size();
On 2016/04/16 02:40:29, mattm wrote:
> does this still work correctly now that certs_der.size is one greater? (And
the
> trust anchor would usually, though not guaranteed to be, self-issued.)
> Though I'm having trouble thinking of how this would break something.

Yes I believe so, although you are correct that the internal value of
max_path_length does change.

However when max_path_length is restricted is unchanged (it only comes into play
if there is at least one certificate in the path which limits it via a pathlen
Basic Constraint).

> 
> Not sure if passing anchor as part of the cert chain will lead to any subtle
> issues vs handling it separately like in the RFC procedure.

I believe everything works without problems.

Conceptually if the trust anchor is a certificate, it can be added to the chain
and verification is unchanged.

The questions that come up are:
  * If the root certificate is self-signed, should its signature be verified?
    The considerations here are:
    - If it is known valid when adding to trust store, then this work is
redundant
      (since the bytes should be coming from trust store)
  * If the root certificate is not self-signed, its signature cannot be verified
  * Should expiration be tested on the root certificate? (If we are considering
it trusted, does it matter whether it is expired?)


In the first draft I am:
  * not testing expiration of this root certificate obtained from trust store
  * not testing the self-signed signature

Testing the signature is probably something best done once when adding the
certificate to the trust store rather than every time the chain is verified. Or
lazily the first time it is used.

Testing the expiration I am not sure on, but will err on the side of caution and
change this CL to check expiration of the root certificate too.

https://codereview.chromium.org/1890193003/diff/20001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain.h (right):

https://codereview.chromium.org/1890193003/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.h:48: // trusted certificates.
On 2016/04/16 02:40:29, mattm wrote:
> I thought we still wanted to allow having trust anchors without a full cert,
> from the conversations with Ryan a while ago.

I spoke with Ryan and he was of the opinion that requiring trust anchors to be
full certificates was fine (and perhaps desirable).

Requiring this allows for some simplifications. For instance the constructed
paths can simply be a sequence of certificates, rather than having to
communicate what the trust anchor was separately. This is nice from an API
perspective, and also great for diagnostics.

https://codereview.chromium.org/1890193003/diff/20001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.h:61: // but shoudl otherwise be
avoided.
On 2016/04/16 02:40:29, mattm wrote:
> should

Done.

[eroman, 2016-04-19 01:31:34]

Description was changed from

==========
Make Cast certificate verification enforce constraints specified in the trusted
root certificate.

 * Hardcode the 2 Chromecast root certificates. Previously we included their
Subject+SPKI, now it includes the full certificates.
 * Change the net::TrustStore to take full-blown certificates rather than
(subjet, spki) tuples.
 * During verification most checks are skipped on these root certificates (no
signature verification or expiration check). However path length constraints and
name constraints are carried forward.
==========

to

==========
Make Cast certificate verification enforce constraints specified in the trusted
root certificate.

 * Hardcode the 2 Chromecast root certificates. Previously we included their
Subject+SPKI, now it includes the full certificates.

 * Change the net::TrustStore to take full-blown certificates rather than
(subjet, spki) tuples.

 * During verification all checks are done on the root certificate except for
signature and issuer check.
==========

[eroman, 2016-04-19 01:32:40]

Updated with some new tests.

Also changed the behavior so that expiration *is* enforced on the trusted root
certificate. Everything is checked except for its signature.

[eroman, 2016-04-19 20:11:18]

PTAL, updated TrustAnchor definition to fix test failures.

[mattm, 2016-04-19 22:33:54]

lgtm

[eroman, 2016-04-19 22:42:30]

[email protected] changed reviewers:
+ [email protected]

[eroman, 2016-04-19 22:42:31]

+sheretov to review the chromecast bits.

[sheretov, 2016-04-21 15:08:56]

lgtm

[eroman, 2016-04-21 16:36:50]

[email protected] changed reviewers:
+ [email protected]

[eroman, 2016-04-21 16:36:51]

+asargent for //extensions approval

(not adding new capabilities, just addressing some implementation TODOs).

[asargent_no_longer_on_chrome, 2016-04-21 18:05:37]

lgtm

[eroman, 2016-04-21 18:06:09]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2016-04-21 18:06:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1890193003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1890193003/140001

[commit-bot: I haz the power, 2016-04-21 19:20:42]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2016-04-21 19:20:44]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[eroman, 2016-04-21 20:10:06]

The CQ bit was checked by [email protected]

[eroman, 2016-04-21 20:10:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
[email protected], [email protected], [email protected]
Link to the patchset: https://codereview.chromium.org/1890193003/#ps180001
(title: "list datafiles for ios (needed following the rebase)")

[commit-bot: I haz the power, 2016-04-21 20:10:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1890193003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1890193003/180001

[commit-bot: I haz the power, 2016-04-21 21:24:40]

Description was changed from

==========
Make Cast certificate verification enforce constraints specified in the trusted
root certificate.

 * Hardcode the 2 Chromecast root certificates. Previously we included their
Subject+SPKI, now it includes the full certificates.

 * Change the net::TrustStore to take full-blown certificates rather than
(subjet, spki) tuples.

 * During verification all checks are done on the root certificate except for
signature and issuer check.
==========

to

==========
Make Cast certificate verification enforce constraints specified in the trusted
root certificate.

 * Hardcode the 2 Chromecast root certificates. Previously we included their
Subject+SPKI, now it includes the full certificates.

 * Change the net::TrustStore to take full-blown certificates rather than
(subjet, spki) tuples.

 * During verification all checks are done on the root certificate except for
signature and issuer check.
==========

[commit-bot: I haz the power, 2016-04-21 21:24:42]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-04-22 19:39:45]

Description was changed from

==========
Make Cast certificate verification enforce constraints specified in the trusted
root certificate.

 * Hardcode the 2 Chromecast root certificates. Previously we included their
Subject+SPKI, now it includes the full certificates.

 * Change the net::TrustStore to take full-blown certificates rather than
(subjet, spki) tuples.

 * During verification all checks are done on the root certificate except for
signature and issuer check.
==========

to

==========
Make Cast certificate verification enforce constraints specified in the trusted
root certificate.

 * Hardcode the 2 Chromecast root certificates. Previously we included their
Subject+SPKI, now it includes the full certificates.

 * Change the net::TrustStore to take full-blown certificates rather than
(subjet, spki) tuples.

 * During verification all checks are done on the root certificate except for
signature and issuer check.

Committed: https://crrev.com/55f0260050a83ff2ed9b3169bce8812a19d07b1e
Cr-Commit-Position: refs/heads/master@{#388905}
==========

[commit-bot: I haz the power, 2016-04-22 19:39:46]

Patchset 10 (id:??) landed as
https://crrev.com/55f0260050a83ff2ed9b3169bce8812a19d07b1e
Cr-Commit-Position: refs/heads/master@{#388905}