Closed
Bug 1092835
Opened 10 years ago
Closed 10 years ago
Log usage of weak ciphers in the console
Categories
(Core :: Security: PSM, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: annevk, Assigned: emk)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(3 files, 9 obsolete files)
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 |
We should start telling sites this is a bad idea.
Updated•10 years ago
|
Keywords: dev-doc-needed
Comment 1•10 years ago
|
||
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•10 years ago
|
||
I recycled STATE_SECURE_LOW and lis_broken_security to indicate that the server uses a weak encryption.
Assignee | ||
Updated•10 years ago
|
Component: Developer Tools: Console → Security: PSM
Product: Firefox → Core
Comment 3•10 years ago
|
||
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 | ||
Comment 4•10 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•10 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•10 years ago
|
||
Attachment #8525500 -
Flags: review?(dkeeler)
Assignee | ||
Comment 7•10 years ago
|
||
Fixed a silly typo.
Attachment #8525499 -
Attachment is obsolete: true
Attachment #8525499 -
Flags: review?(dkeeler)
Attachment #8525501 -
Flags: review?(dkeeler)
Comment 8•10 years ago
|
||
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•10 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•10 years ago
|
||
To make aurora uplift possible later.
Attachment #8527442 -
Flags: review?(dkeeler)
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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 13•10 years ago
|
||
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 14•10 years ago
|
||
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•10 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•10 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•10 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 18•10 years ago
|
||
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•10 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 20•10 years ago
|
||
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 21•10 years ago
|
||
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•10 years ago
|
Attachment #8529122 -
Flags: review?(mcmanus)
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8528488 -
Attachment is obsolete: true
Attachment #8531229 -
Flags: review?(dkeeler)
Assignee | ||
Comment 23•10 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 24•10 years ago
|
||
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+
Comment 25•10 years ago
|
||
(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)
Comment 26•10 years ago
|
||
(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 27•10 years ago
|
||
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•10 years ago
|
||
Attachment #8531229 -
Attachment is obsolete: true
Attachment #8533680 -
Flags: review?(past)
Updated•10 years ago
|
Attachment #8533680 -
Flags: review?(past) → review+
Assignee | ||
Comment 29•10 years ago
|
||
Assignee | ||
Comment 30•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/36d257ead3da
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c5c712698e4
Flags: in-testsuite+
Comment 31•10 years ago
|
||
This should have landed with a nsIWebProgressListener UUID bump due to the IDL change.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb903f13f215
Comment 32•10 years ago
|
||
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
Comment 33•10 years ago
|
||
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•10 years ago
|
||
See comment #19. Adding constants doesn't require a uuid bump in my understanding. Was the policy changed?
Assignee | ||
Comment 36•10 years ago
|
||
android-api-9 is green on try with this patch.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b5df1aaf8bee
Attachment #8535988 -
Flags: review?(jmaher)
Comment 37•10 years ago
|
||
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 38•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/beeec226d615
https://hg.mozilla.org/integration/mozilla-inbound/rev/d25c1449cdbf
https://hg.mozilla.org/integration/mozilla-inbound/rev/01e69340d971
The constant-only change should not require the iid bump because it is binary compatible.
Assignee | ||
Comment 39•10 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.
Comment 40•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/beeec226d615
https://hg.mozilla.org/mozilla-central/rev/d25c1449cdbf
https://hg.mozilla.org/mozilla-central/rev/01e69340d971
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 41•10 years ago
|
||
Added a comment: https://developer.mozilla.org/en-US/Firefox/Releases/37#Security
Keywords: dev-doc-needed → dev-doc-complete
Comment 42•10 years ago
|
||
Added to the compat doc: https://developer.mozilla.org/en-US/Firefox/Releases/37/Site_Compatibility
Comment 43•10 years ago
|
||
Do you know why these warnigns are showing up in the browser console but not the webconsole?
Assignee | ||
Comment 44•10 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•10 years ago
|
||
If you enable e10s, that's bug 1096908.
Comment 46•10 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.
Comment 47•10 years ago
|
||
(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.
Description
•