Chromium Code Reviews
Help | Chromium Project | Sign in
(2)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 4 weeks ago by eroman
Modified:
1 month, 3 weeks 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 57 (39 generated)
eroman
1 month, 4 weeks 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 ...
1 month, 3 weeks 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 ...
1 month, 3 weeks 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 ...
1 month, 3 weeks 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: > ...
1 month, 3 weeks 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: > ...
1 month, 3 weeks 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 > ...
1 month, 3 weeks 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: > ...
1 month, 3 weeks ago (2017-03-08 17:09:24 UTC) #25
mattm
lgtm
1 month, 3 weeks ago (2017-03-08 22:59:58 UTC) #32
mattm
On 2017/03/08 22:59:58, mattm wrote: > lgtm patchset 8 still lgtm
1 month, 3 weeks 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 ...
1 month, 3 weeks 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: ...
1 month, 3 weeks 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 ...
1 month, 3 weeks 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 ...
1 month, 3 weeks 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
1 month, 3 weeks 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)
1 month, 3 weeks 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
1 month, 3 weeks ago (2017-03-09 05:50:24 UTC) #54
commit-bot: I haz the power
1 month, 3 weeks 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46