Closed Bug 1068949 Opened 5 years ago Closed 5 years ago

Add SHA-1 warnings to web console for end entities

Categories

(DevTools :: Console, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: mgoodwin, Assigned: mgoodwin)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 9 obsolete files)

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.
Please also make sure the warnings show up in Firebug.
(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.
See Also: → 942515
Keywords: dev-doc-needed
OS: Linux → All
Hardware: x86_64 → All
Attached patch 1068949 (obsolete) — Splinter Review
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?
Attachment #8512596 - Flags: feedback?(dkeeler)
Attached patch 1068949_tests (obsolete) — Splinter Review
A test for a top level load from a server with a SHA-1 cert
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?
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)?
Flags: needinfo?(dkeeler)
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.))
Attachment #8512596 - Flags: feedback?(dkeeler) → feedback+
(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.
Flags: needinfo?(dkeeler)
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.
Summary: Add SHA-1 warnings to web console → Add SHA-1 warnings to web console for end entities
See Also: → 1091922
Attached patch bug1068949_sha1messages.patch (obsolete) — Splinter Review
Modified to use the NSS functions for checking signature types
Attachment #8512596 - Attachment is obsolete: true
Depends on: 1092050
Attached patch 01_bug1068949_sha1messages.patch (obsolete) — Splinter Review
Added a link to MDN for weak signature algorithms
Attachment #8514826 - Attachment is obsolete: true
Attachment #8516008 - Flags: review?(dkeeler)
Attached patch 02_bug1068949_tests.patch (obsolete) — Splinter Review
Added certs for explicitly testing sha-1 and sha-256 signed certificates.
Attachment #8512621 - Attachment is obsolete: true
Attachment #8516010 - Flags: review?(dkeeler)
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
Attachment #8516008 - Flags: review?(dkeeler) → feedback+
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
Attachment #8516010 - Flags: review?(dkeeler) → feedback+
Attached patch 02_bug1068949_tests.patch (obsolete) — Splinter Review
The test is 100% web console; would you mind taking a look?
Attachment #8516010 - Attachment is obsolete: true
Attachment #8516692 - Flags: review?(past)
Attached patch 01_bug1068949_sha1messages.patch (obsolete) — Splinter Review
Nits addressed.
Attachment #8516008 - Attachment is obsolete: true
Attachment #8516695 - Flags: review?(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
Attachment #8516695 - Flags: review?(mcmanus) → review+
'if' cleaned up, 'this' added to LOG call (as per review feedback)
Attachment #8516695 - Attachment is obsolete: true
Attachment #8516946 - Flags: review+
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?
Attachment #8516692 - Flags: review?(past) → review+
Attached patch 02_bug1068949_tests.patch (obsolete) — Splinter Review
Nits addressed.

Thanks.
Attachment #8516692 - Attachment is obsolete: true
Attachment #8518045 - Flags: review+
Attached patch 02_bug1068949_tests.patch (obsolete) — Splinter Review
Address window leakage test failure
Attachment #8518045 - Attachment is obsolete: true
Attachment #8518110 - Flags: review+
Keywords: checkin-needed
Clearing dev-doc-needed since documentation has been provided (see bug 1092050)
Keywords: dev-doc-needed
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!
Flags: needinfo?(mgoodwin)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #25)
> could you take a look? Thanks!

Sorted
Attachment #8518110 - Attachment is obsolete: true
Flags: needinfo?(mgoodwin)
Attachment #8518714 - Flags: review+
Also builds cleanly, local tests are still good.

I take it I don't need another try?
Flags: needinfo?(cbook)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4d9d00258813
https://hg.mozilla.org/mozilla-central/rev/96e2e929a9f2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Flags: needinfo?(cbook)
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.
(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?
Flags: needinfo?(mozilla)
(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.
Flags: needinfo?(mozilla)
I didn't mean "we tested", I meant "I tested". Sorry.
(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.
(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.
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  */
Depends on: 1152146
Depends on: 1170476
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.