Closed Bug 1183718 Opened 9 years ago Closed 7 years ago

Visually degrade connections with SHA-1-signed certificates in the chain

Categories

(Core :: Security: PSM, defect)

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

Tracking

()

Status:
RESOLVED WONTFIX
Tracking Flags:
Tracking Status
firefox42 --- affected

People

(Reporter: keeler, Unassigned)

References

(
URL
)

Details

(Whiteboard: [psm-backlog])

Attachments

(4 files, 13 obsolete files)

0002-Bug-1183718-Fix-test_pinning.js-to-expect-a-few-extr.patch
2.43 KB, patch
keeler
: review+
Details | Diff | Splinter Review
0003-Bug-1183718-Make-pycert.py-support-SHA-1-signatures-.patch, v3
8.70 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
0001-Bug-1183718-Set-STATE_IS_BROKEN-if-the-cert-chain-co.patch, v6
15.55 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
0004-Bug-1183718-Add-SHA-1-deprecation-tests-r-keeler.patch, v5
36.35 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review 
The backend needs to provide a way for the frontend to know when a server sent a certificate chain that could only be verified using weak signature algorithms (i.e. sha-1).
We should probably copy the approach taken by bug 1153010 and add a WEAK_CERTIFICATE flag, or pick a better name. Would we copy Chrome's approach and only set it for certificates expiring after Dec 2015?
Bug 1093595 is where the WEAK_CIPHER flag was introduced.
Oh, I got it all wrong. The patch reuses STATE_IS_BROKEN and the UI simply checks all possible cases until it then decides a weak cipher is the cause for brokenness.
This copies Chrome's approach and introduces two new SHA-1 flags for a "broken" request:


STATE_SHA1_EXPIRES_IN_2016 for when the cert chain (excluding the root) contains one or more certs signed with SHA-1 and the leaf cert expires in 2016.

https://sha1-2016.badssl.com/


STATE_SHA1_EXPIRES_AFTER_2016 for when the cert chain (excluding the root) contains one or more certs signed with SHA-1 and the leaf cert expires in 2017+.

https://sha1-2017.badssl.com/


(Feel free to suggest better flag names btw :)

nsSecureBrowserUIImpl::MapInternalToExternalState() unfortunately swallows STATE_USES_SSL_3, STATE_USES_WEAK_CRYPTO, and the two new flags so we might want to fix that too. What works is that the patch sets STATE_IS_BROKEN and we thus have:

http://i.imgur.com/31swCUZ.png

If MapInternalToExternalState() would forward the SHA1 flags then we could have that for 2017+ certs:

http://i.imgur.com/IafIbJQ.png
Attachment #8634018 - Flags: feedback?(dkeeler)
CC'ing Kathleen as she worked on the post about phasing out SHA-1-signed certs.
URL: https://blog.mozilla.org/security/201...
Comment on attachment 8634018 [details] [diff] [review]
0001-Bug-1183718-Set-STATE_IS_BROKEN-if-the-cert-chain-co.patch

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

This looks good. Just a couple of comments.

::: security/manager/ssl/nsNSSCallbacks.cpp
@@ +1108,5 @@
>    MOZ_ASSERT(value != 0);
>    Telemetry::Accumulate(probe, value);
>  }
>  
> +uint32_t GetSha1SignatureWarningFlagIfAny(nsSSLStatus* status) {

nit: as I understand it, this is the preferred format for function definitions:

uint32_t
GetSha1SignatureWarningFlagIfAny(nsSSLStatus* status)
{

@@ +1139,5 @@
> +                    nsIWebProgressListener::STATE_SHA1_EXPIRES_IN_2016;
> +
> +  while (true) {
> +    nsCOMPtr<nsIX509Cert> issuer;
> +    rv = cert->GetIssuer(getter_AddRefs(issuer));

Repeatedly calling cert->GetIssuer will repeatedly attempt to build/verify a certificate chain, which is relatively slow. If it fails, it can return a certificate that hasn't actually been verified as an issuer for cert. Also it can cause the thread to block on network i/o, which can be bad if this is the main thread. So this is something we want to avoid. I would recommend calling CertVerifier::VerifySSLServerCert with local only flags to get a verified chain once, and then looping over that. This brings up an important point, though: there are many places where we do this sort of thing (i.e. repeatedly re-determining the verified certificate chain). It would be much better to build a chain once (which we already do in AuthCertificate) and keep it for as long as that connection is valid. There are a few corner cases that make this difficult (e.g. caching, session resumption), but it would be worth it in the long run.

@@ +1312,5 @@
>    bool renegotiationUnsafe = !siteSupportsSafeRenego &&
>                               ioLayerHelpers.treatUnsafeNegotiationAsBroken();
>  
> +  uint32_t state = GetSha1SignatureWarningFlagIfAny(status);
> +  if (state || usesWeakCipher || renegotiationUnsafe) {

nit: I would prefer we not implicitly treat state like a boolean here: 'state != 0'
Attachment #8634018 - Flags: feedback?(dkeeler) → feedback+
Will work on tests in a second patch.
Assignee: nobody → ttaubert
Attachment #8634018 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8634640 - Flags: review?(dkeeler)
Summary: add api to expose the presence of weak signature algorithms (i.e. sha-1) in TLS handshake certificates → Visually degrade connections with SHA-1-signed certificates in the chain
(In reply to Tim Taubert [:ttaubert] from comment #4)
> This copies Chrome's approach and introduces two new SHA-1 flags for a
> "broken" request:
> 
> 
> STATE_SHA1_EXPIRES_IN_2016 for when the cert chain (excluding the root)
> contains one or more certs signed with SHA-1 and the leaf cert expires in
> 2016.
> 
> https://sha1-2016.badssl.com/
> http://i.imgur.com/31swCUZ.png
> 
> 
> STATE_SHA1_EXPIRES_AFTER_2016 for when the cert chain (excluding the root)
> contains one or more certs signed with SHA-1 and the leaf cert expires in
> 2017+.
> 
> https://sha1-2017.badssl.com/
> http://i.imgur.com/IafIbJQ.png

Aislinn, Tanvi, any opinion on that? Are you both fine with reusing the MCB icons to visually degrade those connections?
Flags: needinfo?(tanvi)
Flags: needinfo?(agrigas)
(In reply to Tim Taubert [:ttaubert] from comment #4)
> nsSecureBrowserUIImpl::MapInternalToExternalState() unfortunately swallows
> STATE_USES_SSL_3, STATE_USES_WEAK_CRYPTO, and the two new flags so we might
> want to fix that too.

Tanvi, what would be the best way to fix that? I can also file a follow-up and we could land this patch here first with both states using the same icon (grey lock, yellow yield sign). And then when we properly forward those flags we can use the bolder warning for certs valid until 2017+.
(In reply to Tim Taubert [:ttaubert] from comment #8)
> (In reply to Tim Taubert [:ttaubert] from comment #4)
> > This copies Chrome's approach and introduces two new SHA-1 flags for a
> > "broken" request:
> > 
> > 
> > STATE_SHA1_EXPIRES_IN_2016 for when the cert chain (excluding the root)
> > contains one or more certs signed with SHA-1 and the leaf cert expires in
> > 2016.
> > 
> > https://sha1-2016.badssl.com/
> > http://i.imgur.com/31swCUZ.png
> > 
> > 
> > STATE_SHA1_EXPIRES_AFTER_2016 for when the cert chain (excluding the root)
> > contains one or more certs signed with SHA-1 and the leaf cert expires in
> > 2017+.
> > 
> > https://sha1-2017.badssl.com/
> > http://i.imgur.com/IafIbJQ.png
> 
> Aislinn, Tanvi, any opinion on that? Are you both fine with reusing the MCB
> icons to visually degrade those connections?

I'll let Tanvi weigh in though she's out this week. We will need new copy correct for each state? Will most likely need Tanvi's input there as not sure I know enough about this use case to describe it....
Flags: needinfo?(agrigas)
(In reply to agrigas from comment #10)
> I'll let Tanvi weigh in though she's out this week. We will need new copy
> correct for each state? Will most likely need Tanvi's input there as not
> sure I know enough about this use case to describe it....

We can wait, there's no rush in landing this. And yeah, we'd ideally show a helpful message in the subpanel.
Comment on attachment 8634640 [details] [diff] [review]
0001-Bug-1183718-Set-STATE_IS_BROKEN-if-the-cert-chain-co.patch, v2

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

Great - r=me with comments addressed.

::: security/manager/ssl/nsNSSCallbacks.cpp
@@ +1111,5 @@
>  
> +uint32_t
> +GetSha1SignatureWarningFlagIfAny(nsSSLStatus* status,
> +                           const char* hostname,
> +                           const nsNSSShutDownPreventionLock& /* proofOfLock */)

nit: alignment on these (if the second line ends up being too long, I guess they can both be indented to just two spaces from the beginning of the line)

@@ +1114,5 @@
> +                           const char* hostname,
> +                           const nsNSSShutDownPreventionLock& /* proofOfLock */)
> +{
> +  nsCOMPtr<nsIX509Cert> cert;
> +  nsresult rv = status->GetServerCert(getter_AddRefs(cert));

We should probably add a debug assertion that the status has a certificate (since the only callsite at the moment guarantees that it does).

@@ +1157,5 @@
> +
> +  CERTCertListNode* node = CERT_LIST_HEAD(certList);
> +
> +  // Check only intermediates and the leaf cert.
> +  while (node && !CERT_LIST_END(node, certList)) {

I think this will include the root cert, right? Wouldn't we want '!CERT_LIST_END(CERT_LIST_NEXT(node), certList)'?
Attachment #8634640 - Flags: review?(dkeeler) → review+
(In reply to David Keeler [:keeler] (use needinfo?) from comment #12)
> > +uint32_t
> > +GetSha1SignatureWarningFlagIfAny(nsSSLStatus* status,
> > +                           const char* hostname,
> > +                           const nsNSSShutDownPreventionLock& /* proofOfLock */)
> 
> nit: alignment on these (if the second line ends up being too long, I guess
> they can both be indented to just two spaces from the beginning of the line)

Changed it to:

uint32_t
GetSha1SignatureWarningFlagIfAny(nsSSLStatus* status,
                                 const char* hostname,
                                 const nsNSSShutDownPreventionLock& /* proofOfLock */)
{

That's what we do in the rest of the file so I thought it would be best to stick with that?

> > +{
> > +  nsCOMPtr<nsIX509Cert> cert;
> > +  nsresult rv = status->GetServerCert(getter_AddRefs(cert));
> 
> We should probably add a debug assertion that the status has a certificate
> (since the only callsite at the moment guarantees that it does).

Added.

> > +  // Check only intermediates and the leaf cert.
> > +  while (node && !CERT_LIST_END(node, certList)) {
> 
> I think this will include the root cert, right? Wouldn't we want
> '!CERT_LIST_END(CERT_LIST_NEXT(node), certList)'?

Yeah, you're right. I changed it to:

for (CERTCertListNode* node = CERT_LIST_HEAD(certList);
     node != CERT_LIST_TAIL(certList);
     node = CERT_LIST_NEXT(node)) {
This patch fixes security/manager/ssl/tests/unit/test_pinning.js failures. The test checks the number of histogram values recorded for key pinning. If the check is performed twice than we'll see twice the number of measurements.

Added CertVerifier::FLAG_PINNING_DISABLED to be passed to CertVerifier::VerifyCert() so that we can skip pinning checks when getting a cert chain to check for SHA-1 signatures.
Attachment #8635880 - Flags: review?(dkeeler)
Comment on attachment 8635880 [details] [diff] [review]
0002-Bug-1183718-Allow-to-disable-Public-Key-Pinning-chec.patch

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

This is a known problem with the way pinning telemetry is implemented. We're fixing it in bug 1153444. In the meantime, I think it would be best to actually change the expected telemetry values in the test rather than add this workaround.
Attachment #8635880 - Flags: review?(dkeeler) → review-
Comment on attachment 8635831 [details] [diff] [review]
0001-Bug-1183718-Set-STATE_IS_BROKEN-if-the-cert-chain-co.patch, v3

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

::: security/manager/ssl/nsNSSCallbacks.cpp
@@ +1291,5 @@
>            cipherInfo.symCipher);
>      }
>    }
>  
> +  /* Set the SSL Status information */

Oh, by the way - might be nice to convert this to a // comment while we're here.
r=keeler

(In reply to David Keeler [:keeler] (use needinfo?) from comment #17)
> > +  /* Set the SSL Status information */
> 
> Oh, by the way - might be nice to convert this to a // comment while we're
> here.

Done.
Attachment #8635831 - Attachment is obsolete: true
Attachment #8636435 - Flags: review+
(In reply to David Keeler [:keeler] (use needinfo?) from comment #16)
> This is a known problem with the way pinning telemetry is implemented. We're
> fixing it in bug 1153444. In the meantime, I think it would be best to
> actually change the expected telemetry values in the test rather than add
> this workaround.

Done.
Attachment #8635880 - Attachment is obsolete: true
Attachment #8636444 - Flags: review?(dkeeler)
Comment on attachment 8636444 [details] [diff] [review]
0002-Bug-1183718-Fix-test_pinning.js-to-expect-a-few-extr.patch

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

This is consistent with the behavior I'm seeing on my machine.
Attachment #8636444 - Flags: review?(dkeeler) → review+
(In reply to agrigas from comment #10)
> (In reply to Tim Taubert [:ttaubert] from comment #8)
> > Aislinn, Tanvi, any opinion on that? Are you both fine with reusing the MCB
> > icons to visually degrade those connections?
> 
> I'll let Tanvi weigh in though she's out this week. We will need new copy
> correct for each state? Will most likely need Tanvi's input there as not
> sure I know enough about this use case to describe it....


Its fine to reuse the MCB icons for degrading these SHA-1 connections too.  We are already doing it for rc4.  But Aislinn is right, we do need to get the right copy in.

And we also should check with Richard or Kathleen on when we should start degrading the UI for SHA-1?  Now or after some months when SHA-1 usage has gone down a little.
Flags: needinfo?(tanvi)
Unless/until bug 657228 is implemented, the telemetry for SHA-1 usage is likely to overstate how much SHA-1 is being used.
(In reply to Tim Taubert [:ttaubert] from comment #9)
> (In reply to Tim Taubert [:ttaubert] from comment #4)
> > nsSecureBrowserUIImpl::MapInternalToExternalState() unfortunately swallows
> > STATE_USES_SSL_3, STATE_USES_WEAK_CRYPTO, and the two new flags so we might
> > want to fix that too.
> 
> Tanvi, what would be the best way to fix that? I can also file a follow-up
> and we could land this patch here first with both states using the same icon
> (grey lock, yellow yield sign). And then when we properly forward those
> flags we can use the bolder warning for certs valid until 2017+.

For now you should just use the same icon for sha-1 certs regardless of expiry (grey lock, yellow yield sign).

To fix this in the future, you can add more lockIconStates to nsSecureBrowserUIImpl - lis_weak_security and lis_very_weak_security or something like that and setting them when the weak crypto and ssl3 flags are set.  Then in MapExternalToExternalState you would check for these cases and set STATE_IS_BROKEN | STATE_SHA1_EXPIRES_IN_2016 or STATE_IS_BROKEN | STATE_SHA1_EXPIRES_AFTER_2016.

In bug https://bugzilla.mozilla.org/show_bug.cgi?id=1153010, we came across the same problem and decided to avoid it instead of adding complexity to nsSecureBrowserUIImpl.  So we just assume that if STATE_IS_BROKEN and no mixed content flags are set, we have weak crypto.  I would talk to keeler or mgoodwin to see what they think about adding more lockIconStates.
Adding tests to ensure this works as intended.
Attachment #8639235 - Flags: review?(dkeeler)
This might make things a little more complicated than I want but I had to patch nss/cmd/certutil/certutil.c to allow passing an exact timestamp for 'notAfter' so that we'd have proper certificates to test SHA-1 deprecation.

We could in theory land the stuff here and deal with the NSS patch later but that would mean calling ./generate_certs.sh wouldn't work until that lands in NSS, right?
Attachment #8639238 - Flags: feedback?(dkeeler)
(In reply to Tanvi Vyas out until 7-24 [:tanvi] from comment #21)
> And we also should check with Richard or Kathleen on when we should start
> degrading the UI for SHA-1?  Now or after some months when SHA-1 usage has
> gone down a little.

Chrome (release version) already degrades SHA-1 connections in two stages. We can follow their lead here and do the same thing.
Flags: needinfo?(rlb)
(In reply to Tanvi Vyas out until 7-24 [:tanvi] from comment #23)
> For now you should just use the same icon for sha-1 certs regardless of
> expiry (grey lock, yellow yield sign).

Alright, that might be fine for a start. Guess we would need to touch nsSecureBrowserUIImpl though if we want to show a different copy for SHA-1 than we show for RC4/SSL3.
(In reply to Tanvi Vyas [:tanvi] from comment #21)
> And we also should check with Richard or Kathleen on when we should start
> degrading the UI for SHA-1?  Now or after some months when SHA-1 usage has
> gone down a little.

First we need better telemetry in this area, which is being worked on.
Then we will use telemetry to determine when and how to degrade the UI regarding SHA-1.
(In reply to Kathleen Wilson from comment #28)
> (In reply to Tanvi Vyas [:tanvi] from comment #21)
> > And we also should check with Richard or Kathleen on when we should start
> > degrading the UI for SHA-1?  Now or after some months when SHA-1 usage has
> > gone down a little.
> 
> First we need better telemetry in this area, which is being worked on.
> Then we will use telemetry to determine when and how to degrade the UI
> regarding SHA-1.

But, of course, the backend can be updated now to enable this.
(In reply to Tim Taubert [:ttaubert] from comment #27)
> (In reply to Tanvi Vyas out until 7-24 [:tanvi] from comment #23)
> > For now you should just use the same icon for sha-1 certs regardless of
> > expiry (grey lock, yellow yield sign).
> 
> Alright, that might be fine for a start. Guess we would need to touch
> nsSecureBrowserUIImpl though if we want to show a different copy for SHA-1
> than we show for RC4/SSL3.

Yes.  You will have to use generic copy in the control center.  But I think that's okay.  The user isn't going to know the difference between RC4/SSL3/SHA-1.  Weak crypto is all we have to tell them.  Then the webconsole can tell the developer the details.
Comment on attachment 8639238 [details] [diff] [review]
0004-Bug-1183718-Make-certutil-accept-a-timestamp-for-not.patch

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

In principle this seems like a reasonable approach. We'll probably have to file a separate NSS bug for this change and wait for it to get landed and imported into mozilla-central, though. That said, we're in the process of converting the PSM xpcshell tests to not rely directly on NSS at all (see bug 1174288), so generate_certs.sh will soon be going away.
Attachment #8639238 - Flags: feedback?(dkeeler) → feedback+
Comment on attachment 8639235 [details] [diff] [review]
0003-Bug-1183718-Add-SHA-1-deprecation-tests.patch

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

This looks good. I noted some nits. Ultimately, though, we need to determine if it makes sense to go with the certutil change or wait until the other generate_certs.sh-based tests have been converted to the new system (see previous comment). I'll clear the review in the meantime.

::: security/manager/ssl/tests/unit/test_sha1_deprecation.js
@@ +41,5 @@
> +    let {securityState} =
> +      aTransportSecurityInfo.QueryInterface(Ci.nsITransportSecurityInfo);
> +
> +    ok(securityState & Ci.nsIWebProgressListener.STATE_IS_BROKEN,
> +      "security state should be 'broken'");

nit: indent one more space

@@ +43,5 @@
> +
> +    ok(securityState & Ci.nsIWebProgressListener.STATE_IS_BROKEN,
> +      "security state should be 'broken'");
> +    ok(securityState & Ci.nsIWebProgressListener.STATE_SHA1_EXPIRES_IN_2016,
> +      "security state should indicate a SHA-1 signature and " +

nit: same

@@ +62,5 @@
> +    let {securityState} =
> +      aTransportSecurityInfo.QueryInterface(Ci.nsITransportSecurityInfo);
> +
> +    ok(securityState & Ci.nsIWebProgressListener.STATE_IS_BROKEN,
> +      "security state should be 'broken'");

nit: same

@@ +64,5 @@
> +
> +    ok(securityState & Ci.nsIWebProgressListener.STATE_IS_BROKEN,
> +      "security state should be 'broken'");
> +    ok(securityState & Ci.nsIWebProgressListener.STATE_SHA1_EXPIRES_AFTER_2016,
> +      "security state should indicate a SHA-1 signature and " +

nit: same

::: security/manager/ssl/tests/unit/xpcshell.ini
@@ +139,5 @@
>  [test_validity.js]
>  run-sequentially = hardcoded ports
>  [test_certviewer_invalid_oids.js]
>  skip-if = toolkit == 'android' || buildapp == 'b2g'
> +[test_sha1_deprecation.js]

This needs a 'run-sequentially = hardcoded ports', if I recall correctly.
Attachment #8639235 - Flags: review?(dkeeler)
Comment on attachment 8639238 [details] [diff] [review]
0004-Bug-1183718-Make-certutil-accept-a-timestamp-for-not.patch

Have something almost working with py{cert,key}.py.
Attachment #8639238 - Attachment is obsolete: true
Attachment #8639235 - Attachment is obsolete: true
Flags: needinfo?(rlb)
This adds SHA-1 signature support to pycert.py via "signature:sha1WithRSAEncryption", defaulting to SHA-256. I removed the "signatureAlgorithm" field because it looked it was a duplicate and we only need one?
Attachment #8642312 - Flags: review?(dkeeler)
This patch does a few things:

1) It makes _setupTLSServerTest() a tad more flexible and allows passing options to make it more useful with slightly different TLS server implementations.

2) Adds test_sha1_deprecation.js and assets, pycert.py and pykey will generate the certs and the key.

3) The Sha1CertServer key and cert import code is mostly copied from GenerateOCSPResponse, changed the way cert nicknames are determined.

4) Had to change TLSServer.cpp to let it find the right key for the imported certs. This seems to work just fine with all other existing tests.
Attachment #8642334 - Flags: review?(dkeeler)
(In reply to Kathleen Wilson from comment #29)
> (In reply to Kathleen Wilson from comment #28)
> > (In reply to Tanvi Vyas [:tanvi] from comment #21)
> > > And we also should check with Richard or Kathleen on when we should start
> > > degrading the UI for SHA-1?  Now or after some months when SHA-1 usage has
> > > gone down a little.
> > 
> > First we need better telemetry in this area, which is being worked on.
> > Then we will use telemetry to determine when and how to degrade the UI
> > regarding SHA-1.
> 
> But, of course, the backend can be updated now to enable this.

If we land the patches here then we would start showing a "weak encryption" warning for SHA-1 certs (expiring after 2015) right away. This means we would show a grey lock and a yellow yield sign.

Kathleen, is this something we'd want to do or do we even for the gentle warning want more telemetry data? Chrome currently shows a similar warning for certificates expiring in 2016. They show a much harsher warning for certs expiring in 2017+.

What do you think about showing a gentle warning and waiting for more data on when and how to show a more serious warning for certs expiring after 2016? I suppose that if we show no warning at all there's not a lot of incentive for site owners to update their chains...
Comment on attachment 8642312 [details] [diff] [review]
0003-Bug-1183718-Make-pycert.py-support-SHA-1-signatures.patch

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

Forgive the drive-by review, but this patch breaks tests on my local machine.

::: security/manager/ssl/tests/unit/pycert.py
@@ -215,5 @@
>          hasher.update(self.issuer)
>          hasher.update(str(self.notBefore))
>          hasher.update(str(self.notAfter))
>          hasher.update(self.subject)
> -        hasher.update(self.signatureAlgorithm)

This breaks (test_ev_certs.js, test_keysize_ev.js, test_validity.js) by changing the serial numbers (and thus the fingerprints) of the EV enabled test certs.

One possible solution is changing the line to |hasher.update(self.signature)| instead.

::: security/manager/ssl/tests/unit/test_keysize_ev.js
@@ +135,5 @@
>    let smallKeyEVRoot =
>      constructCertFromFile("test_keysize_ev/ev_root_rsa_2040.pem");
>    equal(smallKeyEVRoot.sha256Fingerprint,
> +        "49:46:10:F4:F5:B1:96:E7:FB:FA:4D:A6:34:03:D0:99:" +
> +        "22:D4:77:20:3F:84:E0:DF:1C:AD:B4:C2:76:BB:63:24",

This should not be necessary if the serial number hashing change in pycert.py is removed.
(In reply to Tanvi Vyas [:tanvi] from comment #30)
> (In reply to Tim Taubert [:ttaubert] from comment #27)
> > (In reply to Tanvi Vyas out until 7-24 [:tanvi] from comment #23)
> > > For now you should just use the same icon for sha-1 certs regardless of
> > > expiry (grey lock, yellow yield sign).
> > 
> > Alright, that might be fine for a start. Guess we would need to touch
> > nsSecureBrowserUIImpl though if we want to show a different copy for SHA-1
> > than we show for RC4/SSL3.
> 
> Yes.  You will have to use generic copy in the control center.  But I think
> that's okay.  The user isn't going to know the difference between
> RC4/SSL3/SHA-1.  Weak crypto is all we have to tell them.  Then the
> webconsole can tell the developer the details.

Cool, sgtm. Thanks!
(In reply to Cykesiopka from comment #37)
> Forgive the drive-by review, but this patch breaks tests on my local machine.

No worries, all feedback welcome :)

> ::: security/manager/ssl/tests/unit/pycert.py
> @@ -215,5 @@
> >          hasher.update(self.issuer)
> >          hasher.update(str(self.notBefore))
> >          hasher.update(str(self.notAfter))
> >          hasher.update(self.subject)
> > -        hasher.update(self.signatureAlgorithm)
> 
> This breaks (test_ev_certs.js, test_keysize_ev.js, test_validity.js) by
> changing the serial numbers (and thus the fingerprints) of the EV enabled
> test certs.
> 
> One possible solution is changing the line to
> |hasher.update(self.signature)| instead.

Yeah, that was my first solution. Wonder why I only see a single breakage here though.

> ::: security/manager/ssl/tests/unit/test_keysize_ev.js
> @@ +135,5 @@
> >    let smallKeyEVRoot =
> >      constructCertFromFile("test_keysize_ev/ev_root_rsa_2040.pem");
> >    equal(smallKeyEVRoot.sha256Fingerprint,
> > +        "49:46:10:F4:F5:B1:96:E7:FB:FA:4D:A6:34:03:D0:99:" +
> > +        "22:D4:77:20:3F:84:E0:DF:1C:AD:B4:C2:76:BB:63:24",
> 
> This should not be necessary if the serial number hashing change in
> pycert.py is removed.

Right.
(In reply to Tim Taubert [:ttaubert] from comment #39)
> Yeah, that was my first solution. Wonder why I only see a single breakage
> here though.

Hmm, do you build opt? The test EV roots only have EV status on debug builds, maybe that's why.
Changed the patch to what Cykesiopka suggest, simply include the sig alg twice when generating a serial number.
Attachment #8642312 - Attachment is obsolete: true
Attachment #8642312 - Flags: review?(dkeeler)
Attachment #8642337 - Flags: review?(dkeeler)
(In reply to Cykesiopka from comment #40)
> Hmm, do you build opt? The test EV roots only have EV status on debug
> builds, maybe that's why.

Yeah, my bad. Check with a debug build and everything is passing now.
Comment on attachment 8642337 [details] [diff] [review]
0003-Bug-1183718-Make-pycert.py-support-SHA-1-signatures.patch, v2

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

LGTM with comments addressed.

::: security/manager/ssl/tests/unit/pycert.py
@@ +15,5 @@
>  [version:<{1,2,3,4}>]
>  [validity:<YYYYMMDD-YYYYMMDD|duration in days>]
>  [issuerKey:alternate]
>  [subjectKey:alternate]
> +[signature:{sha256WithRSAEncryption,sha1WithRSAEncryption}]

Slight consistency nit: version is the same type of field (i.e., specify one value from a set), so we should probably document them the same way. I guess including the '<...>' is a bit unnecessary, so maybe just remove them from version? (i.e. it should look like '[version:{1,2,3,4}]')

Also we should note that sha256WithRSAEncryption is the default.

@@ -196,5 @@
>          aYearAndAWhile = datetime.timedelta(days=550)
>          self.notBefore = self.now - aYearAndAWhile
>          self.notAfter = self.now + aYearAndAWhile
>          self.subject = 'Default Subject'
> -        self.signatureAlgorithm = 'sha256WithRSAEncryption'

The presence of both a 'signature' field and a 'signatureAlgorithm' field is because RFC 5280 defines both (one for the TBSCertificate and one in the signature on the TBSCertificate). The spec says they must match, so I initially included both in case we wanted to test specifying different ones. We have tests for this in mozilla::pkix, though, so it's probably not essential that we support this right now.

@@ +439,4 @@
>          tbsDER = encoder.encode(tbsCertificate)
> +
> +        hashAlg = None
> +        if self.signature == 'sha1WithRSAEncryption':

nit: I would break this out into a helper function (maybe have stringToAlgorithmIdentifier return a tuple (pyasn1 OID, string identifier))?

@@ +444,5 @@
> +        if self.signature == 'sha256WithRSAEncryption':
> +            hashAlg = 'SHA-256'
> +        if hashAlg == None:
> +            raise UnknownAlgorithmTypeError(self.signature)
> +        certificate.setComponentByName('signatureValue', self.issuerKey.sign(tbsDER, hashAlg = hashAlg))

This line looks a little long - we've been breaking them around 99 characters.
Also, maybe just say '...sign(tbsDER, hashAlg)' since 'hashAlg = hashAlg' seems unnecessarily verbose.
Attachment #8642337 - Flags: review?(dkeeler) → review+
Addressed all review comments.

r=keeler
Attachment #8642337 - Attachment is obsolete: true
Attachment #8642942 - Flags: review+
Comment on attachment 8642334 [details] [diff] [review]
0004-Bug-1183718-Add-SHA-1-deprecation-tests.patch, v2

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

Looking good, but there's a bit of cleanup/clarification to be done. r- for now.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +414,5 @@
>    return utilBin;
>  }
>  
>  // Do not call this directly; use add_tls_server_setup
> +function _setupTLSServerTest(serverBinName, options)

These changes should be propagated up to add_tls_server_setup, which does the add_test part itself, so we don't have to do that in test_sha1_deprecation.js. We should also document the properties of options.

::: security/manager/ssl/tests/unit/test_sha1_deprecation.js
@@ +77,5 @@
> +
> +function run_test() {
> +  do_get_profile();
> +
> +  add_test(function () {

use add_tls_server_setup instead

::: security/manager/ssl/tests/unit/test_sha1_deprecation/default.pem.certspec
@@ +1,4 @@
> +issuer:int
> +subject:localhostAndExampleCom
> +extension:basicConstraints:,
> +extension:keyUsage:digitalSignature,nonRepudiation,keyEncipherment,dataEncipherment,keyAgreement

We generally haven't been specifying basicConstraints or keyUsage for end-entities, but it's not a big deal.
Now that I think of it, though, what's this certificate for? It's only valid for the name 'localhostAndExampleCom', which we never connect to.

::: security/manager/ssl/tests/unit/test_sha1_deprecation/ee-2015.pem.certspec
@@ +1,2 @@
> +issuer:int-sha1
> +subject:sha1-ee-signature-2015.example.com

I think this name got switched with sha1-int-signature-2015.example.com, unless I'm misunderstanding.

@@ +2,5 @@
> +subject:sha1-ee-signature-2015.example.com
> +validity:20150101-20151231
> +extension:basicConstraints:,
> +extension:subjectAlternativeName:sha1-ee-signature-2015.example.com
> +extension:keyUsage:digitalSignature,nonRepudiation,keyEncipherment,dataEncipherment,keyAgreement

Same comment regarding basicConstraints and keyUsage. Although, we should maybe define and use the canonically "correct" certificate specifications for roots, intermediates, and end-entities for all newly added certspec files (e.g. the baseline requirements say end-entity certificates must have the extendedKeyUsage extension with serverAuth and/or clientAuth asserted, which we don't do currently).

::: security/manager/ssl/tests/unit/test_sha1_deprecation/ee-sha1-2015.pem.certspec
@@ +1,2 @@
> +issuer:int
> +subject:sha1-int-signature-2015.example.com

I think this name got switched with sha1-ee-signature-2015.example.com (and shouldn't this certificate have a sha1 signature?)

::: security/manager/ssl/tests/unit/test_sha1_deprecation/moz.build
@@ +25,5 @@
> +    props.inputs = [input_file]
> +    TEST_HARNESS_FILES.xpcshell.security.manager.ssl.tests.unit.test_sha1_deprecation += ['!%s' % test_certificate]
> +
> +test_keys = (
> +    'default.key',

Is the default.key.keyspec file included in this patch? Or is splinter just not showing it...?

::: security/manager/ssl/tests/unit/tlsserver/cmd/Sha1CertServer.cpp
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Can we use BadCertServer for this? I think it would be best to avoid adding a new binary.

@@ +189,5 @@
> +  return SECSuccess;
> +}
> +
> +SECStatus
> +LoadCertificatesAndKeys(const char* basePath)

This basically comes from GenerateOCSPResponse, right? We should just factor out the NSS initialization logic so all of these test binaries can use it.

::: security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.cpp
@@ +239,5 @@
>      certList->len = 2;
>    }
>  
> +  ScopedPK11SlotInfo slot(PK11_GetInternalKeySlot());
> +  ScopedSECKEYPrivateKey key(PK11_FindKeyByDERCert(slot, cert, nullptr));

What was the issue here? Was PK11_FindKeyByAnyCert finding the wrong key?
Attachment #8642334 - Flags: review?(dkeeler) → review-
(In reply to David Keeler [:keeler] (use needinfo?) from comment #45)
> security/manager/ssl/tests/unit/test_sha1_deprecation/default.pem.certspec
> Now that I think of it, though, what's this certificate for? It's only valid
> for the name 'localhostAndExampleCom', which we never connect to.

It's the default cert's nickname as specified in TLSServer.cpp:

const char DEFAULT_CERT_NICKNAME[] = "localhostAndExampleCom";

> security/manager/ssl/tests/unit/test_sha1_deprecation/ee-2015.pem.certspec
> @@ +1,2 @@
> > +issuer:int-sha1
> > +subject:sha1-ee-signature-2015.example.com
> 
> I think this name got switched with sha1-int-signature-2015.example.com,
> unless I'm misunderstanding.

Right, thx!

> @@ +2,5 @@
> > +extension:basicConstraints:,
> > +extension:keyUsage:digitalSignature,nonRepudiation,keyEncipherment,dataEncipherment,keyAgreement

Removed from all certspecs.

> security/manager/ssl/tests/unit/test_sha1_deprecation/ee-sha1-2015.pem.
> certspec
> @@ +1,2 @@
> > +issuer:int
> > +subject:sha1-int-signature-2015.example.com
> 
> I think this name got switched with sha1-ee-signature-2015.example.com (and
> shouldn't this certificate have a sha1 signature?)

Thanks again :)

> ::: security/manager/ssl/tests/unit/test_sha1_deprecation/moz.build
> > +test_keys = (
> > +    'default.key',
> 
> Is the default.key.keyspec file included in this patch? Or is splinter just
> not showing it...?

Splinter seems to hide it, can see it when looking at the raw patch:

diff --git a/security/manager/ssl/tests/unit/test_sha1_deprecation/default.key.keyspec b/security/manager/ssl/tests/unit/test_sha1_deprecation/default.key.keyspec
new file mode 100644
index 0000000..e69de29

> ::: security/manager/ssl/tests/unit/tlsserver/cmd/Sha1CertServer.cpp
> Can we use BadCertServer for this? I think it would be best to avoid adding
> a new binary.

I tried, I really didn't want to write a custom binary :)

BadCertServer expects a pre-populated NSS db with certs and keys but pycert/key.py can't do that unfortunately. I can't use the usual "tlsserver" db as that would require me to use the old generate_certs.sh script. Can't specify a new directory because that would require a valid NSS cert db.

What the Sha1CertServer basically does is import all necessary certs and keys and build a temporary DB. I guess it might make sense to rename it later when the BadCertServer is migrated to pycert.py? We could have a generic server importing certs and keys, build a temp DB, and then accept connections using maybe the subjectAlternativeName as the host (or something more flexible).

How about doing that in a follow-up? Or in that bug that would convert BadCertServer tests?

> > +SECStatus
> > +LoadCertificatesAndKeys(const char* basePath)
> 
> This basically comes from GenerateOCSPResponse, right? We should just factor
> out the NSS initialization logic so all of these test binaries can use it.

Most of that stuff, yeah. It does pick cert nicknames differently though.

> ::: security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.cpp
> > +  ScopedPK11SlotInfo slot(PK11_GetInternalKeySlot());
> > +  ScopedSECKEYPrivateKey key(PK11_FindKeyByDERCert(slot, cert, nullptr));
> 
> What was the issue here? Was PK11_FindKeyByAnyCert finding the wrong key?

It didn't find the key, I still don't fully understand why honestly...
Flags: needinfo?(dkeeler)
(In reply to Tim Taubert [:ttaubert] from comment #46)
> (In reply to David Keeler [:keeler] (use needinfo?) from comment #45)
> > @@ +2,5 @@
> > > +extension:basicConstraints:,
> > > +extension:keyUsage:digitalSignature,nonRepudiation,keyEncipherment,dataEncipherment,keyAgreement
> 
> Removed from all certspecs.

From all EE certspecs.
The approach I was hoping for was to factor out the logic in GenerateOCSPResponse that uses an NSS DB if present and otherwise loads all .pem and .key files in the specified directory so that BadCertServer can use it. I don't think that would be significantly more work than adding a new binary. The changes to head_psm.js would still be relevant - that would be how we would tell BadCertServer where to find its certificates/keys if we want to use something other than tlsserver/.
Flags: needinfo?(dkeeler)
Extended the BadCertServer and made GenerateOCSPResponse use the same code. That's a lot better, thanks for the suggestion! All xpcshell tests pass with a debug build, but I'll push to try.
Attachment #8642334 - Attachment is obsolete: true
Attachment #8645412 - Flags: review?(dkeeler)
Comment on attachment 8636435 [details] [diff] [review]
0001-Bug-1183718-Set-STATE_IS_BROKEN-if-the-cert-chain-co.patch, v4

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

::: uriloader/base/nsIWebProgressListener.idl
@@ +16,5 @@
>   * requests in the context of a nsIWebProgress instance as well as any child
>   * nsIWebProgress instances.  nsIWebProgress.idl describes the parent-child
>   * relationship of nsIWebProgress instances.
>   */
> +[scriptable, uuid(a5f7535f-ff31-4540-ba97-04f34728e6c2)]

I'm not sure whether it's necessary here, but FWIW |mach update-uuids nsIWebProgressListener| updates the uuids on these files as well:
> layout/printing/nsIPrintProgress.idl
> toolkit/components/downloads/nsIDownload.idl
> uriloader/base/nsITransfer.idl
> uriloader/base/nsIWebProgressListener2.idl
(In reply to Cykesiopka from comment #51)
> > +[scriptable, uuid(a5f7535f-ff31-4540-ba97-04f34728e6c2)]
> 
> I'm not sure whether it's necessary here, but FWIW |mach update-uuids
> nsIWebProgressListener| updates the uuids on these files as well:

Oh, right. I sometimes forget bumping descendant interfaces :| Thanks!
Carrying over r=keeler.
Attachment #8636435 - Attachment is obsolete: true
Attachment #8645418 - Flags: review+
Had to update the validity checks as they were overflowing on 32-bit. Simply changed them from:

(1451606400 * PR_USEC_PER_SEC) > notAfter

to:

1451606400 > (notAfter / PR_USEC_PER_SEC)
Attachment #8645418 - Attachment is obsolete: true
Attachment #8645430 - Flags: review+
Had to use NSS_Initialize() instead of NSS_InitReadWrite() as the latter doesn't seem to be exported on Windows.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=346c7ee7fe32
Attachment #8645412 - Attachment is obsolete: true
Attachment #8645412 - Flags: review?(dkeeler)
Attachment #8645431 - Flags: review?(dkeeler)
Comment on attachment 8645431 [details] [diff] [review]
0004-Bug-1183718-Add-SHA-1-deprecation-tests.patch, v4

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

Looks good. Just a couple of nits/questions. r=me with those addressed.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +267,5 @@
>  
> +Accepts the following options:
> +
> +{
> +  path: "<folder containing certs or cert DB used by the server>",

Maybe "certDBPath" or similar?

@@ -453,5 @@
>    let process = Cc["@mozilla.org/process/util;1"].createInstance(Ci.nsIProcess);
>    process.init(serverBin);
> -  let certDir = directoryService.get("CurWorkD", Ci.nsILocalFile);
> -  certDir.append("tlsserver");
> -  Assert.ok(certDir.exists(), "tlsserver folder should exist");

Let's keep the assertion that the given cert DB directory exists.

::: security/manager/ssl/tests/unit/test_sha1_deprecation.js
@@ +9,5 @@
> +// signature do not mark connection security as broken as long as the EE cert
> +// expires in 2015.
> +function add_2015_connection_tests() {
> +  // Skip these when we reach 2016.
> +  if (1900 + new Date().getYear() > 2015) {

Let's use getFullYear() for these.

@@ +81,5 @@
> +  let tmp = do_get_tempdir();
> +  let certDir = do_get_cwd();
> +  certDir.append("test_sha1_deprecation");
> +
> +  // Copy all certs to the temp directory.

Why is this necessary? Can't we just pass in certDir.path to add_tls_server_setup?

::: security/manager/ssl/tests/unit/test_sha1_deprecation/default.pem.certspec
@@ +1,1 @@
> +issuer:int

I'm pretty sure we don't actually need this - just the default.key.keyspec file.

::: security/manager/ssl/tests/unit/test_sha1_deprecation/moz.build
@@ +25,5 @@
> +    props.inputs = [input_file]
> +    TEST_HARNESS_FILES.xpcshell.security.manager.ssl.tests.unit.test_sha1_deprecation += ['!%s' % test_certificate]
> +
> +test_keys = (
> +    'default.key',

If this lands after bug 1190532, you'll need to include the specification 'default' in the default.key.keyspec file.

::: security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
@@ +81,5 @@
> +DoSNISocketConfigBySubjectCN(PRFileDesc *aFd, const SECItem *aSrvNameArr,
> +                             uint32_t aSrvNameArrSize)
> +{
> +  for (uint32_t i = 0; i < aSrvNameArrSize; i++) {
> +    ScopedCERTCertificate cert;

Since we don't actually use cert or certKEA and they're optional in ConfigSecureServerWithNamedCert, it seems like we may as well just omit them.

@@ +83,5 @@
> +{
> +  for (uint32_t i = 0; i < aSrvNameArrSize; i++) {
> +    ScopedCERTCertificate cert;
> +    SSLKEAType certKEA;
> +    SECStatus rv;

nit: move this to where it gets a value from ConfigSecureServerWithNamedCert (actually, if you use a ScopedPORTString for name, we don't need to store rv at all).

@@ +85,5 @@
> +    ScopedCERTCertificate cert;
> +    SSLKEAType certKEA;
> +    SECStatus rv;
> +
> +    char* name = (char*)PORT_ZAlloc(aSrvNameArr[i].len + 1);

nit: check that name isn't null
Also, let's use a ScopedPORTString here.

@@ +106,5 @@
>  {
>    const BadCertHost *host = GetHostForSNI(aSrvNameArr, aSrvNameArrSize,
>                                            sBadCertHosts);
>    if (!host) {
> +    // No static cert <-> hostname mapping found.

I would add that this is occurs when we're using a collection of certificates rather than an NSS cert DB. Also, it seems like in the future this is the only mechanism we need to use (so the static mapping can go away in the future, which would be good).

::: security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.cpp
@@ +206,5 @@
> +  if (!slot) {
> +    PrintPRError("PK11_GetInternalKeySlot failed");
> +    return SECFailure;
> +  }
> +  rv = PK11_ImportCert(slot, cert, CK_INVALID_HANDLE, cert->subjectName + 3, false);

If I'm understanding the use of 'subjectName + 3' correctly, I think it might be best to use CERT_GetCommonName instead (use a ScopedPORTString (also, it would be a good idea to null-check it before using it)). 

Also, this line is >80 characters, so we should break it up.

@@ +237,5 @@
> +  // filename collection and then loading. (This is probably a good
> +  // idea anyway because readdir isn't reentrant. Something could change later
> +  // such that it gets called as a result of calling AddCertificateFromFile or
> +  // AddKeyFromFile.)
> +  std::vector<std::string> certificates, keys;

Style nit: one declaration per line ('std::vector<std::string> keys;' should be its own line)

@@ +511,5 @@
>      gCallbackPort = atoi(callbackPort);
>    }
>  
> +  if (InitializeNSS(nssCertDBDir) != SECSuccess) {
> +    PrintPRError("InitializeNSS failed");

PrintPRError really only works when called directly after a call to a NSS/NSPR function, so this should either be removed or replaced with a fprintf/PR_fprintf.
Attachment #8645431 - Flags: review?(dkeeler) → review+
(In reply to David Keeler [:keeler] (use needinfo?) from comment #56)
> > +Accepts the following options:
> > +
> > +{
> > +  path: "<folder containing certs or cert DB used by the server>",
> 
> Maybe "certDBPath" or similar?

Done.

> > -  let certDir = directoryService.get("CurWorkD", Ci.nsILocalFile);
> > -  certDir.append("tlsserver");
> > -  Assert.ok(certDir.exists(), "tlsserver folder should exist");
> 
> Let's keep the assertion that the given cert DB directory exists.

Re-added.

> > +function add_2015_connection_tests() {
> > +  // Skip these when we reach 2016.
> > +  if (1900 + new Date().getYear() > 2015) {
> 
> Let's use getFullYear() for these.

Oh, right. I always forget that exists.

> @@ +81,5 @@
> > +  let tmp = do_get_tempdir();
> > +  let certDir = do_get_cwd();
> > +  certDir.append("test_sha1_deprecation");
> > +
> > +  // Copy all certs to the temp directory.
> 
> Why is this necessary? Can't we just pass in certDir.path to
> add_tls_server_setup?

We could just do that but initializing a certificate DB creates a few files (cert8.db, key3.db, secmod.db). So if you run tests locally we would leave those in the test directory and they would show up as untracked files in the VCS. I thought it would be better to avoid that by copying them to the test's temp dir that is removed right after.

> > +issuer:int
> 
> I'm pretty sure we don't actually need this - just the default.key.keyspec
> file.

Right, copy&paste, sorry.

> > +test_keys = (
> > +    'default.key',
> 
> If this lands after bug 1190532, you'll need to include the specification
> 'default' in the default.key.keyspec file.

Updated.

> > +DoSNISocketConfigBySubjectCN(PRFileDesc *aFd, const SECItem *aSrvNameArr,
> > +                             uint32_t aSrvNameArrSize)
> > +{
> > +  for (uint32_t i = 0; i < aSrvNameArrSize; i++) {
> > +    ScopedCERTCertificate cert;
> 
> Since we don't actually use cert or certKEA and they're optional in
> ConfigSecureServerWithNamedCert, it seems like we may as well just omit them.

Good point.

> > +    char* name = (char*)PORT_ZAlloc(aSrvNameArr[i].len + 1);
> 
> nit: check that name isn't null
> Also, let's use a ScopedPORTString here.

Done.

> >    const BadCertHost *host = GetHostForSNI(aSrvNameArr, aSrvNameArrSize,
> >                                            sBadCertHosts);
> >    if (!host) {
> > +    // No static cert <-> hostname mapping found.
> 
> I would add that this is occurs when we're using a collection of
> certificates rather than an NSS cert DB.

Done.

> Also, it seems like in the future
> this is the only mechanism we need to use (so the static mapping can go away
> in the future, which would be good).

Yeah, indeed.

> > +  rv = PK11_ImportCert(slot, cert, CK_INVALID_HANDLE, cert->subjectName + 3, false);
> 
> If I'm understanding the use of 'subjectName + 3' correctly, I think it
> might be best to use CERT_GetCommonName instead (use a ScopedPORTString
> (also, it would be a good idea to null-check it before using it)). 

Ah, I wondered about a better way to do this, thanks.

> > +  std::vector<std::string> certificates, keys;
> 
> Style nit: one declaration per line ('std::vector<std::string> keys;' should
> be its own line)

Done.

> > +  if (InitializeNSS(nssCertDBDir) != SECSuccess) {
> > +    PrintPRError("InitializeNSS failed");
> 
> PrintPRError really only works when called directly after a call to a
> NSS/NSPR function, so this should either be removed or replaced with a
> fprintf/PR_fprintf.

Fixed.
Unassigning until we figured out whether we want this or not.
Assignee: ttaubert → nobody
Status: ASSIGNED → NEW
Just note that we are going to untrust SHA-1 generally on July 1, 2016:
https://blog.mozilla.org/security/2015/10/20/continuing-to-phase-out-sha-1-certificates/
Whiteboard: [psm-backlog]
SHA-1 is disabled by default now, so I don't think we need to do this.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: