Closed
Bug 1302140
Opened 8 years ago
Closed 8 years ago
add policy mode to completely disallow sha-1 signature except for certificates issued by non-built-in roots
Categories
(Core :: Security: PSM, defect, P1)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: keeler, Assigned: keeler)
References
Details
(Keywords: dev-doc-complete, site-compat, Whiteboard: [psm-assigned])
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
jcj
:
review+
rbarnes
:
review+
gchang
:
approval-mozilla-aurora+
|
Details |
We would like to support a policy of not allowing sha-1 signatures on certificates at all, except for those issued by imported (i.e. non-built-in) root certificates.
Comment hidden (mozreview-request) |
Comment 2•8 years ago
|
||
mozreview-review |
Comment on attachment 8790990 [details] bug 1302140 - add policy to disable SHA-1 except for certificates issued by non-built-in CAs https://reviewboard.mozilla.org/r/78560/#review77166 ::: security/certverifier/CertVerifier.h:115 (Diff revision 1) > enum class SHA1Mode { > Allowed = 0, > Forbidden = 1, > Before2016 = 2, > - ImportedRoot = 3, > + ImportedRootOrBefore2016 = 3, > + ImportedRoot = 4, If we're worried about having a simple telescoping hierarchy here, I would be OK removing the pure Before2016 policy. I don't really see us using it; by the time we're ready to turn off imported roots, we're not going to be worried about certs that old. That would give you: Forbidden > ImportedRoot > ImportedRootOrBefore2016 > Allowed It does seem like that would make some of the below less confusing. I guess there's some risk that users out there have configured that value, so we would have to think abotu what we would do with the code point. I would be inclined to map it to Forbidden, to fail safe. .... After having read the remainder of the patch, I would like to make this change. The logic right now is exceeding my complexity threshold.
Updated•8 years ago
|
Attachment #8790990 -
Flags: review?(rlb) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•8 years ago
|
||
mozreview-review |
Comment on attachment 8790990 [details] bug 1302140 - add policy to disable SHA-1 except for certificates issued by non-built-in CAs https://reviewboard.mozilla.org/r/78562/#review77456 ::: netwerk/base/security-prefs.js:58 (Diff revision 1) > pref("security.OCSP.GET.enabled", false); > > pref("security.pki.cert_short_lifetime_in_days", 10); > // NB: Changes to this pref affect CERT_CHAIN_SHA1_POLICY_STATUS telemetry. > // See the comment in CertVerifier.cpp. > -// 3 = allow SHA-1 for certificates issued before 2016 or by an imported root. > +// 4 = allow SHA-1 only for certificates issued by an imported root. Whoops - I forgot to update this comment. I'll be sure to include that in a future version.
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8790990 [details] bug 1302140 - add policy to disable SHA-1 except for certificates issued by non-built-in CAs https://reviewboard.mozilla.org/r/78562/#review77586 One question about migrating prefs. Otherwise, LGTM. ::: security/certverifier/CertVerifier.cpp:477 (Diff revision 2) > NSSCertDBTrustDomain trustDomain(trustSSL, defaultOCSPFetching, > mOCSPCache, pinArg, ocspGETConfig, > mCertShortLifetimeInDays, > pinningDisabled, MIN_RSA_BITS_WEAK, > ValidityCheckingMode::CheckingOff, > - mSHA1Mode, mNetscapeStepUpPolicy, > + SHA1Mode::Allowed, mNetscapeStepUpPolicy, This is a good catch; there's no test - nor ability to test - modes we may some day apply to roots. obviously no reason to add that in this patch, but I was puzzling through what prompted this before I realized you were fixing behavior. ::: security/manager/ssl/nsNSSComponent.cpp:1530 (Diff revision 2) > break; > } > > + // Convert a previously-available setting to a safe one. > + if (sha1Mode == CertVerifier::SHA1Mode::UnusedPreviouslyBefore2016) { > + sha1Mode = CertVerifier::SHA1Mode::Forbidden; Should we make an attempt to reset the pref?
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8790990 [details] bug 1302140 - add policy to disable SHA-1 except for certificates issued by non-built-in CAs https://reviewboard.mozilla.org/r/78562/#review78104 Oh, hey... with our plan to use a SHIELD study to change the defaults, it looks like we should consider a different approach to [CertVerifier line 479](http://searchfox.org/mozilla-central/source/security/certverifier/CertVerifier.cpp#479).
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8790990 [details] bug 1302140 - add policy to disable SHA-1 except for certificates issued by non-built-in CAs https://reviewboard.mozilla.org/r/78562/#review78194 ::: security/certverifier/CertVerifier.h:113 (Diff revision 2) > }; > > enum class SHA1Mode { > Allowed = 0, > Forbidden = 1, > - Before2016 = 2, > + // This policy is no longer available ("Unused, previously 'Before2016'") It would be good to note what happens if this value is set, namely that it gets converted to Forbidden. (Tempting to name it something like UsedToBeBefore2016ButNowForbidden)
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8790990 [details] bug 1302140 - add policy to disable SHA-1 except for certificates issued by non-built-in CAs https://reviewboard.mozilla.org/r/78562/#review78196
Attachment #8790990 -
Flags: review?(rlb) → review+
Assignee | ||
Comment 9•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8790990 [details] bug 1302140 - add policy to disable SHA-1 except for certificates issued by non-built-in CAs https://reviewboard.mozilla.org/r/78562/#review77586 Thanks! > This is a good catch; there's no test - nor ability to test - modes we may some day apply to roots. obviously no reason to add that in this patch, but I was puzzling through what prompted this before I realized you were fixing behavior. Right - I think we originally made this change a little optimistically, but since not all policies can be enforced the way we validate intermediates/CAs and for the reasoning mentioned in test_cert_sha1.js, I'm just reverting it to the old behavior. > Should we make an attempt to reset the pref? I'm not sure. My sense is users might find either situation weird (either we change the value of a pref they had deliberately set, or them setting it to that value no longer has the expected effect, even though it doesn't change). Since resetting the pref involves more code, I'm inclined to just reinterpret the old value.
Assignee | ||
Comment 10•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8790990 [details] bug 1302140 - add policy to disable SHA-1 except for certificates issued by non-built-in CAs https://reviewboard.mozilla.org/r/78562/#review78104 Right - I had sort-of neglected that. The new patch will effectively keep the default as the current policy of "imported roots or before 2016" with the option to enable "only imported roots". We'll then use SHIELD to enable the new option when the time comes (and eventually we'll make that the default).
Assignee | ||
Comment 11•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8790990 [details] bug 1302140 - add policy to disable SHA-1 except for certificates issued by non-built-in CAs https://reviewboard.mozilla.org/r/78562/#review78194 > It would be good to note what happens if this value is set, namely that it gets converted to Forbidden. (Tempting to name it something like UsedToBeBefore2016ButNowForbidden) Good call.
Comment hidden (mozreview-request) |
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8790990 [details] bug 1302140 - add policy to disable SHA-1 except for certificates issued by non-built-in CAs https://reviewboard.mozilla.org/r/78562/#review78750 Thanks, LGTM!
Attachment #8790990 -
Flags: review?(jjones) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•8 years ago
|
||
Thanks for the reviews. The latest push just updated the commit message to be more accurate. Here's the try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd9884d63e7e
Comment 16•8 years ago
|
||
Pushed by dkeeler@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2df66e8b7411 add policy to disable SHA-1 except for certificates issued by non-built-in CAs r=jcj,rbarnes
Comment 17•8 years ago
|
||
Backed out for Windows build bustage in CertVerifier.cpp: https://hg.mozilla.org/integration/autoland/rev/d8b95e0d8843 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2df66e8b7411fa3f7a998f9d87dc967371577c17 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=3809152&repo=autoland c:/builds/moz2_slave/autoland-w32-00000000000000000/build/src/security/certverifier/CertVerifier.cpp(164): error C2220: warning treated as error - no 'object' file generated
Flags: needinfo?(dkeeler)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•8 years ago
|
||
Looks like MSVC was "helpfully" warning that not all cases in the enum class were being handled (apparently ignoring the default label?). In any case, I added the additional case for completeness. Here's the try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3013b2385965
Flags: needinfo?(dkeeler)
Comment 20•8 years ago
|
||
Pushed by dkeeler@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5436f8c05f6d add policy to disable SHA-1 except for certificates issued by non-built-in CAs r=jcj,rbarnes
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5436f8c05f6d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Comment 22•8 years ago
|
||
Comment on attachment 8790990 [details] bug 1302140 - add policy to disable SHA-1 except for certificates issued by non-built-in CAs Approval Request Comment [Feature/regressing bug #]: deprecating SHA-1 signatures on certificates [User impact if declined]: we won't have the ability to protect users against certificates issued using a hash algorithm that is being deprecated due to potential attacks against it (we basically need this capability on release channels in early 2017, which means we need to get this into 51). [Describe test coverage new/current, TreeHerder]: has tests [Risks and why]: low-medium (this is a security-sensitive part of the code base, but we have good test coverage and we've made changes like this before. Additionally, telemetry from Nightly hasn't indicated any issues so far.) [String/UUID change made/needed]: none
Attachment #8790990 -
Flags: approval-mozilla-aurora?
Comment 23•8 years ago
|
||
Comment on attachment 8790990 [details] bug 1302140 - add policy to disable SHA-1 except for certificates issued by non-built-in CAs Better protect users from potential attack. Take it in 51 aurora.
Attachment #8790990 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 24•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/43c724bde81c
Comment 25•8 years ago
|
||
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/sha-1-certificates-issued-by-public-ca-will-no-longer-be-accepted/
Keywords: dev-doc-needed,
site-compat
Comment 26•8 years ago
|
||
Added to https://developer.mozilla.org/en-US/Firefox/Releases/51#Networking which refers to https://blog.mozilla.org/security/2016/10/18/phasing-out-sha-1-on-the-public-web/
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•