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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks ago by eroman
Modified:
2 weeks, 1 day ago
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
3 weeks ago (2017-03-03 00:48:07 UTC) #7
Ryan Sleevi (conf till 3-24)
I'm going to punt to Matt because I know I won't have a chance to ...
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 ...
2 weeks, 6 days 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 ...
2 weeks, 2 days 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: > ...
2 weeks, 2 days 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: > ...
2 weeks, 2 days 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 > ...
2 weeks, 2 days 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: > ...
2 weeks, 2 days ago (2017-03-08 17:09:24 UTC) #25
mattm
lgtm
2 weeks, 1 day ago (2017-03-08 22:59:58 UTC) #32
mattm
On 2017/03/08 22:59:58, mattm wrote: > lgtm patchset 8 still lgtm
2 weeks, 1 day ago (2017-03-09 00:18:27 UTC) #35
Ryan Sleevi (conf till 3-24)
LGTM with a few minor comments and requests for changes (sorry to be auto-hating on ...
2 weeks, 1 day 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: ...
2 weeks, 1 day ago (2017-03-09 01:09:44 UTC) #41
Ryan Sleevi (conf till 3-24)
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 ...
2 weeks, 1 day 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 ...
2 weeks, 1 day 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
2 weeks, 1 day 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)
2 weeks, 1 day 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
2 weeks, 1 day ago (2017-03-09 05:50:24 UTC) #54
commit-bot: I haz the power
2 weeks, 1 day 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 d1a128a62