+++ to secure your transactions use the Bitcoin Mixer Service +++

 

We need Your Support. Make a Donation now! or Buy Me A Coffee

Closed Bug 942515 Opened 11 years ago Closed 8 years ago

Show Untrusted Connection Error for SHA-1-based SSL certificates with notBefore >= 2016-01-01

Categories

(Core :: Security: PSM, enhancement)

Product:
Core
Component:
Security: PSM
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox43 --- disabled
firefox44 --- disabled
firefox45 --- disabled
firefox46 --- disabled

People

(Reporter: briansmith, Assigned: rbarnes)

References

(
URL
)

Details

(5 keywords, Whiteboard: [adv-main43-][psm-assigned])

Attachments

(1 file, 6 obsolete files)

bug-942515.4.patch
79.96 KB, patch
rbarnes
: review+
Details | Diff | Splinter Review 
See Microsoft's announcement of the deprecation of SHA1-based certificates:
http://blogs.technet.com/b/pki/archive/2013/11/12/sha1-deprecation-policy.aspx

We should support this effort by enforcing that no newly-issued SHA1-based certificates are issued after March 1st of next year. This way, we ensure that we will be more likely to actually turn off support for SHA1-based certificates on 2017-01-01.

Microsoft's original announcement stressed that it applies to "end-entity" certificates. However, in the comments on that page, they clarified:

"[Question:] Does this really only affect end-entitiy certificates and not root- and intermediate-certificates?

"Answer: The policy affects intermediate and end-entity certificates - both intermediates and end-entity certs should transition to SHA2 before the deadlines.  Root certs aren’t validated by the SHA1 signature so they are unaffected by this policy at this time."

However, there was another question on the page that has still not been answered:

"Do certificates signing CRLs and OCSP responses have to be SHA-2 certificates?
Do the CRLs and OCSP responses themself have to be signed with SHA-2?"

We should get clarification from Microsoft about OCSP. We may need to do the enforcement for OCSP in a separate bug. Note that currently NSS does not support SHA2 for OCSP completely yet. See bug 622332. This is something we should be able to fix very easily in insanity::pkix's OCSP processing. We should also contribute the fix to NSS if we have any resources to do so.
Brian, I hope you don't mind me tweaking this bug's summary.  I presume NSS doesn't intend to stop accepting certs with SHA-2-based signatures anytime soon!
Summary: Stop accepting SSL certificates with notAfter >= 2017-01-01 and notBefore >= 2014-03-01 → Stop accepting SHA-1-based SSL certificates with notAfter >= 2017-01-01 and notBefore >= 2014-03-01
(In reply to Brian Smith)
> Note that currently NSS does not support SHA2 for OCSP completely yet. See bug 622332.

Brian, did you mean bug 663315 ?

AFAICT, bug 622332 is only a UI issue.
(In reply to Rob Stradling from comment #1)
> Brian, I hope you don't mind me tweaking this bug's summary.  I presume NSS
> doesn't intend to stop accepting certs with SHA-2-based signatures anytime
> soon!

The new description doesn't accurately capture what is being discussed. Namely, the proposal is to match Microsoft's deprecation of SHA-1 by entirely disabling it - in 2017. in addition (but not in lieu of), new certificates issued post 2014 should also be SHA-2 (conditional upon their expiration). The topic change only captures the latter, while the former remains important (especially since the validity dates are part of the SHA-1 signed content).

Just a minor quibble :)
Ryan, in that case, please suggest an alternative description that does accurately capture what is being discussed.  :-)
Summary: Stop accepting SHA-1-based SSL certificates with notAfter >= 2017-01-01 and notBefore >= 2014-03-01 → Stop accepting all SHA-1-based SSL certificates after 2017-01-01 and certs with notBefore >= 2014-03-01 and notAfter >= 2017-01-01
Ryan, I know you intended it to apply to SHA-1-based SSL certs only, but I read the second requirement in your new description as...
"Stop accepting...certs with notBefore >= 2014-03-01 and notAfter >= 2017-01-01"
Summary: Stop accepting all SHA-1-based SSL certificates after 2017-01-01 and certs with notBefore >= 2014-03-01 and notAfter >= 2017-01-01 → stop accepting SHA-1-based SSL certificates with notBefore >= 2014-03-01 and notAfter >= 2017-01-01, or any SHA-1-based SSL certificates after 2017-01-01
Re: the "or any SHA-1-based SSL certificates after 2017-01-01" part: Microsoft has announced this deadline but until they ship patches, they do have the option of extending it. If we put code in the field now implementing this restriction, then we have no way to back down. There is a risk that we get left high and dry by Microsoft changing their policy for some compatibility reason.

Re: the "stop accepting SHA-1-based SSL certificates with notBefore >= 2014-03-01 and notAfter >= 2017-01-01", we should make that 2013-04-01, to match the ballot currently being run at the CAB Forum.

Gerv
(In reply to Gervase Markham [:gerv] from comment #6)
> Re: the "or any SHA-1-based SSL certificates after 2017-01-01" part:
> Microsoft has announced this deadline but until they ship patches, they do
> have the option of extending it. If we put code in the field now
> implementing this restriction, then we have no way to back down. There is a
> risk that we get left high and dry by Microsoft changing their policy for
> some compatibility reason.

We change the code every 6 weeks. The important thing is that *we* should choose *our* policy, and try to stick to it, but be willing to change the code if that policy cannot be implemented.

> Re: the "stop accepting SHA-1-based SSL certificates with notBefore >=
> 2014-03-01 and notAfter >= 2017-01-01", we should make that 2013-04-01, to
> match the ballot currently being run at the CAB Forum.

I don't want to make it retroactive, because that add compatibility risk. I don't want to deal with an avalanche of bug reports that would likely come from that. Giving CAs a multi-month heads-up that this change is coming will give them time to change their practices. This change is going to be pretty risky due to compatibility risk anyway.

I am tentatively assigning this to cviecco because he expressed interest in the past regarding adding these kinds of checks (also key size checks). I think we should aim to get this into the next release. But, I also recommend not spending too much time on it before insanity::pkix is ready. Otherwise, we'll probably have to do a lot of extra work.
Assignee: nobody → cviecco
(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #7)
> (In reply to Gervase Markham [:gerv] from comment #6)
> > Re: the "or any SHA-1-based SSL certificates after 2017-01-01" part:
> > Microsoft has announced this deadline but until they ship patches, they do
> > have the option of extending it. If we put code in the field now
> > implementing this restriction, then we have no way to back down. There is a
> > risk that we get left high and dry by Microsoft changing their policy for
> > some compatibility reason.
> 
> We change the code every 6 weeks. 

But we don't have 100% uptake of new versions.

> The important thing is that *we* should
> choose *our* policy, and try to stick to it, but be willing to change the
> code if that policy cannot be implemented.

OK... but what is the advantage of inserting a patch to implement a 2017-01-01 SHA-1 no-support deadline now, instead of, say, on 2016-04-01?

> > Re: the "stop accepting SHA-1-based SSL certificates with notBefore >=
> > 2014-03-01 and notAfter >= 2017-01-01", we should make that 2013-04-01, to
> > match the ballot currently being run at the CAB Forum.
> 
> I don't want to make it retroactive, because that add compatibility risk. 

Oops, sorry! I meant 2014-04-01, not 2013-04-01.

Gerv
(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #0)
> We should get clarification from Microsoft about OCSP. We may need to do the
> enforcement for OCSP in a separate bug.

I think we should probably insist that an OCSP response for a SHA-2-signed certificate has a SHA-2 (or better) based signature. But, we need to verify with CABForum that this is the case. I think that CAs will be concerned about clients' support for SHA-2-based signatures for OCSP responses. It should be safe for them to assume that any implementation that can process a SHA-2-signed certificate should be able to process a SHA-2-signed OCSP response, if it processes OCSP responses at all. The code to verify signatures is usually shared between certificate verification and OCSP response verification.

> Note that currently NSS does not support SHA2 for OCSP completely yet.
> See bug 622332.

In an OCSP response, there are two kinds of hashes. The first hash is the hash used in the digital signature of the OCSP response. AFAICT, NSS and NSS-based products support SHA-2 in the signature just fine. The second hash is the one used to identify which certificate a SingleResponse inside the OCSP response is for. NSS does *not* support SHA-2 for that hash, and adding SHA-2 support for that part is what bug 622332 is about.
(In reply to Gervase Markham [:gerv] from comment #8)
> (In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if
> you want a response) from comment #7)
> > We change the code every 6 weeks. 
> 
> But we don't have 100% uptake of new versions.

Right. So, if it is important for security to stop accepting SHA-1 certificates, we should have that change in several releases before January 1, 2017 to maximize the number of people who are protected by the new policy.

> OK... but what is the advantage of inserting a patch to implement a
> 2017-01-01 SHA-1 no-support deadline now, instead of, say, on 2016-04-01?

1. Rhetoric. Having the code committed is a strong statement of support.

2. Efficiency. It will be much easier for the person doing the first part (newly-issued SHA-1 certificates must expire on or before January 1, 2017) to add a line of code and a test now to enforce the second part (no SHA-1-based certificates accepted after January 1, 2017, regardless of notBefore). Conversely, doing it later means having to rehash everything at that later point in time.
(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #9)
> > Note that currently NSS does not support SHA2 for OCSP completely yet.
> > See bug 622332.
> 
> In an OCSP response, there are two kinds of hashes. The first hash is the
> hash used in the digital signature of the OCSP response. AFAICT, NSS and
> NSS-based products support SHA-2 in the signature just fine. The second hash
> is the one used to identify which certificate a SingleResponse inside the
> OCSP response is for. NSS does *not* support SHA-2 for that hash, and adding
> SHA-2 support for that part is what bug 622332 is about.

And, of course, I meant bug 663315, not bug 622332.
After all the edits of the bug description, I think it would make sense to correct the following snippet as well:

(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #0)
> We should support this effort by enforcing that no newly-issued SHA1-based
> certificates are issued after March 1st of next year. This way, we ensure
> that we will be more likely to actually turn off support for SHA1-based
> certificates on 2017-01-01.

"no newly-issued SHA1-based certificates are issued after March 1st of next year" is not what Microsoft's policy states, and I assume it's also not what Brian was really intending to say. In my understanding, this bug is about "rejecting SHA1-based certificates issued after 2014-03-01 with a validity extending past 2016-12-31". The crucial question still remains whether this should apply to end-entity certs only, or also to intermediate CA certificates (not to root certificates, of course - insisting on a specific hash for the root is absolutely pointless, its signature is irrelevant).

> We should get clarification from Microsoft about OCSP.

Not only about OCSP, but also about their "SHA1 Deprecation Policy" in the technical requirements of their program (http://social.technet.microsoft.com/wiki/contents/articles/1760.windows-root-certificate-program-technical-requirements-version-2-0.aspx), which currently states:

> For SSL certificates, Windows will stop accepting SHA1 certificates by 1 January 2017.
> This means any SHA1 SSL certificates issued before or after this announcement must be
> replaced with a SHA2 equivalent by 1 January 2017.

(A rough answer from "Amerk" buried deep down in some Windows PKI blog post is certainly not what CAs and relying parties need to know when preparing for a rather drastic step like turning off SHA1 signature support for certificates.)

(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #9)
> NSS-based products support SHA-2 in the signature just fine.

True. Can be verified like this:

1) set the NSS_HASH_ALG_SUPPORT environment variable to "-SHA-1"

2) start Firefox from the environment with that variable set

3) visit https://2029.globalsign.com (an EV site where you receive two sha256WithRSAEncryption signed OCSP responses)

(For additional fun, try the same exercise with NSS_HASH_ALG_SUPPORT set to "-SHA-256", and also experiment with the "When an OCSP server connection fails, treat the certificate as invalid" setting ["security.OCSP.require" pref].)
I do have several further things to say, but I think there's enough of a discussion about the right way forward here that we should take this back to mozilla.dev.security.policy to work out the policy questions. We can then come back here for implementation.

Brian: might you be able to kick off the discussion with your current proposal?

Gerv
I wonder how many CAs can completely switch to issuing SHA-2 certificate within just a few months, if those CAs don't have any SHA-2 intermediate certificate at all now. As shown in the CAs' responses to Mozilla's questions regarding compliance to the baseline requirements of CABForum, CAs take time to make changes even though CAs are keen on following some "standards". If Mozilla changes the CA Certificate Policy without taking the balance of security and practicality, the outcome may be in favor to a few CAs who have already been issuing SHA2-based SSL certificate or at least got SHA2 intermediate certificate ready.

BTW, what is the effect/impact to the user when Mozilla "stop accepting SHA1-based SSL certificate"?


Man Ho
(In reply to Man Ho from comment #14)
> I wonder how many CAs can completely switch to issuing SHA-2 certificate
> within just a few months, if those CAs don't have any SHA-2 intermediate
> certificate at all now. 

No-one is asking them to.

What this prevents is them issuing certs after 2014-03/4-01 which _expire_ after 2017-01-01. They are free to continue issuing certs with a shorter life than that.

> BTW, what is the effect/impact to the user when Mozilla "stop accepting
> SHA1-based SSL certificate"?

There will be little _extra_ impact, because Microsoft are doing the same thing. So basically, such certs will stop working for SSL on the web.

Gerv
(In reply to Gervase Markham [:gerv] from comment #15)
> No-one is asking them to.
> 
> What this prevents is them issuing certs after 2014-03/4-01 which _expire_
> after 2017-01-01. They are free to continue issuing certs with a shorter
> life than that.
> 

However, if Mozilla set the date limits in NSS now, the impact created will be effect immediately. Unlike Microsoft's approach that basically make the deprecation of SHA-1 certificate effect from 1-Jan-2016, with the same target of deprecating all SHA-1 certificate by 1-Jan-2017.

> 
> There will be little _extra_ impact, because Microsoft are doing the same
> thing. So basically, such certs will stop working for SSL on the web.
> 

You mean the SSL connection cannot be established, so web server will return Error 500. Right?
(In reply to Man Ho from comment #16)
> (In reply to Gervase Markham [:gerv] from comment #15)
> However, if Mozilla set the date limits in NSS now, the impact created will
> be effect immediately. Unlike Microsoft's approach that basically make the
> deprecation of SHA-1 certificate effect from 1-Jan-2016, with the same
> target of deprecating all SHA-1 certificate by 1-Jan-2017.

You will note I made exactly this point in comment 6.

> You mean the SSL connection cannot be established, so web server will return
> Error 500. Right?

No. They will get an SSL certificate error from the browser. It never gets as high as the HTTP level (500 is an HTTP error).

Gerv
Keywords: site-compat
Keywords: relnote
Depends on: 966856
Keywords: sec-want
No longer depends on: 663315, 943651, 949918
See Also: → 949918, 943651
I changed some of the dependencies. As far as OCSP is concerned, mozilla::pkix's OCSP implementation already supports SHA-2 signatures. The SHA-2 CertID matching should be almost completely unrelated to this.
No longer depends on: 966856
See Also: → 966856
thought it might be worth linking to this post from Google regarding their Sunsetting of SHA-1 based certificates and how they are displayed visually to the user starting from Chrome 39 (Branch point 26 September 2014)

http://googleonlinesecurity.blogspot.co.uk/2014/09/gradually-sunsetting-sha-1.html
See Also: → 1068949
to late for 29, hopefully we can get this for ESR 38?
Target Milestone: mozilla29 → ---
https://blog.mozilla.org/security/2014/09/23/phasing-out-certificates-with-sha-1-based-signature-algorithms/
"after January 1, 2016, we plan to show the “Untrusted Connection” error whenever a newly issued SHA-1 certificate is encountered in Firefox. After January 1, 2017, we plan to show the “Untrusted Connection” error whenever a SHA-1 certificate is encountered in Firefox."
Is there a way to disable this warning in the console (Firebug) without disabling warnings outright? I don't care about it being logged in general though for localhost development it's absolutely obnoxious and I doubt I'm spying on myself. I'm using Firefox 9 (38 ESR).
(In reply to John A. Bilicki III from comment #23)
> Is there a way to disable this warning in the console (Firebug)

Asking the Firebug developers would be better than asking in a vaguely related Mozilla bug.

In Firefox, you can easily disable security warnings whilst leaving enabled security errors and all other messages by clicking on the arrow next to the "Security" toggle button and unchecking warnings. (browser console is ctrl+shift+J; web console is ctrl+shift+K)

> I'm using Firefox 9 (38 ESR).

This doesn't make any sense. ;)
(In reply to Kathleen Wilson from comment #22)
> "after January 1, 2016, we plan to show the “Untrusted Connection” error
> whenever a newly issued SHA-1 certificate is encountered in Firefox. After

How is "newly issued" defined?
(In reply to Patrick Ryan from comment #25)
> (In reply to Kathleen Wilson from comment #22)
> > "after January 1, 2016, we plan to show the “Untrusted Connection” error
> > whenever a newly issued SHA-1 certificate is encountered in Firefox. After
> 
> How is "newly issued" defined?

Has notBefore >= 2016-01-01
(In reply to Kathleen Wilson from comment #26)
> (In reply to Patrick Ryan from comment #25)
> > (In reply to Kathleen Wilson from comment #22)
> > > "after January 1, 2016, we plan to show the “Untrusted Connection” error
> > > whenever a newly issued SHA-1 certificate is encountered in Firefox. After
> > 
> > How is "newly issued" defined?
> 
> Has notBefore >= 2016-01-01


Note that this lines up with CA expectations and plans:

https://mozillacaprogram.secure.force.com/Communications/CommunicationActionOptionResponse?CommunicationId=a04o000000M89RCAAZ&Question=ACTION%20%233:%20After%20January%201,%202016
Should we change the title of this bug to: "Show Untrusted Connection Error for SHA-1-based SSL certificates with notBefore >= 2016-01-01"?
Summary: stop accepting SHA-1-based SSL certificates with notBefore >= 2014-03-01 and notAfter >= 2017-01-01, or any SHA-1-based SSL certificates after 2017-01-01 → Show Untrusted Connection Error for SHA-1-based SSL certificates with notBefore >= 2016-01-01
AFAICT, this would have to be done before 2015-09-21 (three weeks from now) in order for it to be effective in the Firefox release channel on 2016-01-01, but it is still unassigned. Who's going to implement it?

Also, the new summary seems like a recipe for weak sauce. The notBefore date matters less than the notAfter date. Note that the spreadsheet above says "valid beyond January 1, 2017" which is referring to the notAfter date. The policy should be stated in terms of a function of the notAfter date.
Assignee: cviecco → nobody
Flags: needinfo?(rlb)
Added back the latter part of the old summary.
Summary: Show Untrusted Connection Error for SHA-1-based SSL certificates with notBefore >= 2016-01-01 → Show Untrusted Connection Error for SHA-1-based SSL certificates with notBefore >= 2016-01-01 and notAfter >= 2017-01-01, or any SHA-1-based SSL certificates after 2017-01-01
Joining the title-editing party.  I think version before :emk's edit is more correct.  The only MUST NOT in the BRs right now is the prohibition on SHA-1 after 2016-01-01.  (I might be willing to entertain a restriction on certs expiring after 2017-01-01, but we would need telemetry first.)  If we want to reject SHA-1 certificates entirely at a certain date, we'll land that patch then.
Flags: needinfo?(rlb)
Summary: Show Untrusted Connection Error for SHA-1-based SSL certificates with notBefore >= 2016-01-01 and notAfter >= 2017-01-01, or any SHA-1-based SSL certificates after 2017-01-01 → Show Untrusted Connection Error for SHA-1-based SSL certificates with notBefore >= 2016-01-01
To Brian's question: I'll take this.  I think I've got enough bandwidth the next couple of weeks to get it landed.

I think the thing to do here is something like the following:

1. Define an integer pref security.pki.sha1_enforcement_level, with values:
   0 = allow SHA-1
   1 = forbid SHA-1
   2 = allow SHA-1 only if notBefore < 2016-01-01
2. Plumb it through like the revocation prefs (nsNSSComponent -> SharedCertVerifier -> CertVerifier)
3. In CertVerifier::VerifySSLServerCert, add a switch on mSHA1EnforcementLevel, in which ...
4. If the cert is forbidden by levels (1) or (2), return ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED

Using the enforcement level pref makes it possible for users to turn this prohibition off if they need to, and it gives us an easy way to reject SHA-1 certificates entirely in the future.  It feels slightly odd to be special-casing SHA-1, but at the same time, I don't really see a useful way to generalize.

Brian, Keeler, does that sound right to you?
Flags: needinfo?(dkeeler)
Flags: needinfo?(brian)
(In reply to Richard Barnes [:rbarnes] from comment #32)
> To Brian's question: I'll take this.  I think I've got enough bandwidth the
> next couple of weeks to get it landed.
> 
> I think the thing to do here is something like the following:
> 
> 1. Define an integer pref security.pki.sha1_enforcement_level, with values:
>    0 = allow SHA-1
>    1 = forbid SHA-1
>    2 = allow SHA-1 only if notBefore < 2016-01-01
> 2. Plumb it through like the revocation prefs (nsNSSComponent ->
> SharedCertVerifier -> CertVerifier)
> 3. In CertVerifier::VerifySSLServerCert, add a switch on
> mSHA1EnforcementLevel, in which ...

Or maybe this should be in CheckIssuerIndependentProperties() in pkixcheck.cpp?
(In reply to Richard Barnes [:rbarnes] from comment #33)
> (In reply to Richard Barnes [:rbarnes] from comment #32)
> > To Brian's question: I'll take this.  I think I've got enough bandwidth the
> > next couple of weeks to get it landed.
> > 
> > I think the thing to do here is something like the following:
> > 
> > 1. Define an integer pref security.pki.sha1_enforcement_level, with values:
> >    0 = allow SHA-1
> >    1 = forbid SHA-1
> >    2 = allow SHA-1 only if notBefore < 2016-01-01
> > 2. Plumb it through like the revocation prefs (nsNSSComponent ->
> > SharedCertVerifier -> CertVerifier)
> > 3. In CertVerifier::VerifySSLServerCert, add a switch on
> > mSHA1EnforcementLevel, in which ...
> 
> Or maybe this should be in CheckIssuerIndependentProperties() in
> pkixcheck.cpp?

Or in TrustDomain::CheckValidityIsAcceptable (passing in the signature algorithm as an additional parameter)?
Attached patch bug-942515.0.patch (obsolete) — — Splinter Review 
Better to say it with code. I think it represents a workable approach:

* Add a pref security.pki.sha1_enforcement_level
  0 => Allowed
  1 => Forbidden
  2 => Only before 2016
* Add a notBefore argument to TrustDomain::CheckSignatureDigestAlgorithm() 
* Plumb the pref through to NSSCertDBTrustDomain
* Apply the pref in NSSCertDBTrustDomain::CheckSignatureDigestAlgorithm()

This patch has been manually tested for the sha1Forbidden case, and it works for that case.  It obviously needs real testing.

bsmith, keeler: Feedback on the approach here?
Assignee: nobody → rlb
Status: NEW → ASSIGNED
Attachment #8654892 - Flags: feedback?(dkeeler)
Attachment #8654892 - Flags: feedback?(brian)
Flags: needinfo?(dkeeler)
Flags: needinfo?(brian)
Comment on attachment 8654892 [details] [diff] [review]
bug-942515.0.patch

Review of attachment 8654892 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't look at the patch carefully, but I found some issues.

::: security/certverifier/CertVerifier.h
@@ +101,5 @@
>      pinningStrict = 2,
>      pinningEnforceTestMode = 3
>    };
>  
> +  enum SHA1Mode {

Use |enum class| to improve the safety checks that the compiler performs.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +34,5 @@
>  using namespace mozilla::pkix;
>  
>  extern PRLogModuleInfo* gCertVerifierLog;
>  
> +static const uint64_t JanuaryFirst2016FromEpoch = 1451606400000;

You should expand out the addition/multiplication chain that arrives at this number so that a reviewer can easily see it is correct by inspection. In particular, I'd like to see how the leap years were considered. (No magic numbers.)


Constants should be NAMED_LIKE_THIS.

This constant should be moved to the function that uses it. (Minimal scope.)

@@ +817,5 @@
>  
>  Result
>  NSSCertDBTrustDomain::CheckSignatureDigestAlgorithm(DigestAlgorithm aAlg,
> +                                                    EndEntityOrCA endEntityOrCA,
> +                                                    Time notBefore)

Again, I'll re-iterate that I think it is most important to reject SHA-1 certificates based on whether they have notAfter >= 2017-1-1 than it is to reject certificates because they have notBefore >= 2016-1-1.

Microsoft's stated policy is that Windows will stop accepting SHA-1 certificates on 2017-1-1, regardless of notAfter/notBefore. And IIRC Google's policy is similar. So, what I'm suggesting is consistent with them.

I realize that the compatibility impact of rejecting based on notAfter is a concern. However, I understand that Mozilla has pretty good tools to assess the compatibility impact now. Plus we can always roll back the notAfter checks if they end up rejecting too many certificates.

@@ +827,5 @@
>    MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
>            ("NSSCertDBTrustDomain: CheckSignatureDigestAlgorithm"));
>    if (aAlg == DigestAlgorithm::sha1) {
> +    // First check based on SHA1Mode
> +    if (mSHA1Mode == CertVerifier::sha1Forbidden) {

Case analysis on enums should be done swith switch/case so that the compiler and static analysis tools will help you with the analysis of the exhaustiveness of the case analysis.

@@ +831,5 @@
> +    if (mSHA1Mode == CertVerifier::sha1Forbidden) {
> +      return Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED;
> +    }
> +
> +    if ((mSHA1Mode == CertVerifier::sha1OnlyBefore2016)&&

s/&&/ &&/

@@ +842,5 @@
>        return Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED;
>      }
>  
>      if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
> +      MOZ_LOG(gCertVerifierLog, LogLevel::Debug, ("CA cert is SHA1"));

It seems like this log statement should be higher up so that the logging happens before the algorithm is rejected.

@@ +847,5 @@
>        return mSignatureDigestOption == DisableSHA1ForCA
>               ? Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED
>               : Success;
>      } else {
> +      MOZ_LOG(gCertVerifierLog, LogLevel::Debug, ("EE cert is SHA1"));

Ditto.

::: security/certverifier/OCSPVerificationTrustDomain.h
@@ +29,5 @@
>  
>    virtual Result CheckSignatureDigestAlgorithm(
>                     mozilla::pkix::DigestAlgorithm digestAlg,
> +                   mozilla::pkix::EndEntityOrCA endEntityOrCA,
> +                   mozilla::pkix::Time notBefore) override;

Add a notAfter attribute too.

::: security/pkix/lib/pkixcheck.cpp
@@ +34,5 @@
>  
>  Result
>  CheckSignatureAlgorithm(TrustDomain& trustDomain,
>                          EndEntityOrCA endEntityOrCA,
> +                        Time notBefore,

You should add notAfter here too.

@@ +862,5 @@
> +  // this method, in order to preserve error ranking.
> +  Time notBefore(Time::uninitialized);
> +  Time notAfter(Time::uninitialized);
> +  Result deferredValidityRV = CheckValidity(cert.GetValidity(), time, &notBefore, &notAfter);
> +  if (deferredValidityRV == Result::ERROR_INVALID_DER_TIME) {

Functions that return errors aren't guaranteed, generally, to have put any meaningful value into their output parameters.

I think we should, instead, separate CheckValidity into two functions. One would parse notAfter and notBefore, which returns Success if they are syntactically valid and ERROR_INVALID_DER_TIME otherwise. The second would verify the validity range. This would clean up all the messiness here, AFAICT.

@@ +864,5 @@
> +  Time notAfter(Time::uninitialized);
> +  Result deferredValidityRV = CheckValidity(cert.GetValidity(), time, &notBefore, &notAfter);
> +  if (deferredValidityRV == Result::ERROR_INVALID_DER_TIME) {
> +    return deferredValidityRV;
> +  }

This block should appear ahead of the |switch (publicKeyAlg)| above so that the calculation of |trustLevel| and the checking of |trustLevel| stay close together.

@@ +965,1 @@
>      return rv;

return deferredValidityRV, right?
Attachment #8654892 - Flags: feedback?(brian)
Just as a drive-by, since I see Brian has stated Google's plans:

We agree with Richard's interpretation. New certificates, issued after 2016 January 1, that use SHA-1, are in violation of the BRs. It's reasonable to reject them.
So,(In reply to Ryan Sleevi from comment #37)
> Just as a drive-by, since I see Brian has stated Google's plans:

No I didn't. Read what I wrote.

> We agree with Richard's interpretation. New certificates, issued after 2016
> January 1, that use SHA-1, are in violation of the BRs. It's reasonable to
> reject them.

I agree that certificates with notBefore >= 2016-1-1 should be rejected. But, also, certificates with notAfter >= 2017-1-1 should be rejected.

Will Chrome be accepting certificates with notAfter >= 2017-1-1?

This is what the Chrome SHA-1 deprecation blog post [1] says: "Sites with end-entity certificates that expire on or after 1 January 2017, and which include a SHA-1-based signature as part of the certificate chain, will be treated as “affirmatively insecure”.

[1] http://googleonlinesecurity.blogspot.com/2014/09/gradually-sunsetting-sha-1.html
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #38)
> No I didn't. Read what I wrote.

"And IIRC Google's policy is similar. So, what I'm suggesting is consistent with them." There are degrees of consistency and inconsistency in that statement :)

Elsewhere, you referred to deprecating based on the notBefore as "the new summary seems like a recipe for weak sauce."

In either event,
- No new SHA-1 certificates should be issued following 2016-1-1. This can be quantified by the notBefore.
- No SHA-1 certificates should be valid following 2017-1-1. This can be quantified by the notAfter.

The former can be enacted sooner, and reflects Mozilla's stated public policy on SHA-1 deprecation (that is, internal, non-public PKIs have had ample warning, as have public CAs). The latter may require care - for example, the issuing CA may still be in the process of spinning up a SHA-256 infrastructure in parallel, in advance of the 2017-1-1 deprecation date, which _would_ work for clients that disabled SHA-1.
(In reply to Ryan Sleevi from comment #39)
> "And IIRC Google's policy is similar. So, what I'm suggesting is consistent
> with them." There are degrees of consistency and inconsistency in that
> statement :)

Right.

> Elsewhere, you referred to deprecating based on the notBefore as "the new
> summary seems like a recipe for weak sauce."

Which I still agree with.

> The latter may require care - for
> example, the issuing CA may still be in the process of spinning up a SHA-256
> infrastructure in parallel, in advance of the 2017-1-1 deprecation date,
> which _would_ work for clients that disabled SHA-1.

I agree with that too. Obviously, if rejecting certificates based on notAfter >= 2017-1-1 is going to break too much stuff now, then it is too early to do it. But, IMO it is worth seeing how much stuff would actually break. Without data a priori, I prefer the "see if anything breaks" approach, because that approach is a way of gathering data and also would be good if it doesn't break too much stuff. I don't like the "we don't have data so we can't even try" approach.
Attached patch bug-942515.1.patch (obsolete) — — Splinter Review 
Addressed most of Brian's comments.  Biggest change is probably the spliting ParseValidity out from CheckValidity.  I did not add the notAfter attributes he suggested, since they're not needed for the 2016-01-01 check.

I hope we can agree that if we are going to do the 2017-01-01 check, it can be done as a follow-on bug.  It's easy to add the notAfter attribute at that time.

Brian, I'm assuming from your detailed comments that you're OK with the general approach here.  Would still be interested in feedback from Keeler.
Attachment #8655440 - Flags: feedback?(dkeeler)
Comment on attachment 8655440 [details] [diff] [review]
bug-942515.1.patch

Review of attachment 8655440 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good. I guess let's file some bugs about extending our telemetry so we can see how much breakage there would be by disallowing SHA-1 certs with notAfter >= 2017-01-01.
Also, let's start using reviewboard for reviews in the future.

::: security/apps/AppTrustDomain.cpp
@@ +244,5 @@
>    return Success;
>  }
>  
>  Result
> +AppTrustDomain::CheckSignatureDigestAlgorithm(DigestAlgorithm, EndEntityOrCA, Time)

nit: long line

::: security/certverifier/CertVerifier.cpp
@@ +274,5 @@
>            trustDomain(trustSSL, evOCSPFetching,
>                        mOCSPCache, pinArg, ocspGETConfig,
>                        mCertShortLifetimeInDays, mPinningMode, MIN_RSA_BITS,
>                        ValidityCheckingMode::CheckForEV,
> +                      digestAlgorithmOptions[i], mSHA1Mode,

If mSHA1Mode is SHA1Mode::forbidden, we shouldn't attempt to verify with digestAlgorithmOptions that allow SHA1.

@@ +391,5 @@
>                                         mOCSPCache, pinArg, ocspGETConfig,
>                                         mCertShortLifetimeInDays,
>                                         pinningDisabled, MIN_RSA_BITS_WEAK,
>                                         ValidityCheckingMode::CheckingOff,
> +                                       AcceptAllAlgorithms, mSHA1Mode,

I'm concerned about enforcing the SHA1 restriction for these other verification usages. We have no telemetry data on the compatibility impact (see also that we pass in MIN_RSA_BITS_WEAK and AcceptAllAlgorithms).

::: security/certverifier/CertVerifier.h
@@ +102,5 @@
>      pinningEnforceTestMode = 3
>    };
>  
> +  enum class SHA1Mode {
> +    allowed = 0,

For other enum classes not in this file, we've capitalized the first letter of each option.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +820,3 @@
>  {
> +  // Generated via (new Date("2016-01-01T00:00:00Z")).getTime()
> +  static const Time januaryFirst2016 = TimeFromEpochInSeconds(1451606400000);

nit: JANUARY_FIRST_2016

@@ +832,5 @@
> +      case CertVerifier::SHA1Mode::onlyBefore2016:
> +        if (januaryFirst2016 <= notBefore) {
> +          MOZ_LOG(gCertVerifierLog, LogLevel::Debug, ("Post-2015 SHA-1 certificate rejected"));
> +          return Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED;
> +        }

This should be annotated as a fall-through.

::: security/manager/ssl/nsNSSComponent.cpp
@@ +888,5 @@
>      pinningMode = CertVerifier::pinningDisabled;
>    }
>  
> +  CertVerifier::SHA1Mode sha1Mode = static_cast<CertVerifier::SHA1Mode>
> +      (Preferences::GetInt("security.pki.sha1_enforcement_level",

We need to add a case for this pref at https://dxr.mozilla.org/mozilla-central/rev/fb720c90eb49590ba55bf52a8a4826ffff9f528b/security/manager/ssl/nsNSSComponent.cpp#1373 too.

::: security/pkix/lib/pkixcheck.cpp
@@ +130,2 @@
>                /*optional out*/ Time* notBeforeOut,
> +              /*optional out*/ Time* notAfterOut) {

nit: { on its own line for functions

@@ +854,5 @@
>    }
>  
> +  // IMPORTANT: We parse the validity interval here, so that we can use the
> +  // notBefore and notAfter values in checks for things that might be deprecated
> +  // over time.  However, we must not fail for semantic errors until the end of

nit: I think one space after a period is most common in PSM, although it's not consistent.

@@ +964,5 @@
>  
>    // 4.2.1.14. Inhibit anyPolicy is implicitly supported; see the documentation
>    //           about policy enforcement in pkix.h.
>  
> +  // IMPORTANT: Even though we check validity above (in order to parse the

I would s/check validity/parse the validity period/

::: security/pkix/lib/pkixcheck.h
@@ +44,5 @@
>  Result CheckNameConstraints(Input encodedNameConstraints,
>                              const BackCert& firstChild,
>                              KeyPurposeId requiredEKUIfPresent);
>  
> +Result ParseValidity(Input encodedValidity,

We should probably document the intention here with ParseValidity and CheckValidity.

::: security/pkix/test/gtest/pkixcheck_CheckValidity_tests.cpp
@@ +58,2 @@
>  
>  TEST_F(pkixcheck_CheckValidity, BothEmptyNull)

We should probably rename the testcases that call ParseValidity to pkixcheck_ParseValidity (and maybe rename the file as well?)

::: security/pkix/test/gtest/pkixgtest.h
@@ +179,5 @@
>    {
>      return TestDigestBuf(item, digestAlg, digestBuf, digestBufLen);
>    }
>  
> +  Result CheckSignatureDigestAlgorithm(DigestAlgorithm, EndEntityOrCA, Time) override

nit: long line
Attachment #8655440 - Flags: feedback?(dkeeler) → feedback+
Attached patch bug-942515.1b.patch (obsolete) — — Splinter Review 
WIP with partly-working tests.
Attached patch bug-942515.2.patch (obsolete) — — Splinter Review 
This patch now has working xpcshell tests (as well as the gtests in moz::pkix).

I pondered adding telemetry, but I don't think that's necessary.  Any increased errors will show up in the existing TLS error telemetry under SIGNATURE_ALGORITHM_DISABLED.


(In reply to David Keeler [:keeler] (use needinfo?) from comment #42)
> Comment on attachment 8655440 [details] [diff] [review]
> bug-942515.1.patch
> 
> ::: security/certverifier/CertVerifier.cpp
> @@ +274,5 @@
> >            trustDomain(trustSSL, evOCSPFetching,
> >                        mOCSPCache, pinArg, ocspGETConfig,
> >                        mCertShortLifetimeInDays, mPinningMode, MIN_RSA_BITS,
> >                        ValidityCheckingMode::CheckForEV,
> > +                      digestAlgorithmOptions[i], mSHA1Mode,
> 
> If mSHA1Mode is SHA1Mode::forbidden, we shouldn't attempt to verify with
> digestAlgorithmOptions that allow SHA1.

This is half the story.  I think to get the telemetry interaction right, we also need to have the observation go in to the NeverChecked (=0) bucket.  I've implemented both.


> @@ +391,5 @@
> >                                         mOCSPCache, pinArg, ocspGETConfig,
> >                                         mCertShortLifetimeInDays,
> >                                         pinningDisabled, MIN_RSA_BITS_WEAK,
> >                                         ValidityCheckingMode::CheckingOff,
> > +                                       AcceptAllAlgorithms, mSHA1Mode,
> 
> I'm concerned about enforcing the SHA1 restriction for these other
> verification usages. We have no telemetry data on the compatibility impact
> (see also that we pass in MIN_RSA_BITS_WEAK and AcceptAllAlgorithms).

I've kept it only for the following cases:

* certificateUsageSSLServer
* certificateUsageSSLCA

Let's keep an eye on whether we can expand these cases, though.


> ::: security/pkix/test/gtest/pkixcheck_CheckValidity_tests.cpp
> @@ +58,2 @@
> >  
> >  TEST_F(pkixcheck_CheckValidity, BothEmptyNull)
> 
> We should probably rename the testcases that call ParseValidity to
> pkixcheck_ParseValidity (and maybe rename the file as well?)

Split these into two files.
Attachment #8654892 - Attachment is obsolete: true
Attachment #8655440 - Attachment is obsolete: true
Attachment #8656873 - Attachment is obsolete: true
Attachment #8657279 - Flags: review?(dkeeler)
Comment on attachment 8657279 [details] [diff] [review]
bug-942515.2.patch

Review of attachment 8657279 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but some items need addressing. In particular, we should make sure the pycert/pykey changes are consistent with other work we're doing.

::: security/certverifier/CertVerifier.cpp
@@ +330,5 @@
>              pinningTelemetryInfo->Reset();
>            }
>  
> +          // If we're not going to do SHA-1 in any case, don't try
> +          if ((mSHA1Mode == SHA1Mode::Forbidden)&&

nit: space before &&

@@ +331,5 @@
>            }
>  
> +          // If we're not going to do SHA-1 in any case, don't try
> +          if ((mSHA1Mode == SHA1Mode::Forbidden)&&
> +              (digestAlgorithmOptions[i] != DisableSHA1Everywhere)) {

Also, I don't think you need either set of additional parentheses.

@@ +367,5 @@
>          break;
> +
> +        // If SHA-1 is forbidden by preference, don't accumulate SHA-1
> +        // telemetry, to avoid skewing the results.
> +        if ((sigDigestStatus)&&(mSHA1Mode == SHA1Mode::Forbidden)) {

nit: spaces before/after &&, no need for extraneous parentheses
Also, this code is after a break - how does it even run?

::: security/manager/ssl/nsIX509CertDB.idl
@@ +391,5 @@
>     *  @return 0 if success or the value or the error code for the verification
>     *          failure
> +   *
> +   *  XXX: The aTime parameter should be of type uint64_t, but that type is
> +   *  truncated to 32 bits by the IDL interface.

Is there a bug on this? If not, let's file it and reference it here.

::: security/manager/ssl/nsNSSCertificateDB.cpp
@@ -121,5 @@
>    }
>    return NS_ERROR_FAILURE;
>  }
>  
> -NS_IMETHODIMP 

The whitespace changes are nice, but let's have them in a separate patch so reviewing this file is easier.

::: security/manager/ssl/tests/unit/pycert.py
@@ +291,5 @@
>          self.notBefore = self.now - aYearAndAWhile
>          self.notAfter = self.now + aYearAndAWhile
>          self.subject = 'Default Subject'
>          self.extensions = None
> +        self.decodeParams(paramStream)

Why did this need to move?

@@ +556,5 @@
>              for extension in self.extensions:
>                  extensions.setComponentByPosition(count, extension)
>                  count += 1
>              tbsCertificate.setComponentByName('extensions', extensions)
> +        digest = 'SHA-256'

Fold this into getSignature as in attachment 8642942 [details] [diff] [review] in bug 1183718.

::: security/manager/ssl/tests/unit/pykey.py
@@ +670,5 @@
>          signature by this key over the specified data.
>          Intended for use with pyasn1.type.univ.BitString"""
> +        # This should really only be used with SHA-256
> +        if digest != "SHA-256":
> +            raise "ECC keys should only be used with SHA-256"

We've been defining specific Error classes for each Exception we raise (mostly because I read pep8 and thought it was a good idea at the time). We should at least be consistent.

::: security/manager/ssl/tests/unit/test_cert_sha1.js
@@ +44,5 @@
> +  // Test cases per pref setting
> +  //
> +  // root
> +  //  |
> +  //  +-- pre-2016        <--- (a)

I guess this is an intermediate? (maybe have a column header naming it and the end-entities so that it's more clear)

@@ +57,5 @@
> +  // Expected good outcomes
> +  //                    a b c d e
> +  // allowed=0          X X X X X
> +  // forbidden=1        - - - - -
> +  // onlyBefore2016=2   X X - - -

This isn't clear - is an X a good outcome or is a dash?

::: security/manager/ssl/tests/unit/test_cert_sha1/ee-post_int-post.pem.certspec
@@ +1,2 @@
> +issuer:int-post
> +subject:ee

nit: maybe call this ee-post as well to be consistent?

::: security/manager/ssl/tests/unit/test_cert_sha1/ee-post_int-pre.pem.certspec
@@ +1,2 @@
> +issuer:int-pre
> +subject:ee

nit: ee-post

::: security/manager/ssl/tests/unit/test_cert_sha1/ee-pre_int-pre.pem.certspec
@@ +1,2 @@
> +issuer:int-pre
> +subject:ee

nit: ee-pre

::: security/manager/ssl/tests/unit/test_cert_sha1/int-post.pem.certspec
@@ +1,5 @@
> +issuer:ca
> +subject:int-post
> +validity:20160101-20260101
> +extension:keyUsage:keyCertSign,cRLSign
> +version:3

We don't need to specify the version here.

::: security/manager/ssl/tests/unit/test_cert_sha1/int-pre.pem.certspec
@@ +1,5 @@
> +issuer:ca
> +subject:int-pre
> +validity:20100101-20200101
> +extension:keyUsage:keyCertSign,cRLSign
> +version:3

same

::: security/pkix/lib/pkixcheck.cpp
@@ +91,5 @@
>    // trustDomain.FindIssuers (which may be slow) for such rejected certs, and
>    // more generally it short-circuits any path building with them (which, of
>    // course, is even slower).
>  
> +  rv = trustDomain.CheckSignatureDigestAlgorithm(digestAlg, endEntityOrCA, notBefore);

nit: long line

::: security/pkix/test/gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp
@@ +203,5 @@
>      , checkedModulusSizeInBits(false)
>    {
>    }
>  
> +  Result CheckSignatureDigestAlgorithm(DigestAlgorithm, EndEntityOrCA, Time) override

nit: long line
Attachment #8657279 - Flags: review?(dkeeler) → review-
Comment on attachment 8657279 [details] [diff] [review]
bug-942515.2.patch

Review of attachment 8657279 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/tests/unit/pycert.py
@@ +259,5 @@
>      to a representation usable by the pyasn1 package"""
>      algorithmIdentifier = rfc2459.AlgorithmIdentifier()
>      algorithm = None
> +    if string == 'sha1WithRSAEncryption':
> +        algorithm = univ.ObjectIdentifier('1.2.840.113549.1.1.5 ')

By the way, looks like rfc2459.sha1WithRSAEncryption is defined by the library, so we can just use that directly.
Attached patch bug-942515.3.patch (obsolete) — — Splinter Review 
(In reply to David Keeler [:keeler] (use needinfo?) from comment #45)
> Comment on attachment 8657279 [details] [diff] [review]
> bug-942515.2.patch
> 
> Review of attachment 8657279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +367,5 @@
> >          break;
> > +
> > +        // If SHA-1 is forbidden by preference, don't accumulate SHA-1
> > +        // telemetry, to avoid skewing the results.
> > +        if ((sigDigestStatus)&&(mSHA1Mode == SHA1Mode::Forbidden)) {
> 
> nit: spaces before/after &&, no need for extraneous parentheses
> Also, this code is after a break - how does it even run?

Oops.  Break is now after if.


> ::: security/manager/ssl/nsIX509CertDB.idl
> @@ +391,5 @@
> >     *  @return 0 if success or the value or the error code for the verification
> >     *          failure
> > +   *
> > +   *  XXX: The aTime parameter should be of type uint64_t, but that type is
> > +   *  truncated to 32 bits by the IDL interface.
> 
> Is there a bug on this? If not, let's file it and reference it here.

The comment is just wrong, from a misunderstanding during my debugging.  I deleted it.


> ::: security/manager/ssl/nsNSSCertificateDB.cpp
> @@ -121,5 @@
> >    }
> >    return NS_ERROR_FAILURE;
> >  }
> >  
> > -NS_IMETHODIMP 
> 
> The whitespace changes are nice, but let's have them in a separate patch so
> reviewing this file is easier.

Oh, sorry.  I've got my editor set to automatically strip trailing whitespace.  I've tried to strip these out of the patch.


> @@ +556,5 @@
> >              for extension in self.extensions:
> >                  extensions.setComponentByPosition(count, extension)
> >                  count += 1
> >              tbsCertificate.setComponentByName('extensions', extensions)
> > +        digest = 'SHA-256'
> 
> Fold this into getSignature as in attachment 8642942 [details] [diff] [review]
> [review] in bug 1183718.

I went ahead and folded that into this patch.  Let me know if I missed anything.
Attachment #8657279 - Attachment is obsolete: true
Attachment #8659437 - Flags: review?(dkeeler)
Attached patch bug-942515.3.patch (obsolete) — — Splinter Review 
Missed a comment fix.
Attachment #8659437 - Attachment is obsolete: true
Attachment #8659437 - Flags: review?(dkeeler)
Attachment #8659446 - Flags: review?(dkeeler)
Comment on attachment 8659446 [details] [diff] [review]
bug-942515.3.patch

Review of attachment 8659446 [details] [diff] [review]:
-----------------------------------------------------------------

Ok - looks good to me with comments addressed.

::: security/certverifier/CertVerifier.cpp
@@ +365,5 @@
>  
>        if (rv == Success) {
> +        // If SHA-1 is forbidden by preference, don't accumulate SHA-1
> +        // telemetry, to avoid skewing the results.
> +        if ((sigDigestStatus)&&(mSHA1Mode == SHA1Mode::Forbidden)) {

Please remove the extraneous parentheses.

@@ +376,5 @@
>        if (keySizeStatus) {
>          *keySizeStatus = KeySizeStatus::AlreadyBad;
>        }
>        if (sigDigestStatus) {
>          *sigDigestStatus = SignatureDigestStatus::AlreadyBad;

I imagine we don't want to collect telemetry in this case either if mSHA1Mode == SHA1Mode::Forbidden.

::: security/manager/ssl/nsNSSCertificateDB.cpp
@@ +1783,5 @@
> +                                  uint32_t aFlags,
> +                                  const char* aHostname,
> +                                  nsIX509CertList** aVerifiedChain,
> +                                  bool* aHasEVPolicy,
> +                                  int32_t* /*PRErrorCode*/ _retval )

nit: no space after _retval

@@ +1803,5 @@
> +                                     const char* aHostname,
> +                                     uint64_t aTime,
> +                                     nsIX509CertList** aVerifiedChain,
> +                                     bool* aHasEVPolicy,
> +                                     int32_t* /*PRErrorCode*/ _retval )

nit: same

@@ +1815,5 @@
> +                            mozilla::pkix::TimeFromEpochInSeconds(aTime),
> +                            aVerifiedChain, aHasEVPolicy, _retval, locker);
> +}
> +
> +

nit: only one blank line necessary

::: security/manager/ssl/tests/unit/pycert.py
@@ +20,4 @@
>  [extension:<extension name:<extension-specific data>>]
>  [...]
>  
> +(SHA-256 will be used for signatures by default)

This is already said on line 51.

@@ +263,5 @@
>      algorithm = None
> +    name = None
> +    if string == 'sha1WithRSAEncryption':
> +        name = 'SHA-1'
> +        algorithm = univ.ObjectIdentifier('1.2.840.113549.1.1.5 ')

nit: use rfc2459.sha1WithRSAEncryption

@@ +270,3 @@
>          algorithm = univ.ObjectIdentifier('1.2.840.113549.1.1.11')
>      elif string == 'ecdsaWithSHA256':
> +        name = 'SHA-256'

Either we need to add a layer of indirection or this needs to be 'sha256' since that's what PyECC is expecting.

@@ +565,5 @@
>                  count += 1
>              tbsCertificate.setComponentByName('extensions', extensions)
> +        digest = 'SHA-256'
> +        if self.signature == 'sha1WithRSAEncryption':
> +            digest = 'SHA-1'

These three lines appear to be unnecessary.

::: security/manager/ssl/tests/unit/pykey.py
@@ +70,5 @@
> +    """Exception type indicating that the key was misconfigured"""
> +
> +    def __init__(self, value):
> +        UnknownBaseError.__init__(self, value)
> +        self.category = 'key parameters'

s/parameters/parameter/

@@ +670,5 @@
>          subjectPublicKey = univ.BitString(hexifiedBitString)
>          spki.setComponentByName('subjectPublicKey', subjectPublicKey)
>          return spki
>  
> +    def sign(self, data, digest):

digest never actually gets passed to the calls to self.key.sign

@@ +676,5 @@
>          signature by this key over the specified data.
>          Intended for use with pyasn1.type.univ.BitString"""
> +        # This should really only be used with SHA-256
> +        if digest != "SHA-256":
> +            raise ParameterError("ECC keys can only be used with SHA-256")

nit: use single quotes for simple strings
Also, pass digest to the ParameterError constructor (it will result in an error message like 'Unknown key parameter type "whatever digest was"').
Also, this is where we would either translate 'SHA-256' to 'sha256' or just use 'sha256' from the beginning.
Attachment #8659446 - Flags: review?(dkeeler) → review+
(In reply to David Keeler [:keeler] (use needinfo?) from comment #49)
> Comment on attachment 8659446 [details] [diff] [review]
> bug-942515.3.patch
> 
> Review of attachment 8659446 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +270,3 @@
> >          algorithm = univ.ObjectIdentifier('1.2.840.113549.1.1.11')
> >      elif string == 'ecdsaWithSHA256':
> > +        name = 'SHA-256'
> 
> Either we need to add a layer of indirection or this needs to be 'sha256'
> since that's what PyECC is expecting.

It actually doesn't matter, because pykey.py just uses this as a flag for whether ECC is allowed.  The digest is still wired in.  I've added a comment.


> @@ +670,5 @@
> >          subjectPublicKey = univ.BitString(hexifiedBitString)
> >          spki.setComponentByName('subjectPublicKey', subjectPublicKey)
> >          return spki
> >  
> > +    def sign(self, data, digest):
> 
> digest never actually gets passed to the calls to self.key.sign

That's OK.  It's just there to expose a consistent interface to pycert.py, for both RSA and ECC.

Try run in progress:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0349711f38c0
Attachment #8659446 - Attachment is obsolete: true
Attachment #8659894 - Flags: review+
Keywords: checkin-needed
Do we need two follow-on bugs for 'notAfter >= 2017-1-1' and stop supporting SHA-1 in 2017?
(In reply to sjw from comment #52)
> Do we need two follow-on bugs for 'notAfter >= 2017-1-1' and stop supporting
> SHA-1 in 2017?

That seems like a good discussion topic for dev.tech.crypto, but I don't think there's enough consensus to open bugs at the moment.
https://hg.mozilla.org/mozilla-central/rev/0516d4db29a9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Whiteboard: [adv-main43-]
hg.mozilla uses sha1 how not-cool is this? :(
https://www.ssllabs.com/ssltest/analyze.html?d=hg.mozilla.org

my 43.0.4 cant access it any more
Since this was backed out in bug 1236975, so I'm reopening it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [adv-main43-] → [adv-main43-][psm-assigned]
I wonder why SHA-1 has been re-enabled for all versions and platforms in bug 1236975.
Referring to the issues there, mainly Windows users were affected. I think it would be safe to re-enable this for Linux users and maybe also for all Nightly users.
Note the affected MitM software vendors might also have updated their cert during the last six months.

As far as I know Firefox is now the only major browser, that still has no visual indicator for SHA-1 usage, even if it expires in 2017 or was issued in 2016.
So is it relay worth to be compatible to some strange MitM software, that are still using SHA-1?
Bug 1254667 changed Firefox to reject sha-1 certificates issued by public certificate authorities after January 1 2016. This policy is intended to balance enforcing strong security in situations we have some control over (how certificate authorities in our root program issue certificates) with compatibility in situations we have less control over (when users either choose to or are forced to use intercepting proxies with certificates that are outside of the public certificate system). Changing this policy would involve a larger discussion that should happen on a mailing list, not in a bug.

In any case, a bug you might be interested in is bug 1183718, which we started but never finished (there were some questions about the UI treatment). See also bug 1068949, which added logging to the web console for sites that use sha-1 certificates.

As a final note, it doesn't quite fit, but I think WONTFIX is the most appropriate status for this bug at this time.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 8 years ago
Resolution: --- → WONTFIX
See Also: → 1254667
Firefox 50 seems to default "security.pki.sha1_enforcement_level" to value 3, although only values 0, 1 and 2 seem to be described in this bug.  I have not been able to find the meaning of value 3 anywhere in the internet.  What does value 3 mean?  Many thanks in advance!
(In reply to Bob Hill from comment #60)
> Firefox 50 seems to default "security.pki.sha1_enforcement_level" to value
> 3, although only values 0, 1 and 2 seem to be described in this bug.  I have
> not been able to find the meaning of value 3 anywhere in the internet.  What
> does value 3 mean?  Many thanks in advance!

https://dxr.mozilla.org/mozilla-central/search?q=ImportedRoot
Many thanks to Yang for the rapid response.  Researching beyond Yang's link revealed that valid values for "security.pki.sha1_enforcement_level" now seem to be 0, 1, 3 and 4 (in case anyone is interested):
https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_cert_sha1.js
(In reply to Bob Hill from comment #62)
> Many thanks to Yang for the rapid response.  Researching beyond Yang's link
> revealed that valid values for "security.pki.sha1_enforcement_level" now
> seem to be 0, 1, 3 and 4 (in case anyone is interested):
> https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/
> unit/test_cert_sha1.js

https://dxr.mozilla.org/mozilla-central/source/security/certverifier/CertVerifier.h#145
  enum class SHA1Mode {
    Allowed = 0,
    Forbidden = 1,
    UsedToBeBefore2016ButNowIsForbidden = 2,
    ImportedRoot = 3,
    ImportedRootOrBefore2016 = 4,
  };
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size:

We need Your Support. Make a Donation now! or