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)

Product:
Core
Component:
Security: PSM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [psm-assigned])

Attachments

(1 file)

bug 1302140 - add policy to disable SHA-1 except for certificates issued by non-built-in CAs
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 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.
Attachment #8790990 - Flags: review?(rlb) → 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 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 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 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 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+
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.
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).
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 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+
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
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
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)
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)
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
https://hg.mozilla.org/mozilla-central/rev/5436f8c05f6d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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 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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/43c724bde81c
status-firefox51: affected → fixed
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
See Also: → 1766687
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: