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=abarth
Committed: https://crrev.com/15faa5904d553ebff1d72969ab1b7631d7d48d78
Cr-Commit-Position: refs/heads/master@{#303286}
Trying again. abarth for OWNERS in chrome/browser/ssl. We'll also need a cert_verify_proc_unittest; coming up.
7 years, 7 months ago
(2013-08-01 19:03:53 UTC)
#1
Trying again.
abarth for OWNERS in chrome/browser/ssl.
We'll also need a cert_verify_proc_unittest; coming up.
Ryan Sleevi
Comments from the bug pasted here: I think we'll want to be careful about this, ...
7 years, 7 months ago
(2013-08-01 19:05:01 UTC)
#2
Comments from the bug pasted here:
I think we'll want to be careful about this, pending discussions in the CA/B
Forum.
I think reasonable steps to take are similar to what we've discussed for other
BR issues:
- UMA to see how frequent users are experiencing such certs
- Backend analysis to see how prevalent such certs are 'in the wild'
If the numbers indicate a 'reasonable' amount (TBD), then we do a two-phase
approach:
- Downgrade the 'security' indicator for the site (eg: mixed content)
- Show an interstitial or warning page.
I think the latter two points are fairly consistent with how Yngve and Opera
have dealt with other SSL related issues (eg: revocation checking), and provides
a reasonable balance between warning fatigue for users.
I think we'll also only want to be applying this check for certificates from
well-known roots (aka public CAs), since internal/enterprise CAs are not
'expected' to conform to the BRs - even if they are best practice.
abarth-chromium
Someone else should review this change. I'd welcome a CL to remove me from browser/ssl/OWNERS.
7 years, 7 months ago
(2013-08-01 20:57:26 UTC)
#3
Someone else should review this change. I'd welcome a CL to remove me from
browser/ssl/OWNERS.
sigbjorn
lgtm https://codereview.chromium.org/20628006/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/20628006/diff/1/chrome/app/generated_resources.grd#newcode3528 chrome/app/generated_resources.grd:3528: + Internet standards bodies have settled on a ...
7 years, 7 months ago
(2013-08-03 17:52:54 UTC)
#4
lgtm
https://codereview.chromium.org/20628006/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):
https://codereview.chromium.org/20628006/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:3528: + Internet standards bodies have
settled on a maximum validity period of 60 months. It is believed that for
certificates with validity periods longer than that, the certification authority
that issued the certificate cannot ensure that the certificate holder is still
the entity they issued the certificate to.
Suggestion:
Internet standards bodies have found that for security reasons, server
certificates should have limited lifespans. This certificate has been issued in
violation of these standards, and might not be safe.
Rationale:
10 years, 60 months, 39 months - the same message will be used for all, so don't
mention the time limit.
There are a bunch of reasons why certificates shouldn't have longer lifespans,
but the main point is that this certificate has been issued contrary to the BRs,
and therefore is suspect.
Ryan Sleevi
Just to circle back on this - we haven't abandoned it, sigbjorn. We're in the ...
7 years, 6 months ago
(2013-08-16 02:39:47 UTC)
#5
Just to circle back on this - we haven't abandoned it, sigbjorn. We're in the
process of finalizing policy regarding such certs. Thanks for your patience with
this CL, and hopefully we'll be able to move forward on this issue soon.
palmer
As Ryan says, https://cabforum.org/pipermail/public/2013-August/002135.html we'll land this in early 2014.
7 years, 6 months ago
(2013-08-19 17:46:39 UTC)
#6
https://codereview.chromium.org/20628006/diff/1/net/cert/cert_verify_proc.cc File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/20628006/diff/1/net/cert/cert_verify_proc.cc#newcode353 net/cert/cert_verify_proc.cc:353: ++month_diff; Definitely should add unittests for this logic. My ...
7 years, 6 months ago
(2013-08-19 17:57:50 UTC)
#7
On 2013/08/03 17:52:54, sigbjorn wrote: > > Suggestion: > Internet standards bodies have found that ...
7 years, 6 months ago
(2013-08-21 00:48:04 UTC)
#8
On 2013/08/03 17:52:54, sigbjorn wrote:
>
> Suggestion:
> Internet standards bodies have found that for security reasons, server
> certificates should have limited lifespans. This certificate has been issued
in
> violation of these standards, and might not be safe.
This seems fine to me. I suggest using "validity periods" instead of
"lifespans".
palmer
https://codereview.chromium.org/20628006/diff/1/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/20628006/diff/1/chrome/app/generated_resources.grd#newcode3528 chrome/app/generated_resources.grd:3528: + Internet standards bodies have settled on a maximum ...
7 years, 6 months ago
(2013-08-21 01:26:24 UTC)
#9
https://codereview.chromium.org/20628006/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):
https://codereview.chromium.org/20628006/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:3528: + Internet standards bodies have
settled on a maximum validity period of 60 months. It is believed that for
certificates with validity periods longer than that, the certification authority
that issued the certificate cannot ensure that the certificate holder is still
the entity they issued the certificate to.
On 2013/08/03 17:52:54, sigbjorn wrote:
> Suggestion:
> Internet standards bodies have found that for security reasons, server
> certificates should have limited lifespans. This certificate has been issued
in
> violation of these standards, and might not be safe.
>
> Rationale:
> 10 years, 60 months, 39 months - the same message will be used for all, so
don't
> mention the time limit.
> There are a bunch of reasons why certificates shouldn't have longer lifespans,
> but the main point is that this certificate has been issued contrary to the
BRs,
> and therefore is suspect.
Done.
https://codereview.chromium.org/20628006/diff/1/net/cert/cert_verify_proc.h
File net/cert/cert_verify_proc.h (right):
https://codereview.chromium.org/20628006/diff/1/net/cert/cert_verify_proc.h#n...
net/cert/cert_verify_proc.h:109: // For certificates issued after 1 April 2015:
39 months.
> This is not correct. After 1 April 2015, it IS permissible to be > 39 months,
> but <= 60 months, provided you meet the criteria.
>
> While I support this change in spirit, I'm not sure what ill-effects will be
> seen from the 39-month (if any), so I think we need to be careful.
Shall we just go with 60 months then?
https://codereview.chromium.org/20628006/diff/1/net/cert/cert_verify_proc.h#n...
net/cert/cert_verify_proc.h:113: // years after the BRs (i.e. by July 2019).
On 2013/08/19 17:57:50, Ryan Sleevi wrote:
> comment nit: rephrase this part without the pronouns, as "we" is unclear here.
>
> // For certificates issued before the BRs took effect, there were no
> // guidelines, but clamp them at a maximum of 10 year validity, with
> // the requirement they expire within 7 years after the effective
> // date of the BRs (i.e. by 1 July 2019).
Done. Your fixation is odd. :)
https://codereview.chromium.org/20628006/diff/1/net/cert/cert_verify_proc.h#n...
net/cert/cert_verify_proc.h:114: static bool HasTooLongValidity(const
X509Certificate& cert);
On 2013/08/19 17:57:50, Ryan Sleevi wrote:
> There's no need to make this a static function, as opposed to within an
unnamed
> namespace, since this is used (internally) by the CertVerifyProc, and isn't
> being unittested.
I see it as being like |IsHostnameNonUnique| and so on, and perhaps should be
unit-tested?
Ryan Sleevi
https://codereview.chromium.org/20628006/diff/1/net/cert/cert_verify_proc.h File net/cert/cert_verify_proc.h (right): https://codereview.chromium.org/20628006/diff/1/net/cert/cert_verify_proc.h#newcode109 net/cert/cert_verify_proc.h:109: // For certificates issued after 1 April 2015: 39 ...
7 years, 6 months ago
(2013-08-21 20:07:41 UTC)
#10
https://codereview.chromium.org/20628006/diff/1/net/cert/cert_verify_proc.h
File net/cert/cert_verify_proc.h (right):
https://codereview.chromium.org/20628006/diff/1/net/cert/cert_verify_proc.h#n...
net/cert/cert_verify_proc.h:109: // For certificates issued after 1 April 2015:
39 months.
On 2013/08/21 01:26:25, Chromium Palmer wrote:
> > This is not correct. After 1 April 2015, it IS permissible to be > 39
months,
> > but <= 60 months, provided you meet the criteria.
> >
> > While I support this change in spirit, I'm not sure what ill-effects will be
> > seen from the 39-month (if any), so I think we need to be careful.
>
> Shall we just go with 60 months then?
I'm fine landing this as the plan of record, and going back on that if
necessary.
I don't think we can safely histogram this either. We can just work to publicize
it as "It's acceptable for CAs to do so for legacy hardware/clients, but if
you're wanting to talk to Chrome, 39 months". This doesn't mean CAs are
violating the BRs though - just that Chrome's policy is security.
May affect the interstitial warnings.
https://codereview.chromium.org/20628006/diff/1/net/cert/cert_verify_proc.h#n...
net/cert/cert_verify_proc.h:114: static bool HasTooLongValidity(const
X509Certificate& cert);
On 2013/08/21 01:26:25, Chromium Palmer wrote:
> On 2013/08/19 17:57:50, Ryan Sleevi wrote:
> > There's no need to make this a static function, as opposed to within an
> unnamed
> > namespace, since this is used (internally) by the CertVerifyProc, and isn't
> > being unittested.
>
> I see it as being like |IsHostnameNonUnique| and so on, and perhaps should be
> unit-tested?
Glad you just volunteered to write unit tests ;) That was the goal :)
palmer
https://codereview.chromium.org/20628006/diff/1/net/cert/cert_verify_proc.cc File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/20628006/diff/1/net/cert/cert_verify_proc.cc#newcode360 net/cert/cert_verify_proc.cc:360: base::Time::FromString("1 Jul 2019", &Jul2019); On 2013/08/19 17:57:50, Ryan Sleevi ...
7 years, 6 months ago
(2013-08-21 22:24:15 UTC)
#11
Is it time to land this? Assuming the bots are green.
7 years, 1 month ago
(2014-01-30 20:04:51 UTC)
#12
Is it time to land this? Assuming the bots are green.
palmer
Soo... this CL is long in the tooth... rsleevi, should I resuscitate it, or is ...
6 years, 4 months ago
(2014-10-17 01:16:43 UTC)
#13
Soo... this CL is long in the tooth... rsleevi, should I resuscitate it, or is
there some alternate plan that is going to happen soon?
Ryan Sleevi
Sorry about the 'slight' delays on this x_x You're gonna blow up all the tests ...
6 years, 4 months ago
(2014-10-27 22:07:39 UTC)
#14
Sorry about the 'slight' delays on this x_x
You're gonna blow up all the tests (since they have too long validity), but the
"is issued by known root" should be sufficient to mitigate this.
https://codereview.chromium.org/20628006/diff/129001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc.cc (right):
https://codereview.chromium.org/20628006/diff/129001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:277: if (HasTooLongValidity(*cert)) {
Remind me why we aren't checking is_issued_by_known_root here?
Creating an internal cert for 30 years is pretty common and not a Bad Thing
(tm), since it's local policy.
https://codereview.chromium.org/20628006/diff/129001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:548: cert.valid_expiry().UTCExplode(&expiry);
Note that both of these can fail for some certificates (e.g. assume hostile
2038-bug exploiting certs)
Also, shouldn't you be checking that expiry > start, since we may be calling
this method for certs where it's not true? (Nothing is guaranteed in the
pre/post conditions that this is a 'valid' cert)
palmer
https://codereview.chromium.org/20628006/diff/129001/net/cert/cert_verify_proc.cc File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/20628006/diff/129001/net/cert/cert_verify_proc.cc#newcode277 net/cert/cert_verify_proc.cc:277: if (HasTooLongValidity(*cert)) { On 2014/10/27 22:07:39, Ryan Sleevi wrote: ...
6 years, 4 months ago
(2014-10-28 00:05:53 UTC)
#15
https://codereview.chromium.org/20628006/diff/129001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc.cc (right):
https://codereview.chromium.org/20628006/diff/129001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:277: if (HasTooLongValidity(*cert)) {
On 2014/10/27 22:07:39, Ryan Sleevi wrote:
> Remind me why we aren't checking is_issued_by_known_root here?
>
> Creating an internal cert for 30 years is pretty common and not a Bad Thing
> (tm), since it's local policy.
Done.
https://codereview.chromium.org/20628006/diff/129001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:548: cert.valid_expiry().UTCExplode(&expiry);
On 2014/10/27 22:07:39, Ryan Sleevi wrote:
> Note that both of these can fail for some certificates (e.g. assume hostile
> 2038-bug exploiting certs)
>
> Also, shouldn't you be checking that expiry > start, since we may be calling
> this method for certs where it's not true? (Nothing is guaranteed in the
> pre/post conditions that this is a 'valid' cert)
Done for a first pass. My immediate goal is to re-build this CL (it's so old it
doesn't apply cleanly anymore). See what you think of the up-coming patchset.
A not-yet-ready patchset is on the way. https://codereview.chromium.org/20628006/diff/229001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/20628006/diff/229001/net/base/net_error_list.h#newcode448 net/base/net_error_list.h:448: NET_ERROR(CERT_TOO_LONG_VALIDITY, -213) ...
6 years, 4 months ago
(2014-10-30 01:23:16 UTC)
#17
OK, here is some more testing. One problem: CertVerifyProcTest.IntranetHostsRejected fails because ok_cert.pem is valid for ...
6 years, 4 months ago
(2014-10-30 20:55:51 UTC)
#18
OK, here is some more testing.
One problem: CertVerifyProcTest.IntranetHostsRejected fails because ok_cert.pem
is valid for too long. If I replace it with a new ok_cert.pem with a legal
validity period, a bunch of other tests fail because the new cert is not valid.
But:
~/chromium/src $ diff net/data/ssl/certificates/ok_cert.pem
net/data/ssl/certificates/ok_cert.pem.new
[key material change...]
36,37c36,37
< Not Before: Aug 14 03:05:29 2014 GMT
< Not After : Aug 11 03:05:29 2024 GMT
---
> Not Before: Oct 30 01:08:46 2014 GMT
> Not After : Jul 26 01:08:46 2017 GMT
43,60c43,60
[signature change...]
Any clues? Its specific key or signature or something is pinned or statically
encoded somewhere?
I see e.g.:
const TestCertMetaData kCert2 = {
"ok_cert.pem", "cert:6C9DFD2CFA9885C71BE6DE0EA0CF962AC8F9131B"};
in net/http/disk_based_cert_cache_unittest.cc.
Ryan Sleevi
On 2014/10/30 20:55:51, Chromium Palmer wrote: > OK, here is some more testing. > > ...
6 years, 4 months ago
(2014-10-30 22:29:33 UTC)
#19
On 2014/10/30 20:55:51, Chromium Palmer wrote:
> OK, here is some more testing.
>
> One problem: CertVerifyProcTest.IntranetHostsRejected fails because
ok_cert.pem
> is valid for too long. If I replace it with a new ok_cert.pem with a legal
> validity period, a bunch of other tests fail because the new cert is not
valid.
Yes, ok_cert.pem is hardcoded all over the place (HSTS especially)
Don't replace ok_cert.pem. Just change IntranetHostsRejected to a new test cert
(and slap it in generate-test-certs.sh)
palmer
> Don't replace ok_cert.pem. Just change IntranetHostsRejected to a new test cert > (and slap ...
6 years, 4 months ago
(2014-10-31 20:05:23 UTC)
#20
> Don't replace ok_cert.pem. Just change IntranetHostsRejected to a new test
cert
> (and slap it in generate-test-certs.sh)
Done.
Ryan Sleevi
LGTM mod several nits that should be addressed/explained. https://codereview.chromium.org/20628006/diff/289001/net/data/ssl/certificates/61_months_after_2012_07.pem File net/data/ssl/certificates/61_months_after_2012_07.pem (right): https://codereview.chromium.org/20628006/diff/289001/net/data/ssl/certificates/61_months_after_2012_07.pem#newcode10 net/data/ssl/certificates/61_months_after_2012_07.pem:10: Subject: ...
6 years, 4 months ago
(2014-11-04 21:48:27 UTC)
#21
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/22903)
6 years, 4 months ago
(2014-11-07 19:25:28 UTC)
#26
Looks like this change caused some errors. Until April 2015 there can be 60 month ...
6 years, 4 months ago
(2014-11-09 12:12:58 UTC)
#31
Message was sent while issue was closed.
Looks like this change caused some errors. Until April 2015 there can be 60
month certs. Many sites are now throwing the NET::ERR_CERT_VALIDITY_TOO_LONG
including my university's site.
The sites I checked all recently obtained 60 month certs, good until July 2018.
For example:
https://stuadmin.flinders.edu.auhttps://www.systemshock.org
"Your connection is not private
Attackers might be trying to steal your information from
stuadmin.flinders.edu.au (for example, passwords, messages, or credit cards).
Back to safetyAdvanced
NET::ERR_CERT_VALIDITY_TOO_LONG"
levente.tamas
Why doesn't it change this in the FUTURE so that issuers have time to react ...
6 years, 3 months ago
(2014-11-10 12:35:31 UTC)
#32
Message was sent while issue was closed.
Why doesn't it change this in the FUTURE so that issuers have time to react and
reissue certs. We bought 10+ certs this year all for 5 year and they became
unusable. Also to make matters worse if HSTS was enabled then there is no bypass
making the site completely unreachable with a message:
You cannot visit XXXXX right now because the website uses HSTS. Network errors
and attacks are usually temporary, so this page will probably work later.
NET::ERR_CERT_VALIDITY_TOO_LONG
Issue 20628006: Reject certificates that are valid for too long.
(Closed)
Created 7 years, 7 months ago by palmer
Modified 6 years, 3 months ago
Reviewers: Ryan Sleevi, abarth-chromium, sigbjorn, levente.tamas, haavardm
Base URL: svn://svn.chromium.org/chrome/trunk/src
Comments: 42