Bug 377496 - Improve auth dialog blocking heuristics. r=MattN
authorJohann Hofmann <jhofmann@mozilla.com>
Sun, 03 Feb 2019 19:42:19 +0000
changeset 456607 d05f776eb8408238fba4a59e24c6189c45653646
parent 456606 be2f62a541cb4e5c0472878f3370092d1a2bbcae
child 456608 cf32e70c234f4ce2361bb8e01a08ccd9a8ea24c6
push id77362
push userjhofmann@mozilla.com
push dateSun, 03 Feb 2019 19:44:57 +0000
treeherderautoland@cf32e70c234f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs377496, 1312243
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 377496 - Improve auth dialog blocking heuristics. r=MattN The fix in bug 1312243 introduced a maximum of three consecutive cancelations (controlled by a pref) that a user could perform until Firefox would prevent the page from showing more dialogs. This, in my opinion, is a great idea. The implementation, however, has a major fallacy: It checks the inner window id in the well-meaning attempt to find user navigation or reloads and clears its internal counter when that window id changes. Unfortunately this also clears the counter on non-user-initiated navigations and reloads. I believe that the true intention of the patch was to cancel the auth dialog after 3 attempts, except if: - The user reloads the page on their own terms - The user navigates to a different site on their own Which is what I plan to implement, using the same pattern we applied to implement temporarily blocked site permissions: - Temporarily store basic auth counter state on the browser object, as a map from baseDomain (eTLD+1) to number of cancellations - Reset this state only on user initiated reload - Reset the counter for a domain if the user has entered login data into the dialog and submitted This would mitigate the DOS issue while hopefully not breaking any sites that rely on basic auth. Differential Revision: https://phabricator.services.mozilla.com/D18019
browser/base/content/browser.js
browser/base/content/tabbrowser.js
browser/base/content/urlbarBindings.xml
toolkit/components/passwordmgr/nsLoginManagerPrompter.js
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -3270,16 +3270,18 @@ function BrowserReloadWithFlags(reloadFl
     maybeRecordAbandonmentTelemetry(tab, "reload");
   }
 
   // Reset temporary permissions on the remaining tabs to reload.
   // This is done here because we only want to reset
   // permissions on user reload.
   for (let tab of unchangedRemoteness) {
     SitePermissions.clearTemporaryPermissions(tab.linkedBrowser);
+    // Also reset DOS mitigations for the basic auth prompt on reload.
+    delete tab.linkedBrowser.canceledAuthenticationPromptCounter;
   }
   PanelMultiView.hidePopup(gIdentityHandler._identityPopup);
 
 
   let handlingUserInput = window.windowUtils.isHandlingUserInput;
 
   for (let tab of unchangedRemoteness) {
     if (tab.linkedPanel) {
--- a/browser/base/content/tabbrowser.js
+++ b/browser/base/content/tabbrowser.js
@@ -3520,16 +3520,18 @@ window._gBrowser = {
     }
   },
 
   reloadTab(aTab) {
     let browser = this.getBrowserForTab(aTab);
     // Reset temporary permissions on the current tab. This is done here
     // because we only want to reset permissions on user reload.
     SitePermissions.clearTemporaryPermissions(browser);
+    // Also reset DOS mitigations for the basic auth prompt on reload.
+    delete browser.canceledAuthenticationPromptCounter;
     PanelMultiView.hidePopup(gIdentityHandler._identityPopup);
     browser.reload();
   },
 
   addProgressListener(aListener) {
     if (arguments.length != 1) {
       Cu.reportError("gBrowser.addProgressListener was " +
         "called with a second argument, " +
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -845,16 +845,23 @@ file, You can obtain one at http://mozil
           try {
             UrlbarUtils.addToUrlbarHistory(url, window);
           } catch (ex) {
             // Things may go wrong when adding url to session history,
             // but don't let that interfere with the loading of the url.
             Cu.reportError(ex);
           }
 
+          // Reset DOS mitigations for the basic auth prompt.
+          // TODO: When bug 1498553 is resolved, we should be able to
+          // remove the !triggeringPrincipal condition here.
+          if (!triggeringPrincipal || triggeringPrincipal.isSystemPrincipal) {
+            delete browser.canceledAuthenticationPromptCounter;
+          }
+
           let params = {
             postData,
             allowThirdPartyFixup: true,
             triggeringPrincipal,
           };
           if (openUILinkWhere == "current") {
             params.targetBrowser = browser;
             params.indicateErrorPageLoad = true;
--- a/toolkit/components/passwordmgr/nsLoginManagerPrompter.js
+++ b/toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ -91,27 +91,35 @@ LoginManagerPromptFactory.prototype = {
       let httpHostname = hostname.replace(/^https:\/\//, "http://");
       hasLogins = (Services.logins.countLogins(httpHostname, null, httpRealm) > 0);
     }
     if (hasLogins && Services.logins.uiBusy) {
       this.log("_doAsyncPrompt:run bypassed, master password UI busy");
       return;
     }
 
-    // Allow only a limited number of authentication dialogs when they are all
-    // canceled by the user.
-    var cancelationCounter = (prompter._browser && prompter._browser.canceledAuthenticationPromptCounter) || { count: 0, id: 0 };
-    if (prompt.channel) {
-      var httpChannel = prompt.channel.QueryInterface(Ci.nsIHttpChannel);
-      if (httpChannel) {
-        var windowId = httpChannel.topLevelContentWindowId;
-        if (windowId != cancelationCounter.id) {
-          // window has been reloaded or navigated, reset the counter
-          cancelationCounter = { count: 0, id: windowId };
-        }
+    // Set up a counter for ensuring that the basic auth prompt can not
+    // be abused for DOS-style attacks. With this counter, each eTLD+1
+    // per browser will get a limited number of times a user can
+    // cancel the prompt until we stop showing it.
+    let browser = prompter._browser;
+    let baseDomain = null;
+    if (browser) {
+      try {
+        baseDomain = Services.eTLD.getBaseDomainFromHost(hostname);
+      } catch (e) {
+        baseDomain = hostname;
+      }
+
+      if (!browser.canceledAuthenticationPromptCounter) {
+        browser.canceledAuthenticationPromptCounter = {};
+      }
+
+      if (!browser.canceledAuthenticationPromptCounter[baseDomain]) {
+        browser.canceledAuthenticationPromptCounter[baseDomain] = 0;
       }
     }
 
     var self = this;
 
     var runnable = {
       cancel: false,
       run() {
@@ -131,23 +139,24 @@ LoginManagerPromptFactory.prototype = {
                              "_doAsyncPrompt:run: " + e + "\n");
             }
           }
 
           delete self._asyncPrompts[hashKey];
           prompt.inProgress = false;
           self._asyncPromptInProgress = false;
 
-          if (ok) {
-            cancelationCounter.count = 0;
-          } else {
-            cancelationCounter.count++;
-          }
-          if (prompter._browser) {
-            prompter._browser.canceledAuthenticationPromptCounter = cancelationCounter;
+          if (browser) {
+            // Reset the counter state if the user replied to a prompt and actually
+            // tried to login (vs. simply clicking any button to get out).
+            if (ok && (prompt.authInfo.username || prompt.authInfo.password)) {
+              browser.canceledAuthenticationPromptCounter[baseDomain] = 0;
+            } else {
+              browser.canceledAuthenticationPromptCounter[baseDomain] += 1;
+            }
           }
         }
 
         for (var consumer of prompt.consumers) {
           if (!consumer.callback) {
           // Not having a callback means that consumer didn't provide it
           // or canceled the notification
             continue;
@@ -163,18 +172,19 @@ LoginManagerPromptFactory.prototype = {
           } catch (e) { /* Throw away exceptions caused by callback */ }
         }
         self._doAsyncPrompt();
       },
     };
 
     var cancelDialogLimit = Services.prefs.getIntPref("prompts.authentication_dialog_abuse_limit");
 
+    let cancelationCounter = browser.canceledAuthenticationPromptCounter[baseDomain];
     this.log("cancelationCounter =", cancelationCounter);
-    if (cancelDialogLimit && cancelationCounter.count >= cancelDialogLimit) {
+    if (cancelDialogLimit && cancelationCounter >= cancelDialogLimit) {
       this.log("Blocking auth dialog, due to exceeding dialog bloat limit");
       delete this._asyncPrompts[hashKey];
 
       // just make the runnable cancel all consumers
       runnable.cancel = true;
     } else {
       this._asyncPromptInProgress = true;
       prompt.inProgress = true;