Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(6)

Issue 20628006: Reject certificates that are valid for too long. (Closed)

Created:
7 years, 4 months ago by palmer
Modified:
6 years, 1 month ago
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cbentzel+watch_chromium.org, jam, mmenke
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 15

Patch Set 2 : Add a test, respond to comments. #

Patch Set 3 : Oops, was missing some glue code. #

Patch Set 4 : Rebase and resolve conflicts. #

Patch Set 5 : Rebase?! In our moment of triumph?! #

Total comments: 4

Patch Set 6 : "Manual rebase" due to age. #

Total comments: 10

Patch Set 7 : Respond to comments, fix some tests. #

Patch Set 8 : More and better testing; some cleanup. #

Patch Set 9 : Make a new cert for IntranetHostsRejected. Tests pass now. #

Total comments: 12

Patch Set 10 : Don't use arithmetic expressions in shell script. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+831 lines, -268 lines) Patch
M chrome/app/generated_resources.grd View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/ssl/ssl_error_info.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ssl/ssl_error_info.cc View 1 2 3 4 5 6 7 4 chunks +33 lines, -21 lines 0 comments Download
M content/browser/ssl/ssl_policy.cc View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M net/base/net_error_list.h View 1 2 3 4 5 6 1 chunk +4 lines, -1 line 0 comments Download
M net/cert/cert_status_flags.cc View 1 2 3 4 5 6 2 chunks +4 lines, -0 lines 0 comments Download
M net/cert/cert_status_flags_list.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M net/cert/cert_verify_proc.h View 1 2 3 2 chunks +13 lines, -0 lines 0 comments Download
M net/cert/cert_verify_proc.cc View 1 2 3 4 5 6 4 chunks +47 lines, -1 line 1 comment Download
M net/cert/cert_verify_proc_unittest.cc View 1 2 3 4 5 6 7 8 7 chunks +37 lines, -20 lines 0 comments Download
A net/data/ssl/certificates/11_year_validity.pem View 1 2 3 4 5 6 7 1 chunk +81 lines, -0 lines 0 comments Download
A net/data/ssl/certificates/40_months_after_2015_04.pem View 1 2 3 4 5 6 7 1 chunk +81 lines, -0 lines 0 comments Download
A net/data/ssl/certificates/61_months_after_2012_07.pem View 1 2 3 4 5 6 7 1 chunk +81 lines, -0 lines 0 comments Download
M net/data/ssl/certificates/README View 1 2 3 4 5 6 7 8 9 3 chunks +8 lines, -4 lines 0 comments Download
A net/data/ssl/certificates/reject_intranet_hosts.pem View 1 2 3 4 5 6 7 8 1 chunk +69 lines, -0 lines 0 comments Download
D net/data/ssl/certificates/satveda.pem View 1 2 3 4 5 6 7 1 chunk +0 lines, -207 lines 0 comments Download
A net/data/ssl/certificates/twitter-chain.pem View 1 2 3 4 5 6 1 chunk +302 lines, -0 lines 0 comments Download
M net/data/ssl/scripts/generate-test-certs.sh View 1 2 3 4 5 6 7 8 9 1 chunk +45 lines, -1 line 0 comments Download
M net/test/test_certificate_data.h View 1 2 3 4 5 6 7 1 chunk +17 lines, -13 lines 0 comments Download

Messages

Total messages: 34 (4 generated)
palmer
Trying again. abarth for OWNERS in chrome/browser/ssl. We'll also need a cert_verify_proc_unittest; coming up.
7 years, 4 months ago (2013-08-01 19:03:53 UTC) #1
Ryan Sleevi
Comments from the bug pasted here: I think we'll want to be careful about this, ...
7 years, 4 months ago (2013-08-01 19:05:01 UTC) #2
abarth-chromium
Someone else should review this change. I'd welcome a CL to remove me from browser/ssl/OWNERS.
7 years, 4 months ago (2013-08-01 20:57:26 UTC) #3
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, 4 months ago (2013-08-03 17:52:54 UTC) #4
Ryan Sleevi
Just to circle back on this - we haven't abandoned it, sigbjorn. We're in the ...
7 years, 4 months ago (2013-08-16 02:39:47 UTC) #5
palmer
As Ryan says, https://cabforum.org/pipermail/public/2013-August/002135.html we'll land this in early 2014.
7 years, 4 months ago (2013-08-19 17:46:39 UTC) #6
Ryan Sleevi
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, 4 months ago (2013-08-19 17:57:50 UTC) #7
wtc
On 2013/08/03 17:52:54, sigbjorn wrote: > > Suggestion: > Internet standards bodies have found that ...
7 years, 4 months ago (2013-08-21 00:48:04 UTC) #8
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, 4 months ago (2013-08-21 01:26:24 UTC) #9
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, 4 months ago (2013-08-21 20:07:41 UTC) #10
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, 4 months ago (2013-08-21 22:24:15 UTC) #11
palmer
Is it time to land this? Assuming the bots are green.
6 years, 10 months ago (2014-01-30 20:04:51 UTC) #12
palmer
Soo... this CL is long in the tooth... rsleevi, should I resuscitate it, or is ...
6 years, 2 months ago (2014-10-17 01:16:43 UTC) #13
Ryan Sleevi
Sorry about the 'slight' delays on this x_x You're gonna blow up all the tests ...
6 years, 1 month ago (2014-10-27 22:07:39 UTC) #14
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, 1 month ago (2014-10-28 00:05:53 UTC) #15
Ryan Sleevi
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) CERT_VALIDITY_TOO_LONG ? https://codereview.chromium.org/20628006/diff/229001/net/cert/cert_verify_proc.cc File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/20628006/diff/229001/net/cert/cert_verify_proc.cc#newcode281 ...
6 years, 1 month ago (2014-10-29 22:22:07 UTC) #16
palmer
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, 1 month ago (2014-10-30 01:23:16 UTC) #17
palmer
OK, here is some more testing. One problem: CertVerifyProcTest.IntranetHostsRejected fails because ok_cert.pem is valid for ...
6 years, 1 month ago (2014-10-30 20:55:51 UTC) #18
Ryan Sleevi
On 2014/10/30 20:55:51, Chromium Palmer wrote: > OK, here is some more testing. > > ...
6 years, 1 month ago (2014-10-30 22:29:33 UTC) #19
palmer
> Don't replace ok_cert.pem. Just change IntranetHostsRejected to a new test cert > (and slap ...
6 years, 1 month ago (2014-10-31 20:05:23 UTC) #20
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, 1 month ago (2014-11-04 21:48:27 UTC) #21
palmer
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: CN=xn--wgv71a119e.com > Punycode CNs may be overkill. It ...
6 years, 1 month ago (2014-11-07 19:16:14 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/20628006/309001
6 years, 1 month ago (2014-11-07 19:18:29 UTC) #24
commit-bot: I haz the power
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, 1 month ago (2014-11-07 19:25:28 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/20628006/309001
6 years, 1 month ago (2014-11-07 19:38:28 UTC) #28
commit-bot: I haz the power
Committed patchset #10 (id:309001)
6 years, 1 month ago (2014-11-07 20:32:24 UTC) #29
commit-bot: I haz the power
Patchset 10 (id:??) landed as https://crrev.com/15faa5904d553ebff1d72969ab1b7631d7d48d78 Cr-Commit-Position: refs/heads/master@{#303286}
6 years, 1 month ago (2014-11-07 20:32:57 UTC) #30
twocsies
Looks like this change caused some errors. Until April 2015 there can be 60 month ...
6 years, 1 month ago (2014-11-09 12:12:58 UTC) #31
levente.tamas
Why doesn't it change this in the FUTURE so that issuers have time to react ...
6 years, 1 month ago (2014-11-10 12:35:31 UTC) #32
haavardm
6 years, 1 month ago (2014-11-10 16:20:01 UTC) #34
Message was sent while issue was closed.
https://codereview.chromium.org/20628006/diff/309001/net/cert/cert_verify_pro...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/20628006/diff/309001/net/cert/cert_verify_pro...
net/cert/cert_verify_proc.cc:656: if (start >= time_2015_04_01)
The 39 month check kicks in since the hardcoded dates are set in seconds, while
start is in microseconds since the epoch. Adding 6 zeros to the hardcoded dates 
should fix the issue.

Powered by Google App Engine
This is Rietveld 408576698