Reject certificates that are valid for too long.

Reject certificates that are valid for too long.

This is in conformance with the CA/Browser Forum Baseline Requirements for
certificate issuance.

This CL is adapted from a diff provided by sigbjorn@opera.com. Thanks!

BUG=119211
TBR=phajdan.jr@chromium.org

Committed: https://crrev.com/4867d6cc021aca57e5a327b7ca8ed365485c0caa
Cr-Commit-Position: refs/heads/master@{#313603}

[palmer, 2014-11-13 00:22:06]

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

[palmer, 2014-11-13 00:22:07]

So, we need more tests. What would you like to see, rsleevi?

[Ryan Sleevi, 2014-11-13 02:29:05]

On 2014/11/13 00:22:07, Chromium Palmer wrote:
> So, we need more tests. What would you like to see, rsleevi?

First, have you fixed the dates? It's not clear.

Second, we should add tests for certificates on the good-side of the boundary
and make sure they all continue to work. The existing unit tests only test the
negative case.

We should have 100% coverage for each branch.

[palmer, 2014-11-15 01:36:30]

> First, have you fixed the dates? It's not clear.

Yes; bask in the glorious 0s at
https://codereview.chromium.org/724543002/diff/20001/net/cert/cert_verify_pro....

> Second, we should add tests for certificates on the good-side of the boundary
> and make sure they all continue to work. The existing unit tests only test the
> negative case.
> 
> We should have 100% coverage for each branch.

OK, working on it.

[Ryan Sleevi, 2014-11-22 01:01:19]

On 2014/11/15 01:36:30, Chromium Palmer wrote:
> > First, have you fixed the dates? It's not clear.
> 
> Yes; bask in the glorious 0s at
>
https://codereview.chromium.org/724543002/diff/20001/net/cert/cert_verify_pro....
> 
> > Second, we should add tests for certificates on the good-side of the
boundary
> > and make sure they all continue to work. The existing unit tests only test
the
> > negative case.
> > 
> > We should have 100% coverage for each branch.
> 
> OK, working on it.

Ping?

[palmer, 2014-11-24 19:15:14]

Yep, got side-tracked. Working on it today.

[palmer, 2014-11-25 01:48:47]

OK, take a look.

[palmer, 2014-11-25 20:57:35]

Confirmed that all the URLs mentioned in
https://code.google.com/p/chromium/issues/detail?id=431573 work with this patch,
too.

[Ryan Sleevi, 2014-11-26 12:25:36]

needs moar tests :)

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:642: }
no braces for one-liners (consistency in //net uber alles). And you know, line
647 et all.

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:655: base::Time::FromUTCExploded({ 2019, 7, 0, 1,
0, 0, 0, 0 });
Forces a lot of lib calls every time. That scared of another typo? :)

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:666: }
no brace

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:671: }
no brace

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_unittest.cc:626: DLOG(INFO) <<
"start_after_expiry.pem";
spam dvlogs are bad, mkay :)

You can use a TEST_P to iterate through this list (I like those much better,
FWIW, and consistent with the file, as it allows it to scale out to multiple
machines simultaneously), or you can use a test data structure (cert name,
expected result)  with a SCOPED_TRACE to log iterations.

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_unittest.cc:646: DLOG(INFO) << "11_year_validity.pem";
add test for 10 year validity == good

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_unittest.cc:654:
EXPECT_TRUE(CertVerifyProc::HasTooLongValidity(*forty_months));
add test for 39 months after 2015_04 == good

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_unittest.cc:659:
EXPECT_TRUE(CertVerifyProc::HasTooLongValidity(*sixty_one_months));
add test for 60 months after 2012_07 == good

[palmer, 2014-12-15 22:55:58]

New patchset coming...

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:642: }
On 2014/11/26 12:25:36, Ryan Sleevi wrote:
> no braces for one-liners (consistency in //net uber alles). And you know, line
> 647 et all.

Done.

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:655: base::Time::FromUTCExploded({ 2019, 7, 0, 1,
0, 0, 0, 0 });
> Forces a lot of lib calls every time. That scared of another typo? :)

This code is much clearer. Our bug was (in part) due to the fact that base::Time
is based on the *Windows* epoch, not the POSIX epoch — it was not *just* a
microseconds vs. seconds mistake.

This explicit format avoids all such confusion.

As for repeated calls to FromUTCExploded, these variables are declared static
and so should be constructed only once. printf debugging indeed shows this to be
the case.

(Even if it were somewhat inefficient, clarity should override in this
situation. Thankfully, we get both clarity and efficiency this time.)

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:666: }
On 2014/11/26 12:25:36, Ryan Sleevi wrote:
> no brace

Done.

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:671: }
On 2014/11/26 12:25:36, Ryan Sleevi wrote:
> no brace

Done.

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_unittest.cc:626: DLOG(INFO) <<
"start_after_expiry.pem";
> spam dvlogs are bad, mkay :)
> 
> You can use a TEST_P to iterate through this list (I like those much better,
> FWIW, and consistent with the file, as it allows it to scale out to multiple
> machines simultaneously), or you can use a test data structure (cert name,
> expected result)  with a SCOPED_TRACE to log iterations.

Oh, I didn't mean to leave them in. Removed.

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_unittest.cc:646: DLOG(INFO) << "11_year_validity.pem";
On 2014/11/26 12:25:36, Ryan Sleevi wrote:
> add test for 10 year validity == good

Done.

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_unittest.cc:654:
EXPECT_TRUE(CertVerifyProc::HasTooLongValidity(*forty_months));
On 2014/11/26 12:25:36, Ryan Sleevi wrote:
> add test for 39 months after 2015_04 == good

Done.

https://codereview.chromium.org/724543002/diff/60001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc_unittest.cc:659:
EXPECT_TRUE(CertVerifyProc::HasTooLongValidity(*sixty_one_months));
On 2014/11/26 12:25:36, Ryan Sleevi wrote:
> add test for 60 months after 2012_07 == good

Done.

[palmer, 2015-01-06 22:02:09]

Anything else needed here, rsleevi?

[Ryan Sleevi, 2015-01-22 02:04:39]

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

[Ryan Sleevi, 2015-01-22 02:04:40]

+felt for strings.

Mostly nits. I would love to see the ssl_policy change tested (e.g. via
chrome/browser/ssl/ssl_browser_tests.cc), but it seems we've been fairly
inconsistent in the past here.

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

https://codereview.chromium.org/724543002/diff/100001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:620: const char* file;
nit: const char* const

https://codereview.chromium.org/724543002/diff/100001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:638: for (size_t i = 0; i < sizeof(tests)
/ sizeof(tests[0]); ++i) {
size_t i = 0; i < arraysize(tests); ++i

https://codereview.chromium.org/724543002/diff/100001/net/data/ssl/certificat...
File net/data/ssl/certificates/README (right):

https://codereview.chromium.org/724543002/diff/100001/net/data/ssl/certificat...
net/data/ssl/certificates/README:72: 
twitter chain (& expiration)

https://codereview.chromium.org/724543002/diff/100001/net/data/ssl/certificat...
net/data/ssl/certificates/README:154: Forum Baseline Requirements are enforced.
10_year_validity
11_year_validity
39_months_after_2015_04
60_months_after_2012_07
pre_br_validity_bad_121
pre_br_validity_bad_2020
pre_br_validity_ok
start_after_expirey.pem


- reject_intranet_hosts.pem
   A certificate with a non-IANA delegated domain, which is rejected since a CA
cannot validate the applicant controls that domain.

https://codereview.chromium.org/724543002/diff/100001/net/data/ssl/scripts/ge...
File net/data/ssl/scripts/generate-test-certs.sh (right):

https://codereview.chromium.org/724543002/diff/100001/net/data/ssl/scripts/ge...
net/data/ssl/scripts/generate-test-certs.sh:213: -enddate 150402000000Z \
You inconsistently align these dates in the file. Pick a form and stick with it
:)

(Compare 212-213 with 226-227)

[palmer, 2015-01-22 20:05:05]

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

https://codereview.chromium.org/724543002/diff/100001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:620: const char* file;
On 2015/01/22 02:04:40, Ryan Sleevi wrote:
> nit: const char* const

Done.

https://codereview.chromium.org/724543002/diff/100001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:638: for (size_t i = 0; i < sizeof(tests)
/ sizeof(tests[0]); ++i) {
On 2015/01/22 02:04:40, Ryan Sleevi wrote:
> size_t i = 0; i < arraysize(tests); ++i

Done.

https://codereview.chromium.org/724543002/diff/100001/net/data/ssl/certificat...
File net/data/ssl/certificates/README (right):

https://codereview.chromium.org/724543002/diff/100001/net/data/ssl/certificat...
net/data/ssl/certificates/README:72: 
On 2015/01/22 02:04:40, Ryan Sleevi wrote:
> twitter chain (& expiration)

Done.

https://codereview.chromium.org/724543002/diff/100001/net/data/ssl/certificat...
net/data/ssl/certificates/README:154: Forum Baseline Requirements are enforced.
On 2015/01/22 02:04:40, Ryan Sleevi wrote:
> 10_year_validity
> 11_year_validity
> 39_months_after_2015_04
> 60_months_after_2012_07
> pre_br_validity_bad_121
> pre_br_validity_bad_2020
> pre_br_validity_ok
> start_after_expirey.pem
> 
> 
> - reject_intranet_hosts.pem
>    A certificate with a non-IANA delegated domain, which is rejected since a
CA
> cannot validate the applicant controls that domain.

Done.

https://codereview.chromium.org/724543002/diff/100001/net/data/ssl/scripts/ge...
File net/data/ssl/scripts/generate-test-certs.sh (right):

https://codereview.chromium.org/724543002/diff/100001/net/data/ssl/scripts/ge...
net/data/ssl/scripts/generate-test-certs.sh:213: -enddate 150402000000Z \
On 2015/01/22 02:04:40, Ryan Sleevi wrote:
> You inconsistently align these dates in the file. Pick a form and stick with
it
> :)
> 
> (Compare 212-213 with 226-227)

Done.

[felt, 2015-01-22 20:15:32]

https://codereview.chromium.org/724543002/diff/100001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_info.h (right):

https://codereview.chromium.org/724543002/diff/100001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_info.h:36: CERT_VALIDITY_TOO_LONG,
From the top:
"This enum is being histogrammed; please only add new values at the end"

Can you move this to the end, right before END_OF_ENUM?

[palmer, 2015-01-22 20:50:39]

https://codereview.chromium.org/724543002/diff/100001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_info.h (right):

https://codereview.chromium.org/724543002/diff/100001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_info.h:36: CERT_VALIDITY_TOO_LONG,
On 2015/01/22 20:15:32, felt wrote:
> From the top:
> "This enum is being histogrammed; please only add new values at the end"
> 
> Can you move this to the end, right before END_OF_ENUM?

Done.

[egm, 2015-01-22 21:06:14]

egm@chromium.org changed reviewers:
+ egm@chromium.org

[egm, 2015-01-22 21:06:15]

https://codereview.chromium.org/724543002/diff/120001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/724543002/diff/120001/chrome/app/generated_re...
chrome/app/generated_resources.grd:2702: +        You attempted to reach <ph
name="DOMAIN">&lt;strong&gt;$1<ex>paypal.com</ex>&lt;/strong&gt;</ph>, but the
server presented a certificate for which the period is too long.
Can we be more specific and include the validity period the cert exceeds? What
if we tried something like: 
"You attempted to reach <ph
name="DOMAIN">&lt;strong&gt;$1<ex>paypal.com</ex>&lt;/strong&gt;</ph>, but its
certificate has been valid longer than the 39 month maximum."

[palmer, 2015-01-22 21:15:23]

>
https://codereview.chromium.org/724543002/diff/120001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:2702: +        You attempted to reach <ph
> name="DOMAIN">&lt;strong&gt;$1<ex>paypal.com</ex>&lt;/strong&gt;</ph>, but the
> server presented a certificate for which the period is too long.
> Can we be more specific and include the validity period the cert exceeds? What
> if we tried something like: 
> "You attempted to reach <ph
> name="DOMAIN">&lt;strong&gt;$1<ex>paypal.com</ex>&lt;/strong&gt;</ph>, but its
> certificate has been valid longer than the 39 month maximum."

It would be nice, but the reason I opted not to do that is because there is no
single 39-month maximum. The logic in CertVerifyProc::HasTooLongValidity is
fairly hairy and not likely to be of interest to people reading that string
(i.e. everyday people).

However, it might very well be a good idea in the future to expose that
information in the Security panel in Dev Tools — *that* audience might want to
know those details. I added a comment to this effect in
https://code.google.com/p/chromium/issues/detail?id=421248.

[egm, 2015-01-22 22:52:53]

On 2015/01/22 21:15:23, palmer (OOO until 30-03-2015) wrote:
> >
>
https://codereview.chromium.org/724543002/diff/120001/chrome/app/generated_re...
> > chrome/app/generated_resources.grd:2702: +        You attempted to reach <ph
> > name="DOMAIN">&lt;strong&gt;$1<ex>paypal.com</ex>&lt;/strong&gt;</ph>, but
the
> > server presented a certificate for which the period is too long.
> > Can we be more specific and include the validity period the cert exceeds?
What
> > if we tried something like: 
> > "You attempted to reach <ph
> > name="DOMAIN">&lt;strong&gt;$1<ex>paypal.com</ex>&lt;/strong&gt;</ph>, but
its
> > certificate has been valid longer than the 39 month maximum."
> 
> It would be nice, but the reason I opted not to do that is because there is no
> single 39-month maximum. The logic in CertVerifyProc::HasTooLongValidity is
> fairly hairy and not likely to be of interest to people reading that string
> (i.e. everyday people).
> 
> However, it might very well be a good idea in the future to expose that
> information in the Security panel in Dev Tools — *that* audience might want to
> know those details. I added a comment to this effect in
> https://code.google.com/p/chromium/issues/detail?id=421248.

Ah, ok. Could we still rephrase the string slightly to clarify that "period" is
referring to time? Something like: "...but its certificate has been valid for
too long." WDYT?

[palmer, 2015-01-23 00:58:20]

> Ah, ok. Could we still rephrase the string slightly to clarify that "period"
is
> referring to time? Something like: "...but its certificate has been valid for
> too long." WDYT?

It isn't necessarily that the certificate *has been* valid for too long; just
that the total validity period is wacky.

How is this: "You attempted to reach <ph
name="DOMAIN">&lt;strong&gt;$1<ex>paypal.com</ex>&lt;/strong&gt;</ph>, but the
server presented a certificate whose validity period is too long to be
trustworthy."

[palmer, 2015-01-26 20:04:23]

I've been driving around the web with this build, including checking the sites
that broke before,

https://code.google.com/p/chromium/issues/detail?id=433932
https://code.google.com/p/chromium/issues/detail?id=431573

and the unit tests pass. sleevi, I'd really like to land this before I go on
leave.

[Ryan Sleevi, 2015-01-26 20:10:58]

ACK! Sorry Chris, I thought I'd LGTM'd in my previous review, since all I had
were nits, and was leaving final approval up to felt re: wording.

To be clear, LGTM % 1 nit and felt/egm's happiness on wording.

https://codereview.chromium.org/724543002/diff/140001/net/data/ssl/certificat...
File net/data/ssl/certificates/README (right):

https://codereview.chromium.org/724543002/diff/140001/net/data/ssl/certificat...
net/data/ssl/certificates/README:73: -twitter-chain.pem : A certificate chain
for twitter.com which should be
nit: - twitter-chain (space in between)

[felt, 2015-01-26 20:19:57]

On 2015/01/26 20:10:58, Ryan Sleevi wrote:
> ACK! Sorry Chris, I thought I'd LGTM'd in my previous review, since all I had
> were nits, and was leaving final approval up to felt re: wording.
> 
> To be clear, LGTM % 1 nit and felt/egm's happiness on wording.
> 
>
https://codereview.chromium.org/724543002/diff/140001/net/data/ssl/certificat...
> File net/data/ssl/certificates/README (right):
> 
>
https://codereview.chromium.org/724543002/diff/140001/net/data/ssl/certificat...
> net/data/ssl/certificates/README:73: -twitter-chain.pem : A certificate chain
> for http://twitter.com which should be
> nit: - twitter-chain (space in between)

i'll defer to egm's opinion. 

egm: are you happy with the latest text changes?

[palmer, 2015-01-26 20:24:07]

https://codereview.chromium.org/724543002/diff/140001/net/data/ssl/certificat...
File net/data/ssl/certificates/README (right):

https://codereview.chromium.org/724543002/diff/140001/net/data/ssl/certificat...
net/data/ssl/certificates/README:73: -twitter-chain.pem : A certificate chain
for twitter.com which should be
On 2015/01/26 20:10:57, Ryan Sleevi wrote:
> nit: - twitter-chain (space in between)

Done.

[egm, 2015-01-26 21:09:27]

On 2015/01/26 20:19:57, felt wrote:
> On 2015/01/26 20:10:58, Ryan Sleevi wrote:
> > ACK! Sorry Chris, I thought I'd LGTM'd in my previous review, since all I
had
> > were nits, and was leaving final approval up to felt re: wording.
> > 
> > To be clear, LGTM % 1 nit and felt/egm's happiness on wording.
> > 
> >
>
https://codereview.chromium.org/724543002/diff/140001/net/data/ssl/certificat...
> > File net/data/ssl/certificates/README (right):
> > 
> >
>
https://codereview.chromium.org/724543002/diff/140001/net/data/ssl/certificat...
> > net/data/ssl/certificates/README:73: -twitter-chain.pem : A certificate
chain
> > for http://twitter.com which should be
> > nit: - twitter-chain (space in between)
> 
> i'll defer to egm's opinion. 
> 
> egm: are you happy with the latest text changes?

The current string SGTM given our conversation Friday.

[palmer, 2015-01-26 23:23:39]

The CQ bit was checked by palmer@chromium.org

[commit-bot: I haz the power, 2015-01-26 23:24:14]

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

[commit-bot: I haz the power, 2015-01-27 01:35:28]

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

[commit-bot: I haz the power, 2015-01-27 01:35:30]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[palmer, 2015-01-28 23:01:47]

The CQ bit was checked by palmer@chromium.org

[commit-bot: I haz the power, 2015-01-28 23:03:21]

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

[commit-bot: I haz the power, 2015-01-28 23:05:40]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-01-28 23:06:51]

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