Closed Bug 1219088 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 45
Iteration:
45.1 - Nov 16
Tracking Status
firefox45 --- verified

People

(Reporter: emk, Assigned: emk)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxprivacy])

Attachments

(2 files, 2 obsolete files)

I didn't implement this in bug 1202489.
Ah, bug 1207137.
Attached patch String changes for infobar (obsolete) — Splinter Review
Probably I have no time to implement this before the next merge. Let's land strings first so that we can uplift later.
Attachment #8679989 - Flags: review?(ttaubert)
Attachment #8679989 - Flags: review?(ttaubert) → review+
Keywords: leave-open
Missed the train, but we can just remove the link.
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.
Attachment #8679989 - Flags: approval-mozilla-aurora?
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?
Assignee: nobody → VYV03354
Comment on attachment 8679989 [details] [diff] [review]
String changes for infobar

Canceling the uplift request until the string is finalized.
Attachment #8679989 - Flags: approval-mozilla-aurora?
Whiteboard: [fxprivacy][triage]
Flags: qe-verify?
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Flags: qe-verify? → qe-verify+
This patch includes string changes.
Attachment #8679989 - Attachment is obsolete: true
Attachment #8682282 - Flags: review?(ttaubert)
Attachment #8682282 - Flags: review?(ttaubert)
Attachment #8682282 - Flags: review?(paolo.mozmail)
Attachment #8682282 - Flags: review?(bgrinstead)
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)?
Attachment #8682282 - Flags: review?(bgrinstead)
Attachment #8682282 - Flags: review?(paolo.mozmail) → review?(past)
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".
Attachment #8682282 - Flags: review?(past)
(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.
Attachment #8682282 - Attachment is obsolete: true
Attachment #8685385 - Flags: review?(past)
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.
Attachment #8685385 - Flags: review?(past) → review+
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.
Attachment #8685384 - Flags: review?(dkeeler) → review+
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
https://hg.mozilla.org/mozilla-central/rev/fc4a5bfc904e
https://hg.mozilla.org/mozilla-central/rev/52dc9d73dcd3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Iteration: --- → 45.1 - Nov 16
Priority: -- → P1
QA Contact: paul.silaghi
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?
Flags: needinfo?(VYV03354)
No, filing a follow-up bug. (But I couldn't reproduce the latter.)
Flags: needinfo?(VYV03354)
Depends on: 1225299
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.