Log usage of weak ciphers in the console

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: annevk, Assigned: emk)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla37
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 9 obsolete attachments)

9.14 KB, patch
keeler
: review+
mcmanus
: review+
Details | Diff | Splinter Review
102.04 KB, patch
past
: review+
Details | Diff | Splinter Review
4.86 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
We should start telling sites this is a bad idea.
(Reporter)

Updated

5 years ago
See Also: → RC4
If we're going to do this, then we should make a general list of weak ciphers and warn on all of them.  Or conversely, make a list of recommended cipher, and warn for anything else.

And we need to publish this guidance, and keep it aligned with what we've already said for servers.

https://wiki.mozilla.org/Security/Server_Side_TLS
Summary: Log RC4 usage in the console → Log usage of weak ciphers in the console
(Assignee)

Comment 2

5 years ago
Posted patch patch (obsolete) — Splinter Review
I recycled STATE_SECURE_LOW and lis_broken_security to indicate that the server uses a weak encryption.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8522228 - Flags: review?(dkeeler)
(Assignee)

Updated

5 years ago
Component: Developer Tools: Console → Security: PSM
Product: Firefox → Core
Comment on attachment 8522228 [details] [diff] [review]
patch

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

If we're going to be making changes to nsSecureBrowserUIImpl and related code, we'll need tests with each new feature.
Also, this code has historically been difficult to work with. It may be worthwhile to clean it up a bit before adding new features (see bug 832834).

::: dom/locales/en-US/chrome/security/security.properties
@@ +22,5 @@
> +
> +# LOCALIZATION NOTE: "%1$S" is the URI contaning a weak encrypted resource.
> +WeakEncryptionWarning="%1$S" uses a weak encryption method.
> +# LOCALIZATION NOTE: "%1$S" is the URI contaning a weak encrypted resource.
> +# %2$S is the name of the encryption standard.

What does "encryption standard" mean in this case?

@@ +30,5 @@
> +# %2$S is the name of the encryption standard.
> +WeakCipherSuiteWarning="%1$S" uses a weak cipher suite (%2$S).
> +# LOCALIZATION NOTE: "%1$S" is the URI contaning a weak encrypted resource.
> +# %2$S is protocol version like "SSL 3".
> +WeakProtocolVersionWarning="%1$S" uses a weak protocol version (%2$S).

Maybe we should have just one generic warning, like so:

"%1$S" was transmitted using %2$S, which is deprecated and insecure.

Where the first argument is the resource and the second argument is, for example, "SSL 3" or "RC4".

::: security/manager/boot/src/nsSecureBrowserUIImpl.cpp
@@ +242,5 @@
>  
>    switch (lock)
>    {
>      case lis_broken_security:
> +      *aState = STATE_IS_BROKEN | STATE_SECURE_LOW;

It isn't necessarily the case that aState should include STATE_SECURE_LOW here - this would only be true if mSubRequestsWeakEncryption > 0, right?

@@ +1325,5 @@
>          ||
>          mSubRequestsNoSecurity)
>      {
>        newSecurityState = lis_mixed_security;
> +      if (mSubRequestsWeakEncryption) {

Shouldn't mSubRequestsWeakEncryption be part of the (mSubRequestsBrokenSecurity || mSubRequestsNoSecurity) disjunction, above?

@@ +1337,5 @@
>    }
>    else
>    if (mNewToplevelSecurityState & STATE_IS_BROKEN)
>    {
> +    newSecurityState = lis_mixed_security;

I don't think this change is correct. Previously this branch always set newSecurityState to lis_broken_security. Why should it be setting it to lis_mixed_security now in some cases?

@@ +1581,5 @@
> +  uint16_t protocolVersion = nsISSLStatus::SSL_VERSION_3;
> +  nsCOMPtr<nsISSLStatus> sslStatus;
> +  if (NS_SUCCEEDED(GetSSLStatus(getter_AddRefs(sslStatus)))) {
> +    sslStatus->GetCipherName(cipherName);
> +    isWeakCipher = cipherName.Find(NS_LITERAL_CSTRING("_RC4_")) >= 0;

Rather than gathering/calculating this information here, shouldn't we depend on nsNSSCallbacks.cpp to set the appropriate flags and then just report them here if they're present? e.g. if the connection is using SSL 3, nsNSSCallbacks will set some flag STATE_USES_SSL_3 which we then test for here and emit a warning if so.

@@ +1619,5 @@
>    ReentrantMonitorAutoEnter lock(mReentrantMonitor);
>  
>    switch (mNotifiedSecurityState)
>    {
> +    case lis_broken_security:

Why was this change necessary?

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +1267,5 @@
>    }
>  
> +  if (weakEncryption) {
> +    infoObject->SetSecurityState(nsIWebProgressListener::STATE_IS_BROKEN |
> +                                 nsIWebProgressListener::STATE_SECURE_LOW);

Describing a state as STATE_IS_BROKEN | STATE_SECURE_LOW is confusing at best. We should get rid of the STATE_SECURE_<whatever> states (see bug 1095602) and introduce a new flag like STATE_USES_WEAK_CRYPTO or something. We could even add a flag like STATE_USES_UNSAFE_RENEGOTIATION.
Attachment #8522228 - Flags: review?(dkeeler) → review-
(Assignee)

Updated

5 years ago
Depends on: 1100917
(Assignee)

Comment 4

5 years ago
> @@ +1337,5 @@
> >    }
> >    else
> >    if (mNewToplevelSecurityState & STATE_IS_BROKEN)
> >    {
> > +    newSecurityState = lis_mixed_security;
> 
> I don't think this change is correct. Previously this branch always set
> newSecurityState to lis_broken_security. Why should it be setting it to
> lis_mixed_security now in some cases?

I merged lis_broken_security and lis_mixed_security because they had little difference. Both states will be converted to STATE_IS_BROKEN and external callers will not see any difference, after all. The only difference was lis_broken_security didn't expose SSLStatus (I'll comment it later).
Attachment #8522228 - Attachment is obsolete: true
Attachment #8525498 - Flags: review?(dkeeler)
(Assignee)

Comment 5

5 years ago
(In reply to David Keeler (:keeler) [use needinfo?] from comment #3)
> ::: dom/locales/en-US/chrome/security/security.properties
> @@ +22,5 @@
> > +
> > +# LOCALIZATION NOTE: "%1$S" is the URI contaning a weak encrypted resource.
> > +WeakEncryptionWarning="%1$S" uses a weak encryption method.
> > +# LOCALIZATION NOTE: "%1$S" is the URI contaning a weak encrypted resource.
> > +# %2$S is the name of the encryption standard.
> 
> What does "encryption standard" mean in this case?

I just copy&pasted it from pippki.properties. Changed to "encryption method".

> @@ +30,5 @@
> > +# %2$S is the name of the encryption standard.
> > +WeakCipherSuiteWarning="%1$S" uses a weak cipher suite (%2$S).
> > +# LOCALIZATION NOTE: "%1$S" is the URI contaning a weak encrypted resource.
> > +# %2$S is protocol version like "SSL 3".
> > +WeakProtocolVersionWarning="%1$S" uses a weak protocol version (%2$S).
> 
> Maybe we should have just one generic warning, like so:
> 
> "%1$S" was transmitted using %2$S, which is deprecated and insecure.
> 
> Where the first argument is the resource and the second argument is, for
> example, "SSL 3" or "RC4".

Done, but I couldn't remove a dedicated message for broken subresources.

> ::: security/manager/boot/src/nsSecureBrowserUIImpl.cpp
> @@ +242,5 @@
> >  
> >    switch (lock)
> >    {
> >      case lis_broken_security:
> > +      *aState = STATE_IS_BROKEN | STATE_SECURE_LOW;
> 
> It isn't necessarily the case that aState should include STATE_SECURE_LOW
> here - this would only be true if mSubRequestsWeakEncryption > 0, right?

STATE_SECURE_LOW (now STATE_BROKEN_SUBRESOURCE) was necessary because otherwise subrequest brokenness was not propagated.
Indeed we need a test :)

> @@ +1325,5 @@
> >          ||
> >          mSubRequestsNoSecurity)
> >      {
> >        newSecurityState = lis_mixed_security;
> > +      if (mSubRequestsWeakEncryption) {
> 
> Shouldn't mSubRequestsWeakEncryption be part of the
> (mSubRequestsBrokenSecurity || mSubRequestsNoSecurity) disjunction, above?

No, the purpose of mSubRequestsWeakEncryption is to distinguish subresources using RC4/SSL3 from other mixed content. Webdevs will waste time to find what subresources are the source of the broken security. Unlike other mixed content, all URLs will start with https://.

> @@ +1581,5 @@
> > +  uint16_t protocolVersion = nsISSLStatus::SSL_VERSION_3;
> > +  nsCOMPtr<nsISSLStatus> sslStatus;
> > +  if (NS_SUCCEEDED(GetSSLStatus(getter_AddRefs(sslStatus)))) {
> > +    sslStatus->GetCipherName(cipherName);
> > +    isWeakCipher = cipherName.Find(NS_LITERAL_CSTRING("_RC4_")) >= 0;
> 
> Rather than gathering/calculating this information here, shouldn't we depend
> on nsNSSCallbacks.cpp to set the appropriate flags and then just report them
> here if they're present? e.g. if the connection is using SSL 3,
> nsNSSCallbacks will set some flag STATE_USES_SSL_3 which we then test for
> here and emit a warning if so.

Done.

> @@ +1619,5 @@
> >    ReentrantMonitorAutoEnter lock(mReentrantMonitor);
> >  
> >    switch (mNotifiedSecurityState)
> >    {
> > +    case lis_broken_security:
> 
> Why was this change necessary?

Because I needed SSLStatus to construct warning messages. (But it is no longer needed, so new flags will not expose SSLStatus).

> ::: security/manager/ssl/src/nsNSSCallbacks.cpp
> @@ +1267,5 @@
> >    }
> >  
> > +  if (weakEncryption) {
> > +    infoObject->SetSecurityState(nsIWebProgressListener::STATE_IS_BROKEN |
> > +                                 nsIWebProgressListener::STATE_SECURE_LOW);
> 
> Describing a state as STATE_IS_BROKEN | STATE_SECURE_LOW is confusing at
> best. We should get rid of the STATE_SECURE_<whatever> states (see bug
> 1095602) and introduce a new flag like STATE_USES_WEAK_CRYPTO or something.
> We could even add a flag like STATE_USES_UNSAFE_RENEGOTIATION.

Done. I added new flags instead of recycling an existing flag to avoid confusions.
But I didn't introduce STATE_USES_UNSAFE_RENEGOTIATION as of now; I couldn't write a test because NSS doesn't provide a way to turn off safe renegotiation.
Attachment #8525499 - Flags: review?(dkeeler)
(Assignee)

Comment 6

5 years ago
Attachment #8525500 - Flags: review?(dkeeler)
(Assignee)

Comment 7

5 years ago
Fixed a silly typo.
Attachment #8525499 - Attachment is obsolete: true
Attachment #8525499 - Flags: review?(dkeeler)
Attachment #8525501 - Flags: review?(dkeeler)
Masatoshi, unfortunately it might be a while before I can put the proper attention into reviewing these patches. Next week is a short week due to Thanksgiving (US), and the week after is the work week (where, unintuitively, no actual work will get done).
(Assignee)

Comment 9

5 years ago
(In reply to David Keeler (:keeler) [use needinfo?] from comment #8)
> Masatoshi, unfortunately it might be a while before I can put the proper
> attention into reviewing these patches. Next week is a short week due to
> Thanksgiving (US), and the week after is the work week (where,
> unintuitively, no actual work will get done).

Could you review only security.properties quickly? I'll consider uplifting to Aurora it will miss the next merge.
(Assignee)

Comment 10

5 years ago
Posted patch String change only (obsolete) — Splinter Review
To make aurora uplift possible later.
Attachment #8527442 - Flags: review?(dkeeler)
Comment on attachment 8525501 [details] [diff] [review]
Log usage of weak ciphers in the console, v2.01

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

Rather than making changes to nsSecureBrowserUIImpl (which I'm reluctant to do until we have a good understanding of it and perhaps until we've refactored it into a workable state (see bug 832834)), let's take the approach that bug 1068949 did (i.e. check for weak security on channels and log them in nsHttpChannel).
Attachment #8525501 - Flags: review?(dkeeler) → review-
Comment on attachment 8525498 [details] [diff] [review]
Merge lis_broken_security and lis_mixed_security

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

See comment 11.
Attachment #8525498 - Flags: review?(dkeeler) → review-
Comment on attachment 8525500 [details] [diff] [review]
Tests for SSL3/RC4 only servers

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

Unless I'm misunderstanding how this works, this patch is missing some changes. Clearing review for now.

::: build/pgo/server-locations.txt
@@ +235,5 @@
>  https://sha256ee.example.com:443                                  privileged,cert=sha256_end_entity
> +
> +# Hosts for ssl3/rc4 console warning tests
> +https://ssl3.example.com:443        privileged,ssl3
> +https://rc4.example.com:443         privileged,rc4

Don't these changes require changes to ssltunnel.cpp? Did those not get included in this patch?
Attachment #8525500 - Flags: review?(dkeeler)
Comment on attachment 8527442 [details] [diff] [review]
String change only

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

I'm not convinced there's a particular urgency to this such that we should land the UI strings before the actual implementation.
Attachment #8527442 - Flags: review?(dkeeler) → review-
(Assignee)

Comment 15

5 years ago
(In reply to David Keeler (:keeler) [use needinfo?] from comment #13)
> Comment on attachment 8525500 [details] [diff] [review]
> Tests for SSL3/RC4 only servers
> 
> Review of attachment 8525500 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Unless I'm misunderstanding how this works, this patch is missing some
> changes. Clearing review for now.
> 
> ::: build/pgo/server-locations.txt
> @@ +235,5 @@
> >  https://sha256ee.example.com:443                                  privileged,cert=sha256_end_entity
> > +
> > +# Hosts for ssl3/rc4 console warning tests
> > +https://ssl3.example.com:443        privileged,ssl3
> > +https://rc4.example.com:443         privileged,rc4
> 
> Don't these changes require changes to ssltunnel.cpp? Did those not get
> included in this patch?

See bug 1100917.
(Assignee)

Comment 16

5 years ago
All right, I gave up detecting broken subresources this time.
Attachment #8525498 - Attachment is obsolete: true
Attachment #8525501 - Attachment is obsolete: true
Attachment #8527442 - Attachment is obsolete: true
Attachment #8528487 - Flags: review?(dkeeler)
(Assignee)

Comment 17

5 years ago
Removed a test for broken subresources.
See bug 1100917 about ssltunnel changes.
Attachment #8525500 - Attachment is obsolete: true
Attachment #8528488 - Flags: review?(dkeeler)
Comment on attachment 8528487 [details] [diff] [review]
Log usage of weak ciphers in the console, v3

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

Great - this looks a lot more manageable. Still some things to fix, though, so r- for now.

::: dom/locales/en-US/chrome/security/security.properties
@@ +20,5 @@
>  # LOCALIZATION NOTE: Do not translate "allow-scripts", "allow-same-origin", "sandbox" or "iframe"
>  BothAllowScriptsAndSameOriginPresent=An iframe which has both allow-scripts and allow-same-origin for its sandbox attribute can remove its sandboxing.
> +
> +# LOCALIZATION NOTE: Do not translate "SSL 3.0".
> +WeakProtocolVersionWarning=This page was transmitted using SSL 3.0, which is deprecated and insecure.

Maybe "This site uses the protocol SSL 3.0 for encryption, which..."

@@ +22,5 @@
> +
> +# LOCALIZATION NOTE: Do not translate "SSL 3.0".
> +WeakProtocolVersionWarning=This page was transmitted using SSL 3.0, which is deprecated and insecure.
> +# LOCALIZATION NOTE: Do not translate "RC4".
> +WeakCipherSuiteWarning=This page was transmitted using RC4, which is deprecated and insecure.

Maybe "This site uses the cipher RC4 for encryption, which..."

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1224,5 @@
> +    if (securityInfo &&
> +        NS_SUCCEEDED(securityInfo->GetSecurityState(&state)) &&
> +        (state & nsIWebProgressListener::STATE_IS_BROKEN)) {
> +      // Send weak crypto warnings to the web console
> +      if (state & nsIWebProgressListener::STATE_USES_SSL_3) {

nit: I think necko uses 4 spaces for indentation

@@ +1228,5 @@
> +      if (state & nsIWebProgressListener::STATE_USES_SSL_3) {
> +        nsString consoleErrorTag = NS_LITERAL_STRING("WeakProtocolVersionWarning");
> +        nsString consoleErrorCategory = NS_LITERAL_STRING("SSL");
> +        AddSecurityMessage(consoleErrorTag, consoleErrorCategory);
> +      } else if (state & nsIWebProgressListener::STATE_USES_WEAK_CRYPTO) {

It doesn't seem like these should necessarily be mutually-exclusive. Just take out the 'else' here.

@@ +1231,5 @@
> +        AddSecurityMessage(consoleErrorTag, consoleErrorCategory);
> +      } else if (state & nsIWebProgressListener::STATE_USES_WEAK_CRYPTO) {
> +        nsString consoleErrorTag = NS_LITERAL_STRING("WeakCipherSuiteWarning");
> +        nsString consoleErrorCategory = NS_LITERAL_STRING("SSL");
> +        AddSecurityMessage(consoleErrorTag, consoleErrorCategory);

Looks like you'll have to update browser/devtools/webconsole/webconsole.js as well for this to work as expected. See http://hg.mozilla.org/mozilla-central/rev/4d9d00258813

@@ +1258,5 @@
>              }
>          }
>      }
>  
> +    // Send weak crypto warnings to the web console

nit: shouldn't this comment be with the code that actually does this?

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +1270,5 @@
> +    uint32_t state = nsIWebProgressListener::STATE_IS_BROKEN;
> +    if (channelInfo.protocolVersion <= SSL_LIBRARY_VERSION_3_0) {
> +      state |= nsIWebProgressListener::STATE_USES_SSL_3;
> +    } else {
> +      state |= nsIWebProgressListener::STATE_USES_WEAK_CRYPTO;

The peer could be using SSL 3 and RC4, right? It doesn't seem like these should be mutually-exclusive.
To that end, I would restructure this a bit by having three indicator bools: usesWeakProtocol, usesWeakCipher, renegotiationUnsafe. Then, we can do something like this:

uint32_t state;
if (usesWeakProtocol || usesWeakCipher || renegotiationUnsafe) {
  state = nsIWebProgressListener::STATE_IS_BROKEN;
  if (usesWeakProtocol) {
    state |= nsIWebProgressListener::STATE_USES_SSL_3;
  }
  if (usesWeakCipher) {
    state |= nsIWebProgressListener::STATE_USES_WEAK_CRYPTO;
  }
} else {
  state = nsIWebProgressListener::STATE_IS_SECURE |
          nsIWebProgressListener::STATE_SECURE_HIGH;
}
infoObject->SetSecurityState(state);

This should help with bug 883674 as well, because we can add a flag and check for it like we do STATE_USES_SSL3, etc.

::: uriloader/base/nsIWebProgressListener.idl
@@ +262,5 @@
> +    *
> +    * STATE_USES_WEAK_CRYPTO
> +    *   The topmost document uses a weak cipher suite such as RC4.
> +    */
> +  const unsigned long STATE_USES_SSL_3                = 0x01000000;

This is the same value as STATE_IS_RESTORING, which I don't think we can re-use here. Also, I believe these changes require the uuid to be updated on this interface.
Attachment #8528487 - Flags: review?(dkeeler) → review-
(Assignee)

Comment 19

5 years ago
(In reply to David Keeler (:keeler) [use needinfo?] from comment #18)
> Looks like you'll have to update browser/devtools/webconsole/webconsole.js
> as well for this to work as expected. See
> http://hg.mozilla.org/mozilla-central/rev/4d9d00258813

It is needed only if we add a [learn more] link to the message which I didn't for now.

> > +    // Send weak crypto warnings to the web console
> 
> nit: shouldn't this comment be with the code that actually does this?

Oops, I forgot to remove the garbage.

> > +  const unsigned long STATE_USES_SSL_3                = 0x01000000;
> 
> This is the same value as STATE_IS_RESTORING, which I don't think we can
> re-use here. Also, I believe these changes require the uuid to be updated on
> this interface.

Security flags have a different value space from any other types of state. For example, STATE_START/REDIRECTING/TRANSFERRING have the same value as STATE_IS_BROKEN/SECURE/INSECURE, respectively. So the duplication does not matter here.
(But it is indeed confusing. Even a debug code in nsSecureBrowserUIImpl::OnStateChange confuses two values. Definitely this class needs some refactors...)

Adding constants is binary compatible because it does not change the vtable. So the iid bump is not needed.

Other comments are addressed.
Attachment #8528487 - Attachment is obsolete: true
Attachment #8529122 - Flags: review?(dkeeler)
Comment on attachment 8528488 [details] [diff] [review]
Tests for SSL3/RC4 only servers

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

Looks good in general, but I think we should make the control flow a bit more explicit. r- for now for that.

::: browser/devtools/webconsole/test/browser_webconsole_certificate_messages.js
@@ +1,5 @@
>  /* vim:set ts=2 sw=2 sts=2 et: */
>  /* Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  // Tests that the Web Console shows SHA-1 Certificate warnings

nit: update comment

@@ +3,5 @@
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  // Tests that the Web Console shows SHA-1 Certificate warnings
>  
> +const tests = [

If we're going to have an array of tests, it would be better to be more clear about how they're processed. Currently, as far as I can tell, we only implicitly end this test after executing a test-case that doesn't have a 'warning' property. This won't work if we add other test-cases that don't have 'warning' properties. Also, rather than having an index, I think it would be better to do something more like this:

let gWebconsoleTests = [ ... ];
let gCurrentTest;

runTestLoop() {
  gCurrentTest = gWebconsoleTests.shift();
  if (!gCurrentTest) {
    finishTest();
  }

  // run the test-case...

@@ +62,2 @@
>        validatorFn: function() {
> +        return gHud.outputNode.textContent.indexOf(tests[i].warning) > -1;

We should probably also make sure that the possible warnings we're not expecting aren't present.

@@ +70,5 @@
>  function testForNoWarning() {
>    let aOutputNode = gHud.outputNode;
>  
>    waitForSuccess({
> +      name: "SSL warning appropriately missed",

nit: 'SSL warnings appropriately not present' might be a better description

@@ +75,3 @@
>        validatorFn: function() {
>          if (gHud.outputNode.textContent.indexOf(TRIGGER_MSG) > -1) {
> +          for (let test of tests) {

Maybe rather than looping through the array of tests again, we could just have a list of the possible errors that we shouldn't see here (which would also make this a bit more clear).

::: build/pgo/server-locations.txt
@@ +235,5 @@
>  https://sha256ee.example.com:443                                  privileged,cert=sha256_end_entity
> +
> +# Hosts for ssl3/rc4 console warning tests
> +https://ssl3.example.com:443        privileged,ssl3
> +https://rc4.example.com:443         privileged,rc4

How about a ssl3 and rc4 server?
Attachment #8528488 - Flags: review?(dkeeler) → review-
Comment on attachment 8529122 [details] [diff] [review]
Log usage of weak ciphers in the console, v4

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

Looks great! Have a Necko peer take a look, and we should be good to go.

::: uriloader/base/nsIWebProgressListener.idl
@@ +262,5 @@
> +    *
> +    * STATE_USES_WEAK_CRYPTO
> +    *   The topmost document uses a weak cipher suite such as RC4.
> +    */
> +  const unsigned long STATE_USES_SSL_3                = 0x01000000;

Ah, I see what you're saying about these flags. Really they should probably be in nsITransportSecurityInfo, but refactoring that would be outside the scope of this bug.
Attachment #8529122 - Flags: review?(dkeeler) → review+
(Assignee)

Updated

4 years ago
Attachment #8529122 - Flags: review?(mcmanus)
(Assignee)

Comment 22

4 years ago
Attachment #8528488 - Attachment is obsolete: true
Attachment #8531229 - Flags: review?(dkeeler)
(Assignee)

Comment 23

4 years ago
(In reply to David Keeler (:keeler) [use needinfo?] from comment #18)
> @@ +1228,5 @@
> > +      if (state & nsIWebProgressListener::STATE_USES_SSL_3) {
> > +        nsString consoleErrorTag = NS_LITERAL_STRING("WeakProtocolVersionWarning");
> > +        nsString consoleErrorCategory = NS_LITERAL_STRING("SSL");
> > +        AddSecurityMessage(consoleErrorTag, consoleErrorCategory);
> > +      } else if (state & nsIWebProgressListener::STATE_USES_WEAK_CRYPTO) {
> 
> It doesn't seem like these should necessarily be mutually-exclusive. Just
> take out the 'else' here.

I recalled the reason why I made it mutually-exclusive.
I thought we should not warn about the use of RC4 if the protocol version is SSLv3 because RC4 is the least weak cipher available in SSLv3 due to the POODLE attack.
Of course it is only less weak, not secure. It only means that we have no secure cipher suites to continue to use SSLv3. But we already disabled SSLv3 by default. Users will see the warning only if they changed the configuration explicitly. So adding yet another warning (for RC4) looks a bit spammy to me.
What do you think?
Flags: needinfo?(dkeeler)
Comment on attachment 8529122 [details] [diff] [review]
Log usage of weak ciphers in the console, v4

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

I'm curious about gating the warning on IS_BROKEN.. does that mean if someone has configured to allow ssl3 they won't get the warning in its presence? It might be pretty useful in that circumstance
Attachment #8529122 - Flags: review?(mcmanus) → review+
(In reply to Masatoshi Kimura [:emk] from comment #23)
> I recalled the reason why I made it mutually-exclusive.
> I thought we should not warn about the use of RC4 if the protocol version is
> SSLv3 because RC4 is the least weak cipher available in SSLv3 due to the
> POODLE attack.
> Of course it is only less weak, not secure. It only means that we have no
> secure cipher suites to continue to use SSLv3. But we already disabled SSLv3
> by default. Users will see the warning only if they changed the
> configuration explicitly. So adding yet another warning (for RC4) looks a
> bit spammy to me.
> What do you think?

I think the extra warnings are fine in that case. They'll only be seen if someone opens the dev console, in which case we want to get as much attention as possible on how broken that server is. Also, if we add additional, similar warnings, we probably won't want them to be mutually exclusive either.
Flags: needinfo?(dkeeler)
(In reply to Patrick McManus [:mcmanus] from comment #24)
> I'm curious about gating the warning on IS_BROKEN.. does that mean if
> someone has configured to allow ssl3 they won't get the warning in its
> presence? It might be pretty useful in that circumstance

If I understand correctly, as of bug 1093595, we consider ssl3 and rc4 as STATE_IS_BROKEN (similar to if a server doesn't support secure renegotiation and the user has set the pref to expose that). So they should get the warnings if they've enabled ssl3 and connect to a server that uses it.
Comment on attachment 8531229 [details] [diff] [review]
Tests for SSL3/RC4 only servers, v2

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

Great! r=me with comments addressed. A devtools peer should also probably sign off on these changes.

::: browser/devtools/webconsole/test/browser_webconsole_certificate_messages.js
@@ +14,5 @@
> +   name: "RC4",  warning: ["RC4"], nowarning: ["SHA-1", "SSL 3.0"]},
> +  {url: "https://ssl3rc4.example.com/browser/browser/devtools/webconsole/test/test-certificate-messages.html",
> +   name: "SSL3 and RC4",  warning: ["SSL 3.0", "RC4"], nowarning: ["SHA-1"],
> +   pref: [["security.tls.version.min", 0], ["security.enable_ssl3", true]]},
> +  {url: "https://sha256ee.example.com/browser/browser/devtools/webconsole/test/test-certificate-messages.html",

Since all of the URI paths are the same, it might make sense to do something like this:

const TEST_URI_PATH = "/browser/browser/devtools/webconsole/test/test-certificate-messages.html";

and then, for each test:

{ url: "https://sha1ee.example.com" + TEST_PATH, ...

@@ +60,5 @@
>    let aOutputNode = gHud.outputNode;
>  
>    waitForSuccess({
> +      name: gCurrentTest.warning ? gCurrentTest.name + " warning displayed successfully"
> +                                 : "SSL warnings appropriately not present",

Why not just define a complete name for each test case and use it here?

@@ +65,3 @@
>        validatorFn: function() {
> +        if (gHud.outputNode.textContent.indexOf(TRIGGER_MSG) >= 0) {
> +          for (let warning of (gCurrentTest.warning || [])) {

Instead of doing (gCurrentTest.warning || []), I think it would be clearer to define warning for each test (even if it's just []).
Attachment #8531229 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 28

4 years ago
Attachment #8531229 - Attachment is obsolete: true
Attachment #8533680 - Flags: review?(past)
Attachment #8533680 - Flags: review?(past) → review+
This should have landed with a nsIWebProgressListener UUID bump due to the IDL change.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb903f13f215
Unfortunately, the UUID update didn't fix the Android 2.3 permafail in test_csp_allow_https_schemes.html that started on the original push. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dce2f7ec002e

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4467596&repo=mozilla-inbound
Please be sure to include the UUID bump when this relands. You can do it easily by running |mach update-uuids nsIWebProgressListener| from the root of your tree.
(Assignee)

Comment 34

4 years ago
See comment #19. Adding constants doesn't require a uuid bump in my understanding. Was the policy changed?
(Assignee)

Comment 35

4 years ago
Looks like I hit bug 1109310.
Depends on: 1109310
Comment on attachment 8535988 [details] [diff] [review]
Workaround for old ssltunnel from hostutils

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

Thanks for working around this.  When we do update hostutils, I would like to remove this- could you file a bug to remove this patch once we fix bug 1109310 and then make it blocked by bug 1109310?
Attachment #8535988 - Flags: review?(jmaher) → review+
(Assignee)

Comment 39

4 years ago
(In reply to Joel Maher (:jmaher) from comment #37)
> When we do update hostutils, I would like
> to remove this- could you file a bug to remove this patch once we fix bug
> 1109310 and then make it blocked by bug 1109310?

Filed bug 1111188.
(Assignee)

Updated

4 years ago
Depends on: 1111908
(Assignee)

Updated

4 years ago
No longer depends on: 1109310

Updated

4 years ago
See Also: → 1142157
Do you know why these warnigns are showing up in the browser console but not the webconsole?
(Assignee)

Comment 44

4 years ago
I saw warnings in both Browser Console and Web Console.
Note: if the page is loaded from the cache, no warnings will be displayed.
(Assignee)

Comment 45

4 years ago
If you enable e10s, that's bug 1096908.

Comment 46

4 years ago
For the sake of anyone who might be testing this feature in the future: Tanvi and I discovered that the message doesn't appear in private windows, or if Permanent Private Browsing is turned on, but works fine in normal non-private windows.
(In reply to Cykesiopka from comment #46)
> For the sake of anyone who might be testing this feature in the future:
> Tanvi and I discovered that the message doesn't appear in private windows,
> or if Permanent Private Browsing is turned on, but works fine in normal
> non-private windows.

Filed bug 1143198.
You need to log in before you can comment on or make changes to this bug.