Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2483783003:
Distrust publicly trusted SHA-1 certs (Closed)
|
Created:
4 years, 1 month ago by Ryan Sleevi Modified:
4 years, 1 month ago Reviewers:
davidben CC:
chromium-reviews, cbentzel+watch_chromium.org, awhalley Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDistrust publicly trusted SHA-1 certs
Reject all publicly trusted SHA-1 certificates, as announced
September 2014 at
https://security.googleblog.com/2014/09/gradually-sunsetting-sha-1.html
and
https://blog.chromium.org/2014/09/gradually-sunsetting-sha-1.html
To avoid too much disruption, enterprise SHA-1
is still allowed for M56; in M57, it will be
disabled unless the EnableSha1ForLocalAnchors policy is
set, as described at
https://www.chromium.org/Home/chromium-security/education/tls/sha-1
As with other TLS deprecations, an emergency 'undeprecate'
switch is kept around in the event of unexpected breakage,
to allow rapid reverting to the previous behaviour.
BUG=653691
Committed: https://crrev.com/63efb444af72ffca8e96a9df770c05dc97f241bb
Cr-Commit-Position: refs/heads/master@{#430866}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Rebased & Fixed #
Total comments: 3
Messages
Total messages: 36 (21 generated)
rsleevi@chromium.org changed reviewers: + davidben@chromium.org
Davidben for review, cc'ing awhalley for awareness It's finally happening! (Except for Win7 intermediates, as captured in the tests and description) The change is a bit larger than expected in the tests, but I think it's cleaner this way; the WeakCert tests are moved to 'detect only' tests, to make sure they flag, and then the rest of the policy is left as specific tests to make sure that all of CertVerifyProc::Verify's policy controls work, independent of the library-specific CVP::VerifyInternal I avoided simplifying the conditional intentionally; I think it was easier to read by putting all the conditions out explicitly, rather than trying to have to mentally track their state.
Description was changed from ========== Ditrust publicly trusted SHA-1 certs Reject all publicly trusted SHA-1 certificates, as announced September 2014 at https://security.googleblog.com/2014/09/gradually-sunsetting-sha-1.html and https://blog.chromium.org/2014/09/gradually-sunsetting-sha-1.html To avoid too much disruption, enterprise SHA-1 is still allowed for M56; in M57, it will be disabled unless the EnableSha1ForLocalAnchors policy is set, as described at https://www.chromium.org/Home/chromium-security/education/tls/sha-1 As with other TLS deprecations, an emergency 'undeprecate' switch is kept around in the event of unexpected breakage, to allow rapid reverting to the previous behaviour. BUG=653691 ========== to ========== Distrust publicly trusted SHA-1 certs Reject all publicly trusted SHA-1 certificates, as announced September 2014 at https://security.googleblog.com/2014/09/gradually-sunsetting-sha-1.html and https://blog.chromium.org/2014/09/gradually-sunsetting-sha-1.html To avoid too much disruption, enterprise SHA-1 is still allowed for M56; in M57, it will be disabled unless the EnableSha1ForLocalAnchors policy is set, as described at https://www.chromium.org/Home/chromium-security/education/tls/sha-1 As with other TLS deprecations, an emergency 'undeprecate' switch is kept around in the event of unexpected breakage, to allow rapid reverting to the previous behaviour. BUG=653691 ==========
The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good. Just minor questions about the tests. https://codereview.chromium.org/2483783003/diff/1/net/cert/cert_verify_proc_u... File net/cert/cert_verify_proc_unittest.cc (left): https://codereview.chromium.org/2483783003/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:1620: } Why were these removed? https://codereview.chromium.org/2483783003/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:1873: result.verified_cert = cert; Why this change (and below)? Looks like this is meant as a filled in CertVerifyResult to pass into MockCertVerifyProc.
https://codereview.chromium.org/2483783003/diff/1/net/cert/cert_verify_proc_u... File net/cert/cert_verify_proc_unittest.cc (left): https://codereview.chromium.org/2483783003/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:1620: } On 2016/11/08 16:40:33, davidben wrote: > Why were these removed? I thought I addressed this in https://codereview.chromium.org/2483783003/#msg2 :) the WeakCert tests are moved to 'detect only' tests, to make sure they flag, and then the rest of the policy is left as specific tests to make sure that all of CertVerifyProc::Verify's policy controls work, independent of the library-specific CVP::VerifyInternal In particular, detection is consistent for all certs in CVP::VerifyInternal - that is, we need to detect MD2 / SHA1 consistently across all CVP::VI. However, policy - whether SHA-1 is rejected or not - is dependent upon publicly trusted cert or not. So it made sense to separate out the CVP::VI testing (which this is), from the CVP::Verify testing (which the new tests, through the use of the MockCertVerifyProc, can do - by simulating public or private, similar to the existing SHA-1 tests) https://codereview.chromium.org/2483783003/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:1873: result.verified_cert = cert; On 2016/11/08 16:40:33, davidben wrote: > Why this change (and below)? Looks like this is meant as a filled in > CertVerifyResult to pass into MockCertVerifyProc. Because MockCertVerifyProc's contract is that it already fills in verified_cert, because all CVPs have to do that :) So it was two fold: 1) Existing bit was unnecessary 2) ASSERT was needed to make sure the cert parsed versus the test crashing. I could land these as separate, but they were so minor it seemed... too tempting.
The CQ bit was checked by rsleevi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm https://codereview.chromium.org/2483783003/diff/1/net/cert/cert_verify_proc_u... File net/cert/cert_verify_proc_unittest.cc (left): https://codereview.chromium.org/2483783003/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:1620: } On 2016/11/08 18:23:41, Ryan Sleevi wrote: > On 2016/11/08 16:40:33, davidben wrote: > > Why were these removed? > > I thought I addressed this in https://codereview.chromium.org/2483783003/#msg2 > :) > > the WeakCert tests are moved to 'detect only' tests, to make sure they > flag, and then the rest of the policy is left as specific tests to make sure > that all of CertVerifyProc::Verify's policy controls work, independent of the > library-specific CVP::VerifyInternal > > > In particular, detection is consistent for all certs in CVP::VerifyInternal - > that is, we need to detect MD2 / SHA1 consistently across all CVP::VI. However, > policy - whether SHA-1 is rejected or not - is dependent upon publicly trusted > cert or not. So it made sense to separate out the CVP::VI testing (which this > is), from the CVP::Verify testing (which the new tests, through the use of the > MockCertVerifyProc, can do - by simulating public or private, similar to the > existing SHA-1 tests) I'm sorry, I can't read! :-) Makes sense.
The CQ bit was checked by rsleevi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Distrust publicly trusted SHA-1 certs Reject all publicly trusted SHA-1 certificates, as announced September 2014 at https://security.googleblog.com/2014/09/gradually-sunsetting-sha-1.html and https://blog.chromium.org/2014/09/gradually-sunsetting-sha-1.html To avoid too much disruption, enterprise SHA-1 is still allowed for M56; in M57, it will be disabled unless the EnableSha1ForLocalAnchors policy is set, as described at https://www.chromium.org/Home/chromium-security/education/tls/sha-1 As with other TLS deprecations, an emergency 'undeprecate' switch is kept around in the event of unexpected breakage, to allow rapid reverting to the previous behaviour. BUG=653691 ========== to ========== Distrust publicly trusted SHA-1 certs Reject all publicly trusted SHA-1 certificates, as announced September 2014 at https://security.googleblog.com/2014/09/gradually-sunsetting-sha-1.html and https://blog.chromium.org/2014/09/gradually-sunsetting-sha-1.html To avoid too much disruption, enterprise SHA-1 is still allowed for M56; in M57, it will be disabled unless the EnableSha1ForLocalAnchors policy is set, as described at https://www.chromium.org/Home/chromium-security/education/tls/sha-1 As with other TLS deprecations, an emergency 'undeprecate' switch is kept around in the event of unexpected breakage, to allow rapid reverting to the previous behaviour. BUG=653691 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Distrust publicly trusted SHA-1 certs Reject all publicly trusted SHA-1 certificates, as announced September 2014 at https://security.googleblog.com/2014/09/gradually-sunsetting-sha-1.html and https://blog.chromium.org/2014/09/gradually-sunsetting-sha-1.html To avoid too much disruption, enterprise SHA-1 is still allowed for M56; in M57, it will be disabled unless the EnableSha1ForLocalAnchors policy is set, as described at https://www.chromium.org/Home/chromium-security/education/tls/sha-1 As with other TLS deprecations, an emergency 'undeprecate' switch is kept around in the event of unexpected breakage, to allow rapid reverting to the previous behaviour. BUG=653691 ========== to ========== Distrust publicly trusted SHA-1 certs Reject all publicly trusted SHA-1 certificates, as announced September 2014 at https://security.googleblog.com/2014/09/gradually-sunsetting-sha-1.html and https://blog.chromium.org/2014/09/gradually-sunsetting-sha-1.html To avoid too much disruption, enterprise SHA-1 is still allowed for M56; in M57, it will be disabled unless the EnableSha1ForLocalAnchors policy is set, as described at https://www.chromium.org/Home/chromium-security/education/tls/sha-1 As with other TLS deprecations, an emergency 'undeprecate' switch is kept around in the event of unexpected breakage, to allow rapid reverting to the previous behaviour. BUG=653691 Committed: https://crrev.com/a6bdfc7c128e0e51b3717c52c113d8dcff30bcb9 Cr-Commit-Position: refs/heads/master@{#430674} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/a6bdfc7c128e0e51b3717c52c113d8dcff30bcb9 Cr-Commit-Position: refs/heads/master@{#430674}
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2487063003/ by scheib@chromium.org. The reason for reverting is: CertVerifyProcTest.RejectsPublicSHA1IntermediatesUnlessAllowed failing in net_unittests on Windows-10 Findit helped narrow: https://findit-for-me.appspot.com/waterfall/build-failure?url=https://build.c... Reliable failure: https://chromium-swarm.appspot.com/user/task/325ecf51e2458510 """ [ RUN ] CertVerifyProcTest.RejectsPublicSHA1IntermediatesUnlessAllowed c:\b\c\b\win\src\net\cert\cert_verify_proc_unittest.cc(1625): error: Value of: error Expected: net::OK Actual: -213, net::ERR_CERT_VALIDITY_TOO_LONG [ FAILED ] CertVerifyProcTest.RejectsPublicSHA1IntermediatesUnlessAllowed (5 ms) """.
Message was sent while issue was closed.
Description was changed from ========== Distrust publicly trusted SHA-1 certs Reject all publicly trusted SHA-1 certificates, as announced September 2014 at https://security.googleblog.com/2014/09/gradually-sunsetting-sha-1.html and https://blog.chromium.org/2014/09/gradually-sunsetting-sha-1.html To avoid too much disruption, enterprise SHA-1 is still allowed for M56; in M57, it will be disabled unless the EnableSha1ForLocalAnchors policy is set, as described at https://www.chromium.org/Home/chromium-security/education/tls/sha-1 As with other TLS deprecations, an emergency 'undeprecate' switch is kept around in the event of unexpected breakage, to allow rapid reverting to the previous behaviour. BUG=653691 Committed: https://crrev.com/a6bdfc7c128e0e51b3717c52c113d8dcff30bcb9 Cr-Commit-Position: refs/heads/master@{#430674} ========== to ========== Distrust publicly trusted SHA-1 certs Reject all publicly trusted SHA-1 certificates, as announced September 2014 at https://security.googleblog.com/2014/09/gradually-sunsetting-sha-1.html and https://blog.chromium.org/2014/09/gradually-sunsetting-sha-1.html To avoid too much disruption, enterprise SHA-1 is still allowed for M56; in M57, it will be disabled unless the EnableSha1ForLocalAnchors policy is set, as described at https://www.chromium.org/Home/chromium-security/education/tls/sha-1 As with other TLS deprecations, an emergency 'undeprecate' switch is kept around in the event of unexpected breakage, to allow rapid reverting to the previous behaviour. BUG=653691 ==========
David: Can you re-review just the diffs between patches 1 & 2 https://codereview.chromium.org/2483783003/diff/20001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2483783003/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:1533: return base::win::GetVersion() < base::win::VERSION_WIN8; This is why it made it past bots https://codereview.chromium.org/2483783003/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:1611: GetTestCertsDirectory(), "39_months_after_2015_04.pem")); I only changed this test, because the hope is for this test to be removed. It does mean that the other tests, w/r/t using ok_cert.pem and setting is_issued_by_known_root, are assuming that algorithm policies trump date policies - but that's a reasonable expectation.
The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2483783003/diff/20001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2483783003/diff/20001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:1533: return base::win::GetVersion() < base::win::VERSION_WIN8; On 2016/11/09 01:02:14, Ryan Sleevi wrote: > This is why it made it past bots Oops. I should have been paying more attention. :-(
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rsleevi@chromium.org
The CQ bit was unchecked by rsleevi@chromium.org
The CQ bit was checked by rsleevi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Distrust publicly trusted SHA-1 certs Reject all publicly trusted SHA-1 certificates, as announced September 2014 at https://security.googleblog.com/2014/09/gradually-sunsetting-sha-1.html and https://blog.chromium.org/2014/09/gradually-sunsetting-sha-1.html To avoid too much disruption, enterprise SHA-1 is still allowed for M56; in M57, it will be disabled unless the EnableSha1ForLocalAnchors policy is set, as described at https://www.chromium.org/Home/chromium-security/education/tls/sha-1 As with other TLS deprecations, an emergency 'undeprecate' switch is kept around in the event of unexpected breakage, to allow rapid reverting to the previous behaviour. BUG=653691 ========== to ========== Distrust publicly trusted SHA-1 certs Reject all publicly trusted SHA-1 certificates, as announced September 2014 at https://security.googleblog.com/2014/09/gradually-sunsetting-sha-1.html and https://blog.chromium.org/2014/09/gradually-sunsetting-sha-1.html To avoid too much disruption, enterprise SHA-1 is still allowed for M56; in M57, it will be disabled unless the EnableSha1ForLocalAnchors policy is set, as described at https://www.chromium.org/Home/chromium-security/education/tls/sha-1 As with other TLS deprecations, an emergency 'undeprecate' switch is kept around in the event of unexpected breakage, to allow rapid reverting to the previous behaviour. BUG=653691 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Distrust publicly trusted SHA-1 certs Reject all publicly trusted SHA-1 certificates, as announced September 2014 at https://security.googleblog.com/2014/09/gradually-sunsetting-sha-1.html and https://blog.chromium.org/2014/09/gradually-sunsetting-sha-1.html To avoid too much disruption, enterprise SHA-1 is still allowed for M56; in M57, it will be disabled unless the EnableSha1ForLocalAnchors policy is set, as described at https://www.chromium.org/Home/chromium-security/education/tls/sha-1 As with other TLS deprecations, an emergency 'undeprecate' switch is kept around in the event of unexpected breakage, to allow rapid reverting to the previous behaviour. BUG=653691 ========== to ========== Distrust publicly trusted SHA-1 certs Reject all publicly trusted SHA-1 certificates, as announced September 2014 at https://security.googleblog.com/2014/09/gradually-sunsetting-sha-1.html and https://blog.chromium.org/2014/09/gradually-sunsetting-sha-1.html To avoid too much disruption, enterprise SHA-1 is still allowed for M56; in M57, it will be disabled unless the EnableSha1ForLocalAnchors policy is set, as described at https://www.chromium.org/Home/chromium-security/education/tls/sha-1 As with other TLS deprecations, an emergency 'undeprecate' switch is kept around in the event of unexpected breakage, to allow rapid reverting to the previous behaviour. BUG=653691 Committed: https://crrev.com/63efb444af72ffca8e96a9df770c05dc97f241bb Cr-Commit-Position: refs/heads/master@{#430866} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/63efb444af72ffca8e96a9df770c05dc97f241bb Cr-Commit-Position: refs/heads/master@{#430866} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
