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

Issue 2731603002: Check TBSCertificate.algorithm and Certificate.signatureAlgorithm for (Closed)

Created:
9 months, 1 week ago by eroman
Modified:
9 months, 1 week ago
Reviewers:
Ryan Sleevi, mattm
CC:
chromium-reviews, cbentzel+watch_chromium.org, mac-reviews_chromium.org, net-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Check TBSCertificate.algorithm and Certificate.signatureAlgorithm for consistency when verifying certificates. The underlying platform verifiers don't do this, which can lead to confusion when trying to enforce policy for SHA1 on the verified chain. * If the two signature algorithms don't match will fail with ERR_INVALID_CERT. * If the chain contains a signature algorithm that we don't know how to parse, will also fail with ERR_INVALID_CERT BUG=690821 Review-Url: https://codereview.chromium.org/2731603002 Cr-Commit-Position: refs/heads/master@{#455682} Committed: https://chromium.googlesource.com/chromium/src/+/a77953fe670968fe6728796b77cedf48f0954d78

Patch Set 1 #

Patch Set 2 : fix #

Total comments: 27

Patch Set 3 : address mattm's comments #

Patch Set 4 : moar #

Total comments: 4

Patch Set 5 : Address mattm's comments (use fixed serial numbers) #

Patch Set 6 : rebase #

Patch Set 7 : add a special case for iOS #

Patch Set 8 : add tests for root #

Total comments: 12

Patch Set 9 : address rsleevi's comments #

Patch Set 10 : Use rsleevi's background comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+614 lines, -205 lines) Patch
M net/cert/asn1_util.h View 1 2 3 4 5 6 7 8 1 chunk +17 lines, -0 lines 0 comments Download
M net/cert/asn1_util.cc View 1 chunk +52 lines, -0 lines 0 comments Download
M net/cert/cert_verify_proc.cc View 1 2 3 4 5 6 7 8 9 3 chunks +100 lines, -28 lines 0 comments Download
M net/cert/cert_verify_proc_mac.cc View 1 2 3 4 5 2 chunks +39 lines, -16 lines 0 comments Download
M net/cert/cert_verify_proc_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +360 lines, -0 lines 0 comments Download
M net/cert/internal/signature_algorithm.h View 1 2 3 2 chunks +8 lines, -1 line 0 comments Download
M net/cert/internal/signature_algorithm.cc View 1 2 3 4 5 6 7 8 2 chunks +34 lines, -1 line 0 comments Download
M net/cert/internal/verify_certificate_chain.cc View 1 2 2 chunks +4 lines, -15 lines 0 comments Download
M net/cert/x509_certificate.h View 1 2 3 4 5 2 chunks +0 lines, -17 lines 0 comments Download
M net/cert/x509_certificate_ios.cc View 1 chunk +0 lines, -23 lines 0 comments Download
M net/cert/x509_certificate_mac.cc View 1 chunk +0 lines, -37 lines 0 comments Download
M net/cert/x509_certificate_nss.cc View 1 chunk +0 lines, -22 lines 0 comments Download
M net/cert/x509_certificate_openssl.cc View 1 chunk +0 lines, -18 lines 0 comments Download
M net/cert/x509_certificate_win.cc View 1 chunk +0 lines, -27 lines 0 comments Download

Messages

Total messages: 57 (39 generated)
eroman
9 months, 1 week ago (2017-03-03 00:48:07 UTC) #7
Ryan Sleevi
I'm going to punt to Matt because I know I won't have a chance to ...
9 months, 1 week ago (2017-03-03 01:46:36 UTC) #11
mattm
https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_proc.cc File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_proc.cc#newcode404 net/cert/cert_verify_proc.cc:404: // * Sets |*has_mismatched_signature_algorithms| to true if note that ...
9 months, 1 week ago (2017-03-04 02:34:27 UTC) #12
eroman
https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_proc.cc File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_proc.cc#newcode404 net/cert/cert_verify_proc.cc:404: // * Sets |*has_mismatched_signature_algorithms| to true if On 2017/03/04 ...
9 months, 1 week ago (2017-03-07 23:43:00 UTC) #17
mattm
https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_proc_unittest.cc File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_proc_unittest.cc#newcode797 net/cert/cert_verify_proc_unittest.cc:797: *const_cast<char*>(serial_value.data()) = next_serial_number_++; On 2017/03/07 23:43:00, eroman wrote: > ...
9 months, 1 week ago (2017-03-08 01:41:35 UTC) #20
eroman
https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_proc_unittest.cc File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_proc_unittest.cc#newcode797 net/cert/cert_verify_proc_unittest.cc:797: *const_cast<char*>(serial_value.data()) = next_serial_number_++; On 2017/03/08 01:41:34, mattm wrote: > ...
9 months, 1 week ago (2017-03-08 01:59:29 UTC) #21
mattm
On 2017/03/08 01:59:29, eroman wrote: > https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_proc_unittest.cc > File net/cert/cert_verify_proc_unittest.cc (right): > > https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_proc_unittest.cc#newcode797 > ...
9 months, 1 week ago (2017-03-08 02:02:05 UTC) #22
eroman
https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_proc_unittest.cc File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2731603002/diff/20001/net/cert/cert_verify_proc_unittest.cc#newcode797 net/cert/cert_verify_proc_unittest.cc:797: *const_cast<char*>(serial_value.data()) = next_serial_number_++; On 2017/03/08 01:59:28, eroman wrote: > ...
9 months, 1 week ago (2017-03-08 17:09:24 UTC) #25
mattm
lgtm
9 months, 1 week ago (2017-03-08 22:59:58 UTC) #32
mattm
On 2017/03/08 22:59:58, mattm wrote: > lgtm patchset 8 still lgtm
9 months, 1 week ago (2017-03-09 00:18:27 UTC) #35
Ryan Sleevi
LGTM with a few minor comments and requests for changes (sorry to be auto-hating on ...
9 months, 1 week ago (2017-03-09 00:45:26 UTC) #38
eroman
https://codereview.chromium.org/2731603002/diff/140001/net/cert/asn1_util.h File net/cert/asn1_util.h (right): https://codereview.chromium.org/2731603002/diff/140001/net/cert/asn1_util.h#newcode59 net/cert/asn1_util.h:59: // with TBSCertificate.algorithm. On 2017/03/09 00:45:26, Ryan Sleevi wrote: ...
9 months, 1 week ago (2017-03-09 01:09:44 UTC) #41
Ryan Sleevi
It wasn't meant to be a comment-nit, but since you mentioned it... :D https://codereview.chromium.org/2731603002/diff/140001/net/cert/cert_verify_proc_unittest.cc File ...
9 months, 1 week ago (2017-03-09 01:19:46 UTC) #42
eroman
https://codereview.chromium.org/2731603002/diff/140001/net/cert/cert_verify_proc_unittest.cc File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2731603002/diff/140001/net/cert/cert_verify_proc_unittest.cc#newcode633 net/cert/cert_verify_proc_unittest.cc:633: // system verifiers don't check this. This can lead ...
9 months, 1 week ago (2017-03-09 01:56:36 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2731603002/180001
9 months, 1 week ago (2017-03-09 03:05:17 UTC) #50
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/336900)
9 months, 1 week ago (2017-03-09 04:02:11 UTC) #52
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2731603002/180001
9 months, 1 week ago (2017-03-09 05:50:24 UTC) #54
commit-bot: I haz the power
9 months, 1 week ago (2017-03-09 06:45:59 UTC) #57
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/a77953fe670968fe6728796b77ce...

Powered by Google App Engine
This is Rietveld 0eb02b776