Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(21)

Issue 8374020: Make it a fatal SSL error when encountering certs signed with md[2,4], and interstitial md5 (Closed)

Created:
7 years, 3 months ago by Ryan Sleevi
Modified:
7 years, 2 months ago
Reviewers:
palmer, joth, wtc, Chris Palmer
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cbentzel+watch_chromium.org, jam, dpranke-watch+content_chromium.org, agl, ian fette, Chris Evans
Visibility:
Public.

Description

When encountering certificates signed with md2/md4, make it a fatal error. When encountering certificates signed with md5, interstitial the page with an error about md5 being a weak signing algorithm. This excludes checking the signatures of root certificates (trust anchors), as their self-signed signatures are not relevant to the security of the chain. R=wtc@chromium.org BUG=101123 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114432

Patch Set 1 #

Total comments: 6

Patch Set 2 : Rebased #

Patch Set 3 : MD2 & MD4: INVALID, MD5: WEAK #

Total comments: 4

Patch Set 4 : joth feedback #

Total comments: 3

Patch Set 5 : Rebase #

Patch Set 6 : Feedback and address the major error masking #

Patch Set 7 : Fix extra newline #

Patch Set 8 : Add extra check #

Total comments: 8

Patch Set 9 : Rebase before commit #

Patch Set 10 : wtc feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+48 lines, -9 lines) Patch
M net/base/x509_certificate.cc View 1 2 3 4 5 6 7 8 9 1 chunk +16 lines, -0 lines 0 comments Download
M net/base/x509_certificate_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +30 lines, -1 line 0 comments Download
M net/base/x509_certificate_win.cc View 1 2 3 4 5 6 7 8 9 3 chunks +2 lines, -8 lines 0 comments Download

Messages

Total messages: 33 (0 generated)
Ryan Sleevi
wtc: For your review joth: The one _openssl change to make it treat an invalid ...
7 years, 3 months ago (2011-10-22 03:39:22 UTC) #1
palmer
http://codereview.chromium.org/8374020/diff/1/net/base/x509_certificate.cc File net/base/x509_certificate.cc (right): http://codereview.chromium.org/8374020/diff/1/net/base/x509_certificate.cc#newcode607 net/base/x509_certificate.cc:607: verify_result->has_md5) { wtc does not want to ban MD5 ...
7 years, 3 months ago (2011-10-24 18:22:31 UTC) #2
wtc
Review comments on Patch Set 1: rsleevi: thanks for writing the patch quickly! I have ...
7 years, 3 months ago (2011-10-24 22:36:42 UTC) #3
wtc
http://codereview.chromium.org/8374020/diff/1/net/base/x509_certificate.cc File net/base/x509_certificate.cc (right): http://codereview.chromium.org/8374020/diff/1/net/base/x509_certificate.cc#newcode607 net/base/x509_certificate.cc:607: verify_result->has_md5) { On 2011/10/24 18:22:31, Chris P. wrote: > ...
7 years, 3 months ago (2011-10-24 22:41:14 UTC) #4
Ryan Sleevi
wtc: In discussing with Chris, I want to make sure we're on the same page: ...
7 years, 3 months ago (2011-10-24 23:16:32 UTC) #5
agl
On Mon, Oct 24, 2011 at 7:16 PM, <rsleevi@chromium.org> wrote: > wtc: In discussing with ...
7 years, 3 months ago (2011-10-24 23:19:00 UTC) #6
Chris Palmer
> I don't believe that the algorithm of the self-signature on a root > matters ...
7 years, 3 months ago (2011-10-24 23:22:56 UTC) #7
wtc
On 2011/10/24 23:22:56, Chris Palmer wrote: > > For locally-installed roots, it seems better safe ...
7 years, 3 months ago (2011-10-24 23:42:51 UTC) #8
Ryan Sleevi
On 2011/10/24 23:22:56, Chris Palmer wrote: > > I don't believe that the algorithm of ...
7 years, 3 months ago (2011-10-24 23:46:21 UTC) #9
wtc
On 2011/10/24 23:16:32, Ryan Sleevi wrote: > wtc: In discussing with Chris, I want to ...
7 years, 3 months ago (2011-10-24 23:47:54 UTC) #10
Ryan Sleevi
Thanks for the feedback all. Now that all the platform-specific changes have landed, as well ...
7 years, 3 months ago (2011-11-03 05:51:31 UTC) #11
joth
just one question from me.. http://codereview.chromium.org/8374020/diff/12003/net/base/x509_certificate.cc File net/base/x509_certificate.cc (right): http://codereview.chromium.org/8374020/diff/12003/net/base/x509_certificate.cc#newcode602 net/base/x509_certificate.cc:602: rv = MapCertStatusToNetError(verify_result->cert_status); I ...
7 years, 3 months ago (2011-11-03 09:09:38 UTC) #12
palmer
http://codereview.chromium.org/8374020/diff/12003/net/base/x509_certificate.cc File net/base/x509_certificate.cc (right): http://codereview.chromium.org/8374020/diff/12003/net/base/x509_certificate.cc#newcode602 net/base/x509_certificate.cc:602: rv = MapCertStatusToNetError(verify_result->cert_status); As I understand it, only CERT_STATUS_WEAK_SIGNATURE_ALGORITHM ...
7 years, 3 months ago (2011-11-03 17:47:04 UTC) #13
joth
http://codereview.chromium.org/8374020/diff/12003/net/base/x509_certificate.cc File net/base/x509_certificate.cc (right): http://codereview.chromium.org/8374020/diff/12003/net/base/x509_certificate.cc#newcode602 net/base/x509_certificate.cc:602: rv = MapCertStatusToNetError(verify_result->cert_status); On 2011/11/03 17:47:04, Chris P. wrote: ...
7 years, 3 months ago (2011-11-03 20:01:27 UTC) #14
Ryan Sleevi
http://codereview.chromium.org/8374020/diff/12003/net/base/x509_certificate.cc File net/base/x509_certificate.cc (right): http://codereview.chromium.org/8374020/diff/12003/net/base/x509_certificate.cc#newcode602 net/base/x509_certificate.cc:602: rv = MapCertStatusToNetError(verify_result->cert_status); On 2011/11/03 20:01:27, joth wrote: > ...
7 years, 3 months ago (2011-11-03 23:22:45 UTC) #15
joth
LGTM (but IANAO)
7 years, 3 months ago (2011-11-04 08:43:09 UTC) #16
wtc
Patch Set 4 LGTM. http://codereview.chromium.org/8374020/diff/14002/net/base/x509_certificate.cc File net/base/x509_certificate.cc (right): http://codereview.chromium.org/8374020/diff/14002/net/base/x509_certificate.cc#newcode604 net/base/x509_certificate.cc:604: // with a benign, user-overridable ...
7 years, 3 months ago (2011-11-04 22:57:41 UTC) #17
Ryan Sleevi
In light of Palmer's < 1023 check, I'm wondering whether or not incomplete chains should ...
7 years, 3 months ago (2011-11-20 00:17:00 UTC) #18
Ryan Sleevi
wtc: Would you mind double checking the explanation of Patchset 8 and making sure there ...
7 years, 2 months ago (2011-11-30 03:27:30 UTC) #19
agl
On Tue, Nov 29, 2011 at 10:27 PM, <rsleevi@chromium.org> wrote: > wtc: Would you mind ...
7 years, 2 months ago (2011-11-30 15:16:58 UTC) #20
Chris Palmer
On Wed, Nov 30, 2011 at 7:16 AM, Adam Langley <agl@chromium.org> wrote: > My (weak) ...
7 years, 2 months ago (2011-11-30 19:41:29 UTC) #21
wtc
On 2011/11/30 03:27:30, Ryan Sleevi wrote: > wtc: Would you mind double checking the explanation ...
7 years, 2 months ago (2011-12-02 01:29:43 UTC) #22
wtc
Patch Set 8 LGTM. http://codereview.chromium.org/8374020/diff/24005/net/base/x509_certificate.cc File net/base/x509_certificate.cc (right): http://codereview.chromium.org/8374020/diff/24005/net/base/x509_certificate.cc#newcode608 net/base/x509_certificate.cc:608: bool has_cert_status_error = Nit: has_cert_status_error ...
7 years, 2 months ago (2011-12-02 23:04:59 UTC) #23
Ryan Sleevi
http://codereview.chromium.org/8374020/diff/24005/net/base/x509_certificate.cc File net/base/x509_certificate.cc (right): http://codereview.chromium.org/8374020/diff/24005/net/base/x509_certificate.cc#newcode615 net/base/x509_certificate.cc:615: if (rv == OK || (IsCertificateError(rv) && has_cert_status_error)) On ...
7 years, 2 months ago (2011-12-02 23:54:27 UTC) #24
wtc
http://codereview.chromium.org/8374020/diff/24005/net/base/x509_certificate.cc File net/base/x509_certificate.cc (right): http://codereview.chromium.org/8374020/diff/24005/net/base/x509_certificate.cc#newcode615 net/base/x509_certificate.cc:615: if (rv == OK || (IsCertificateError(rv) && has_cert_status_error)) On ...
7 years, 2 months ago (2011-12-06 00:56:17 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsleevi@chromium.org/8374020/40001
7 years, 2 months ago (2011-12-14 02:38:12 UTC) #26
commit-bot: I haz the power
Try job failure for 8374020-40001 (retry) on linux_rel for step "ui_tests". It's a second try, ...
7 years, 2 months ago (2011-12-14 04:01:25 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsleevi@chromium.org/8374020/40001
7 years, 2 months ago (2011-12-14 04:03:31 UTC) #28
commit-bot: I haz the power
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is ...
7 years, 2 months ago (2011-12-14 06:06:09 UTC) #29
commit-bot: I haz the power
No LGTM from valid reviewers yet.
7 years, 2 months ago (2011-12-14 06:47:28 UTC) #30
commit-bot: I haz the power
No LGTM from valid reviewers yet.
7 years, 2 months ago (2011-12-14 08:05:26 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsleevi@chromium.org/8374020/40001
7 years, 2 months ago (2011-12-14 14:41:20 UTC) #32
commit-bot: I haz the power
7 years, 2 months ago (2011-12-14 16:08:21 UTC) #33
Change committed as 114432

Powered by Google App Engine
This is Rietveld 408576698