1219088 - Add a notification bar to revoke override for RC4/Cert errors

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Masatoshi Kimura [:emk]]

I didn't implement this in bug 1202489.

Comment 1

[Masatoshi Kimura [:emk]]

Ah, bug 1207137.

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: String changes for infobar

Probably I have no time to implement this before the next merge. Let's land strings first so that we can uplift later.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6b3c99e54177

Comment 4

[Masatoshi Kimura [:emk]]

Missed the train, but we can just remove the link.

Comment 5

[Masatoshi Kimura [:emk]]

Comment on attachment 8679989 [details] [diff] [review]
String changes for infobar

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Users will not be warned enough when sites are using weak crypto.
[Describe test coverage new/current, TreeHerder]: Build locally, string change only.
[Risks and why]: Very low, string change only.
[String/UUID change made/needed]: string change (only) missed due to the merge day shift.

Comment 6

[Axel Hecht [:Pike]]

Similar question as in bug 1219109 comment 9: What's the ETA for the actual patch, and what's the risk for the actual patch using the strings here?

Comment 7

[Masatoshi Kimura [:emk]]

Comment on attachment 8679989 [details] [diff] [review]
String changes for infobar

Canceling the uplift request until the string is finalized.

Comment 8

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/5db171f726b2

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: Show infobar when RC4 override is enabled

This patch includes string changes.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8682282 [details] [diff] [review]
Show infobar when RC4 override is enabled

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

I don't think I'm the best reviewer for this, one note though

::: browser/base/content/browser.js
@@ +7286,5 @@
>      this._identityIconLabel.parentNode.style.direction = icon_labels_dir;
>      // Hide completely if the organization label is empty
>      this._identityIconLabel.parentNode.collapsed = icon_label ? false : true;
>    },
> + 

Nit: trailing whitespace

@@ +7293,5 @@
> +   */
> +  showWeakCryptoInfoBar() {
> +    if (!this._uriHasHost || !this._isBroken || !this._sslStatus.cipherName ||
> +        this._sslStatus.cipherName.indexOf("_RC4_") < 0) {
> +      return;

Should an existing notification get removed if a security change happens that doesn't fit this criteria (i.e. we've navigated to a new page that doesn't need to notification)?

Comment 11

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

Comment on attachment 8682282 [details] [diff] [review]
Show infobar when RC4 override is enabled

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

::: browser/base/content/browser.js
@@ +7322,5 @@
> +      callback: function (aNotification, aButton) {
> +        try {
> +          let weakCryptoOverride = Cc["@mozilla.org/security/weakcryptooverride;1"]
> +                                     .getService(Ci.nsIWeakCryptoOverride);
> +          Cu.reportError(host + ":" + port);

Is this reporting necessary? I already see the more elaborate error message in the Browser Console from TransportSecurityInfo and this doesn't add any more value.

@@ +7325,5 @@
> +                                     .getService(Ci.nsIWeakCryptoOverride);
> +          Cu.reportError(host + ":" + port);
> +          weakCryptoOverride.removeWeakCryptoOverride(host, port,
> +            PrivateBrowsingUtils.isBrowserPrivate(gBrowser.selectedBrowser));
> +          BrowserReloadWithFlags(nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE);

This callback causes an ssl_error_illegal_parameter_alert error to appear first and then I have to click "Try Again" to actually visit the page with the override properly removed. Can we tweak the reload action somehow to avoid this?

@@ +7333,5 @@
> +      }
> +    }];
> +
> +    const priority = notificationBox.PRIORITY_WARNING_MEDIUM;
> +    notificationBox.appendNotification(message, "popup-blocked", null,

You meant to use "weak-crypto" here, not "popup-blocked".

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: Clear the session cache when a weak crypto override is revoked

Comment 13

[Masatoshi Kimura [:emk]]

(In reply to Panos Astithas [:past] from comment #11)
> ::: browser/base/content/browser.js
> @@ +7322,5 @@
> > +      callback: function (aNotification, aButton) {
> > +        try {
> > +          let weakCryptoOverride = Cc["@mozilla.org/security/weakcryptooverride;1"]
> > +                                     .getService(Ci.nsIWeakCryptoOverride);
> > +          Cu.reportError(host + ":" + port);
> 
> Is this reporting necessary? I already see the more elaborate error message
> in the Browser Console from TransportSecurityInfo and this doesn't add any
> more value.

Vestigial debug output. Removed.

> @@ +7325,5 @@
> > +                                     .getService(Ci.nsIWeakCryptoOverride);
> > +          Cu.reportError(host + ":" + port);
> > +          weakCryptoOverride.removeWeakCryptoOverride(host, port,
> > +            PrivateBrowsingUtils.isBrowserPrivate(gBrowser.selectedBrowser));
> > +          BrowserReloadWithFlags(nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE);
> 
> This callback causes an ssl_error_illegal_parameter_alert error to appear
> first and then I have to click "Try Again" to actually visit the page with
> the override properly removed. Can we tweak the reload action somehow to
> avoid this?

Oops, I forgot to attach a PSM change. No workaround is necessary with the PSM change.

> @@ +7333,5 @@
> > +      }
> > +    }];
> > +
> > +    const priority = notificationBox.PRIORITY_WARNING_MEDIUM;
> > +    notificationBox.appendNotification(message, "popup-blocked", null,
> 
> You meant to use "weak-crypto" here, not "popup-blocked".

Fixed.

Comment 14

[Masatoshi Kimura [:emk]]

Attachment: Show infobar when RC4 override is enabled, v2

Comment 15

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

Comment on attachment 8685385 [details] [diff] [review]
Show infobar when RC4 override is enabled, v2

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

This looks great, thanks!

::: browser/base/content/browser.js
@@ +7286,5 @@
>      this._identityIconLabel.parentNode.style.direction = icon_labels_dir;
>      // Hide completely if the organization label is empty
>      this._identityIconLabel.parentNode.collapsed = icon_label ? false : true;
>    },
> + 

Please remove this trailing whitespace.

Comment 16

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

Comment on attachment 8685384 [details] [diff] [review]
Clear the session cache when a weak crypto override is revoked

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

::: security/manager/ssl/WeakCryptoOverride.cpp
@@ +59,5 @@
>    const nsPromiseFlatCString& host = PromiseFlatCString(aHostName);
>    sharedState->IOLayerHelpers().removeInsecureFallbackSite(host, aPort);
>  
> +  // SharedSSLState is available only if NSS is initialized.
> +  MOZ_ASSERT(NSS_IsInitialized());

This seems unnecessary.

Comment 17

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cc54fd43bd7

Comment 18

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/fx-team/rev/fc4a5bfc904e3fe1e35804de8663e5bb26aad1b4
Bug 1219088 - Clear the session cache when a weak crypto override is revoked. r=keeler

https://hg.mozilla.org/integration/fx-team/rev/52dc9d73dcd3fc033ea01137377c82fd1240a4f9
Bug 1219088 - Show infobar when RC4 override is enabled. r=past

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fc4a5bfc904e
https://hg.mozilla.org/mozilla-central/rev/52dc9d73dcd3

Comment 20

[Paul Silaghi, QA [:pauly]]

I tested this on https://rc4.badssl.com/.
1. While the notification bar is displayed, reloading the page makes it disappear.
2. Even if I dismiss the notification bar, it will still appear if I load the page in a new tab.
Are these expected?

Comment 21

[Masatoshi Kimura [:emk]]

No, filing a follow-up bug. (But I couldn't reproduce the latter.)

Comment 22

[Paul Silaghi, QA [:pauly]]

Marking as verified fixed on FF 45.0a1 (2015-11-16) Win 7, Ubuntu 14.04, OS X 10.11, considering the follow-up bug 1225299.