1068949 - Add SHA-1 warnings to web console for end entities

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Mark Goodwin [:mgoodwin]]

SHA-1 is on the way out. While SHA-1 certificates remain valid in the short term, developers need to be aware if their certificate (or its chains) uses SHA-1.

Comment 1

[Kathleen Wilson]

Please also make sure the warnings show up in Firebug.

Comment 2

[Mark Goodwin [:mgoodwin]]

(In reply to Kathleen Wilson from comment #1)
> Please also make sure the warnings show up in Firebug.

I've spoken with Honza; Firebug does nothing to change the visibility of devtools web console messages - though it can provide customization and append additional info if required.

In any case, many developers are now finding the developer tools provide the functionality they need without resorting to the use of Firebug.

Comment 3

[Mark Goodwin [:mgoodwin]]

Attachment: 1068949

This is an initial attempt at making sha-1 certificate warnings appear in the web console.

Turns out we don't need to do this in nsSecureBrowserUIImpl - it looks like it can all live in nsHttpChannel.cpp 

At the moment, I'm getting an nsIASN1Tree from the nsIX509Cert and searching the displayData from that to find SHA-1 - this doesn't feel right. Is there a way I can compare on OIDs? If so, how? Is there a list (in source or documentation) of signatureAlgorithm OIDs and what they correspond to?

Comment 4

[Mark Goodwin [:mgoodwin]]

Attachment: 1068949_tests

A test for a top level load from a server with a SHA-1 cert

Comment 5

[Mark Goodwin [:mgoodwin]]

It occurs to me that I need to check more than just the end entity (which is what my code currently does) here - we need to check everything except the root here, don't we?

Comment 6

[Mark Goodwin [:mgoodwin]]

So I started sketching out how to do check other parts of the chain. This doesn't look hard but it's becoming increasingly clear that, as more cert related operations happen, this doesn't belong in nsHttpChannel; reason being, we need to import various pkix types, the certverifier, etc. Even if this isn't frowned upon, it's messy and doesn't feel right.

I don't think it should belong in nsSecureBrowserUIImpl either (since we no longer need window IDs at the time security messages are generated).

Would it be reasonable to, for example, modify the interface of nsISiteSecurityService (or something else 'security' that's used in /netwerk) to allow warnings, etc. to be generated for a given channel since we'll want to do more of these messages in future (for this to work, we'd need to expose AddSecurityMessage)?

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8512596 [details] [diff] [review]
1068949

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

This is a valiant effort, but I think we should avoid using nsIASN1Object and friends. I think there are two pre-requisites before we'll be able to do this:
1. Exposing the signature algorithm in nsIX509Cert (probably easiest by essentially exposing CERTCertificate.signature)
2. Stashing the verified certificate chain on the channel's nsITransportSecurityInfo or nsISSLStatus or something (note that session resumption makes this a little tricky - we'll probably just have to re-build the chain with local-only information for the time being).
Then, whereever we want, we can loop over the verified chain and see if any of the signature algorithms are insufficient (note that the signature on the root should be ignored, since it's not actually important to check).

I think checking/adding the warnings in nsHttpChannel makes sense, but a necko peer would have more insight.

::: dom/locales/en-US/chrome/security/security.properties
@@ +10,5 @@
>  InvalidSTSHeaders=The site specified an invalid Strict-Transport-Security header.
>  # LOCALIZATION NOTE: Do not translate "Public-Key-Pins or HPKP"
>  InvalidPKPHeaders=The site specified an invalid Public-Key-Pins header.
> +# LOCALIZATION NOTE: Do not translate "SHA-1"
> +SHA1Sig=This site makes use of an SHA-1 Certificate; it's recommended you use certificates with signature algorithms that use hash functions stronger than SHA-1.

Should we generalize this to "weak signature"? (That way, we can also warn on MD2/MD5/etc. (which, I realize, we already make users add an override for, but still.))

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Mark Goodwin [:mgoodwin] from comment #3)
> At the moment, I'm getting an nsIASN1Tree from the nsIX509Cert and searching
> the displayData from that to find SHA-1 - this doesn't feel right. Is there
> a way I can compare on OIDs? If so, how? Is there a list (in source or
> documentation) of signatureAlgorithm OIDs and what they correspond to?

This is probably a good list to start with: https://mxr.mozilla.org/mozilla-central/source/security/pkix/include/pkix/pkixtypes.h#46
We might need to expand that and/or expose some DER OID decoding/matching functions.

Comment 9

[Mark Goodwin [:mgoodwin]]

Initially, I'm looking for sha-1WithRSAEncryption, id-dsa-with-sha1 and ecdsa_with_sha1 - is this sane?

(In reply to David Keeler (:keeler) [use needinfo?] from comment #8)
> We might need to expand that and/or expose some DER OID decoding/matching
> functions.

By expanding, are you thinking of other weak signatures?

For matching; I can get a SECOidTag using SECOID_GetAlgorithmTag - this looks like a fairly comprehensive list.

Comment 10

[Mark Goodwin [:mgoodwin]]

Attachment: bug1068949_sha1messages.patch

Modified to use the NSS functions for checking signature types

Comment 11

[Mark Goodwin [:mgoodwin]]

Attachment: 01_bug1068949_sha1messages.patch

Added a link to MDN for weak signature algorithms

Comment 12

[Mark Goodwin [:mgoodwin]]

Attachment: 02_bug1068949_tests.patch

Added certs for explicitly testing sha-1 and sha-256 signed certificates.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8516008 [details] [diff] [review]
01_bug1068949_sha1messages.patch

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

LGTM, but yeah, you'll need review from necko peers. I pointed out a few nits, in any case.

::: dom/locales/en-US/chrome/security/security.properties
@@ +10,5 @@
>  InvalidSTSHeaders=The site specified an invalid Strict-Transport-Security header.
>  # LOCALIZATION NOTE: Do not translate "Public-Key-Pins or HPKP"
>  InvalidPKPHeaders=The site specified an invalid Public-Key-Pins header.
> +# LOCALIZATION NOTE: Do not translate "SHA-1"
> +SHA1Sig=This site makes use of an SHA-1 Certificate; it's recommended you use certificates with signature algorithms that use hash functions stronger than SHA-1.

nit: 'a SHA-1 certificate'

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +69,5 @@
>  #include "AlternateServices.h"
>  #include "InterceptedChannel.h"
>  #include "nsIHttpPushListener.h"
> +#include "nsIASN1Tree.h"
> +#include "nsIASN1Object.h"

nit: the ASN1 #includes shouldn't be necessary anymore

@@ +1218,5 @@
>          return;
>  
> +    // Send (SHA-1) signature algorithm errors to the web console
> +    nsCOMPtr<nsIX509Cert> cert;
> +    if (NS_OK == sslstat->GetServerCert(getter_AddRefs(cert))) {

nit: commonly this comparison is switched around, but I would see what the necko peers say

@@ +1221,5 @@
> +    nsCOMPtr<nsIX509Cert> cert;
> +    if (NS_OK == sslstat->GetServerCert(getter_AddRefs(cert))) {
> +        ScopedCERTCertificate nssCert(cert->GetCert());
> +        if (nssCert) {
> +            SECOidTag tag = SECOID_GetAlgorithmTag(& nssCert->signature);

nit: &nssCert->signature should be fine

@@ +1222,5 @@
> +    if (NS_OK == sslstat->GetServerCert(getter_AddRefs(cert))) {
> +        ScopedCERTCertificate nssCert(cert->GetCert());
> +        if (nssCert) {
> +            SECOidTag tag = SECOID_GetAlgorithmTag(& nssCert->signature);
> +            LOG(("Checking certificate signature: The OID tag is %i\n",tag));

nit: space after ','

@@ +1229,5 @@
> +            // from http://tools.ietf.org/html/rfc2437#section-8 since I
> +            // can't see reference to it outside this spec
> +            if (SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION == tag
> +                    || SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST == tag
> +                    || SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE == tag) {

nit: I think the style for this commonly is:

if (tag == SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION ||
    tag == SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST ||
etc.

(then again, go with what the necko peers say)

@@ +1231,5 @@
> +            if (SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION == tag
> +                    || SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST == tag
> +                    || SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE == tag) {
> +                nsString consoleErrorTag;
> +                consoleErrorTag = NS_LITERAL_STRING("SHA1Sig");

nit: I would declare and define this on the same line (I guess unless it goes over 80 chars?)

@@ +1233,5 @@
> +                    || SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE == tag) {
> +                nsString consoleErrorTag;
> +                consoleErrorTag = NS_LITERAL_STRING("SHA1Sig");
> +                nsString consoleErrorMessage;
> +                consoleErrorMessage = NS_LITERAL_STRING("SHA-1 Signature");

same here

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8516010 [details] [diff] [review]
02_bug1068949_tests.patch

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

LGTM in general, but you should get review from a browser and/or devtools peer.

::: browser/devtools/webconsole/test/browser_webconsole_bug_1068949_sha1_cert.js
@@ +11,5 @@
> +const TEST_GOOD_URI = "https://sha256ee.example.com/browser/browser/devtools/webconsole/test/test-crypto-securityerrors.html";
> +const TRIGGER_MSG = 'If you haven\'t seen ssl warnings yet, you won\'t';
> +const WARNING_MSG = 'This site makes use of an SHA-1 Certificate; it\'s recommended you use certificates with signature algorithms that use hash functions stronger than SHA-1.'
> +
> +let hud = undefined;

maybe gHud?

@@ +21,5 @@
> +    openConsole(null, loadBadDocument);
> +  }, true);
> +}
> +
> +function loadBadDocument(theHud){

nit: space before {

@@ +32,5 @@
> +  browser.removeEventListener("load", onBadLoad, true);
> +  testForWarningMessage();
> +}
> +
> +function loadGoodDocument(theHud){

nit: same

Comment 15

[Mark Goodwin [:mgoodwin]]

Attachment: 02_bug1068949_tests.patch

The test is 100% web console; would you mind taking a look?

Comment 16

[Mark Goodwin [:mgoodwin]]

Attachment: 01_bug1068949_sha1messages.patch

Nits addressed.

Comment 17

[Patrick McManus [:mcmanus]]

Comment on attachment 8516695 [details] [diff] [review]
01_bug1068949_sha1messages.patch

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

r+ with a couple nits addressed.. thanks

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1217,5 @@
>          return;
>  
> +    // Send (SHA-1) signature algorithm errors to the web console
> +    nsCOMPtr<nsIX509Cert> cert;
> +    if (sslstat->GetServerCert(getter_AddRefs(cert)) == NS_OK) {

if(NS_SUCCEEDED(sslstat->blah())) {
or more simply if (cert)

@@ +1221,5 @@
> +    if (sslstat->GetServerCert(getter_AddRefs(cert)) == NS_OK) {
> +        ScopedCERTCertificate nssCert(cert->GetCert());
> +        if (nssCert) {
> +            SECOidTag tag = SECOID_GetAlgorithmTag(&nssCert->signature);
> +            LOG(("Checking certificate signature: The OID tag is %i\n", tag));

put nshttpchannel and %p this in log

Comment 18

[Mark Goodwin [:mgoodwin]]

Attachment: 01_bug1068949_sha1messages.patch

'if' cleaned up, 'this' added to LOG call (as per review feedback)

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8516692 [details] [diff] [review]
02_bug1068949_tests.patch

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

Just a few nits and minor issues.

::: browser/devtools/webconsole/test/browser.ini
@@ +265,5 @@
>  skip-if = buildapp == 'mulet'
>  [browser_webconsole_bug_915141_toggle_response_logging_with_keyboard.js]
>  [browser_webconsole_bug_1006027_message_timestamps_incorrect.js]
>  [browser_webconsole_bug_1010953_cspro.js]
> +[browser_webconsole_bug_1068949_sha1_cert.js]

We no longer use the bug number in the file name.

::: browser/devtools/webconsole/test/browser_webconsole_bug_1068949_sha1_cert.js
@@ +2,5 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + *
> + * ***** END LICENSE BLOCK ***** */

Nit: you can drop the BEGIN/END lines.

@@ +9,5 @@
> +
> +const TEST_BAD_URI = "https://sha1ee.example.com/browser/browser/devtools/webconsole/test/test-crypto-securityerrors.html";
> +const TEST_GOOD_URI = "https://sha256ee.example.com/browser/browser/devtools/webconsole/test/test-crypto-securityerrors.html";
> +const TRIGGER_MSG = 'If you haven\'t seen ssl warnings yet, you won\'t';
> +const WARNING_MSG = 'This site makes use of a SHA-1 Certificate; it\'s recommended you use certificates with signature algorithms that use hash functions stronger than SHA-1.'

Nit: you could use double quotes for these strings and avoid escaping the apostrophe if you want.

@@ +49,5 @@
> +
> +  waitForSuccess({
> +      name: "SHA1 warning displayed successfully",
> +      validatorFn: function() {
> +        return gHud.outputNode.textContent.indexOf(WARNING_MSG) > -1;

Are we sure the warning message will not be reworded in the future? Wouldn't a loose match against "SHA-1" suffice here?

Comment 20

[Mark Goodwin [:mgoodwin]]

Attachment: 02_bug1068949_tests.patch

Nits addressed.

Thanks.

Comment 21

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f8a92658d6f8

Comment 22

[Mark Goodwin [:mgoodwin]]

Attachment: 02_bug1068949_tests.patch

Address window leakage test failure

Comment 23

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e120cb4cd4ae

Comment 24

[Mark Goodwin [:mgoodwin]]

Clearing dev-doc-needed since documentation has been provided (see bug 1092050)

Comment 25

[Carsten Book [:Tomcat]]

Hi Mark,

part2 fails to apply:

renamed 1068949 -> 02_bug1068949_tests.patch
applying 02_bug1068949_tests.patch
patching file browser/devtools/webconsole/test/browser.ini
Hunk #1 FAILED at 77
1 out of 2 hunks FAILED -- saving rejects to file browser/devtools/webconsole/test/browser.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 02_bug1068949_tests.patch

could you take a look? Thanks!

Comment 26

[Mark Goodwin [:mgoodwin]]

Attachment: 02_bug1068949_tests.patch

(In reply to Carsten Book [:Tomcat] from comment #25)
> could you take a look? Thanks!

Sorted

Comment 27

[Mark Goodwin [:mgoodwin]]

Also builds cleanly, local tests are still good.

I take it I don't need another try?

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/4d9d00258813
https://hg.mozilla.org/integration/fx-team/rev/96e2e929a9f2

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d9d00258813
https://hg.mozilla.org/mozilla-central/rev/96e2e929a9f2

Comment 30

[Mike Kaply [:mkaply]]

Can we get a way to turn this off via a pref please for developers? It makes the console unusable.

If we don't, developers will simply turn off security warnings in the console, and that's not a good thing.

Comment 31

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Mike Kaply [:mkaply] from comment #30)
> Can we get a way to turn this off via a pref please for developers? It makes
> the console unusable.
> 
> If we don't, developers will simply turn off security warnings in the
> console, and that's not a good thing.

I don't really see the value in hiding only a specific type of security warning...  Why is this message seemingly less valuable then other security warnings?

Comment 32

[Mike Kaply [:mkaply]]

(In reply to J. Ryan Stinnett [:jryans] from comment #31)
> I don't really see the value in hiding only a specific type of security
> warning...  Why is this message seemingly less valuable then other security
> warnings?

Because it happens a lot, including many mozilla sites and google sites that mozilla sites reference. Honestly, it happens on pretty much every SSL site we tested.

At the very least, we should have at least waited to turn this on until we were following the advice we recommend to other sites.

Comment 33

[Mike Kaply [:mkaply]]

I didn't mean "we tested", I meant "I tested". Sorry.

Comment 34

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Mike Kaply [:mkaply] from comment #32)
> (In reply to J. Ryan Stinnett [:jryans] from comment #31)
> > I don't really see the value in hiding only a specific type of security
> > warning...  Why is this message seemingly less valuable then other security
> > warnings?
> 
> Because it happens a lot, including many mozilla sites and google sites that
> mozilla sites reference. Honestly, it happens on pretty much every SSL site
> we tested.
> 
> At the very least, we should have at least waited to turn this on until we
> were following the advice we recommend to other sites.

As I see it, the goal of all security warnings today is to point out an outdated or bad practice which is known to be insecure in some way.  So, it seems to make sense to me.

Anyway, if you feel strongly about a separate control, let's discuss it in a new bug.

Comment 35

[Mark Goodwin [:mgoodwin]]

(In reply to J. Ryan Stinnett [:jryans] from comment #34)
> As I see it, the goal of all security warnings today is to point out an
> outdated or bad practice which is known to be insecure in some way.  So, it
> seems to make sense to me.

Also, browsers will soon start treating such resources as mixed content when included from other https servers - we're making the noise for a reason; such sites will start to break soon.

> Anyway, if you feel strongly about a separate control, let's discuss it in a
> new bug.

I agree that noise can be a problem and I'm happy to join in discussions around this.

Comment 36

[Kent James (:rkent)]

Yes the noise is a problem. This error message occurs AFAICT for each and every web service call that I make, which generates A LOT of noise for repeated calls to the same site. All of these errors specifically ignore the warning in the code for HttpBaseChannel::AddSecurityMessage:

1656 /* Please use this method with care. This can cause the message
1657  * queue to grow large and cause the channel to take up a lot
1658  * of memory. Use only static string messages and do not add
1659  * server side data to the queue, as that can be large.
1660  * Add only a limited number of messages to the queue to keep
1661  * the channel size down and do so only in rare erroneous situations.
1662  * More information can be found here:
1663  * https://bugzilla.mozilla.org/show_bug.cgi?id=846918
1664  */

Comment 37

[Johnny-five]

I understand that this is an old topic, however, it is a new and continued annoyance to my development team.  Please limit the error "This site makes use of a SHA-1 Certificate; it's recommended you use certificates with signature algorithms that use hash functions stronger than SHA-1." to 1 time per browser session.  The spamming is more ridiculous then even the notion of singling out this one specific error.  It is impeding progress and preventing developers from being as effective as they once were.  Btw: it doesn't always correctly identify Sha-1 from sha-256 but that's another issue.

Comment 38

[Panos Astithas (he/him) [:past] (please ni?)]

That's bug 1170476.