1312243 - (CVE-2017-5419) Trap site uses repeated http auth prompts to keep the user on the page

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Chris]

Attachment: browserhijacking.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20161019084923

Steps to reproduce:

went to a website


Actual results:

browser was redirected multiple times. eventually stopping on a "data" url that pretended to be microsoft and claiming my computer was compromised. What was different to other hijacks was this loaded a 403 password modal dialogue and could not be cleared. The only way to close the rogue site was to shut down the browser from task manager. 


Expected results:

data urls should not be able to render pages and should not be able to trigger dialogues especially modal dialogue boxes that prevent navigating away from the site

Comment 1

[:Gijs (he/him)]

This isn't related to the data: URL (you could cause multiple http auth prompts from a 'normal' webpage too), and we would likely not be able to break data: URLs altogether anyway. The problem is the repeated prompts making it impossible to close the site.


Patrick, is there a way we could limit these prompts if making them tab-modal isn't feasible in the near future?

Comment 2

[Honza Bambas (:mayhemer)]

Is bug 1312836 related?

Comment 3

[:Gijs (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> Is bug 1312836 related?

Yeah, that looks like it's the same. I'll dupe. The URL and attached data URIs from bug 1312836 look quite useful though.

Comment 5

[:Gijs (he/him)]

Apparently bug 647010 covers disabling these prompts for non-toplevel loads, which would also help.

Honza, can you think of anything else that would (help)?

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to :Gijs Kruitbosch from comment #5)
> Apparently bug 647010 covers disabling these prompts for non-toplevel loads,
> which would also help.

Not sure what the state of that bug is.  Dragana?  Just quickly - what is status of that bug?

> 
> Honza, can you think of anything else that would (help)?

The first idea that popped to my mind was to queue the prompts, but recently I touched that code and thought we were already was doing that.  Double checking if that is broken or not would be good (LoginManagerPrompter.js).

Bad thing is that just queuing will not get rid of all the modal dialogs flood.  Something like "don't ask me for auth from this page" check box, similar what we have for MsgBox would do (a UI change).  Only refresh of the page by the user would allow it again then.

I'll try to find time to think of something simpler tho.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to Honza Bambas (:mayhemer) from comment #6)
> I'll try to find time to think of something simpler tho.

Queue the prompts, and have a longer and longer time in between subsequent prompts, ramping up quickly?  After some number of queued prompts, just auto-fail auth requests for that page for a while.  It's a bit opaque, but maybe we can mitigate it with a doorhanger popup that says "This page has created too many auth prompts in a short period of time, we're going to disable it until you reload"?  (i18n impact, but still)

Comment 8

[:Gijs (he/him)]

Could we block prompts for domain X immediately if the user clicks cancel (or OK with no username/password) ? Or do these pages use different domains?

Comment 9

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > Apparently bug 647010 covers disabling these prompts for non-toplevel loads,
> > which would also help.
> 
> Not sure what the state of that bug is.  Dragana?  Just quickly - what is
> status of that bug?
> 

Bug 647010 introduced a pref to disable a cross-origin authentication but usage of such is to high to disable it:
https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-10-26&keys=__none__!__none__!__none__&max_channel_version=nightly%252F52&measure=HTTP_AUTH_DIALOG_STATS&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-09-19&table=0&trim=1&use_submission_date=0

Comment 10

[Dragana Damjanovic [:dragana]]

(In reply to :Gijs Kruitbosch from comment #8)
> Could we block prompts for domain X immediately if the user clicks cancel
> (or OK with no username/password) ? Or do these pages use different domains?

or block it after second or third cancel. In case user make a mistake canceling.

Comment 11

[:Gijs (he/him)]

(In reply to Dragana Damjanovic [:dragana] from comment #10)
> (In reply to :Gijs Kruitbosch from comment #8)
> > Could we block prompts for domain X immediately if the user clicks cancel
> > (or OK with no username/password) ? Or do these pages use different domains?
> 
> or block it after second or third cancel. In case user make a mistake
> canceling.

In a 'normal' case where a website needs resources, cancel (or not providing correct credentials) will break that resource, likely necessitating a reload anyway. I don't think we need to wait for the second/third cancel - if the user makes a mistake, they'll want to reload the page anyway.

Comment 12

[Honza Bambas (:mayhemer)]

Boris, I'd like to ask for a little help here.

I want to track the number of authentication prompts a page invokes (via a subresource or iframe) and user cancels them as mitigation to the bloat this bug deals with.  

In nsLoginManagerPrompter.js (note this is js, so no access to nsPI* interfaces) I only have access to the outer window (nsDocShell->mScriptGlobal->AsOuter(), which is nsGlobalWindow AFAIK).  I'm trying to implement a counter nsLoginManagerPrompter could access and which simply resets when top level navigation happens.  Sounds like the point of reset is nsGlobalWindow::SetNewDocument.  But here I may need a new property exposed to chrome on global window, not sure it's the way to go.

Other option is this: I can reach the browser's chrome document element and set attributes on it (keep the counter there).  But I don't know how to reset this attribute easily on navigation/reload.  Apparently, browser doc element is not the one that resets in nsGlobalWindow::SetNewDocument (mDoc).

So, the question is, do you know about something that is reachable in chrome js from the outer window, capable of storing arbitrary data (the counter) and is reset on navigation/reload?  :)

Thanks.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> So, the question is, do you know about something that is reachable in chrome js from the outer window

Is your chrome js running in the content process or the chrome process?

Because window.document seems like a totally reasonable place to store this information.  You can ether use an expando (over Xrays) with a suitably chosen symbol name or string name of some sort, or a weakmap with the document as the key.  But obviously that's only a good idea in the content process.  Of course poking at the docshell in the chrome process is bad too, so I really hope you're in the content process.

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> > So, the question is, do you know about something that is reachable in chrome js from the outer window
> 
> Is your chrome js running in the content process or the chrome process?

The login manager solely runs in chrome.  To be more precise, it's queried at TabParent::GetAuthPrompt https://dxr.mozilla.org/mozilla-central/rev/3e73fd638e687a4d7f46613586e5156b8e2af846/dom/ipc/TabParent.cpp#2424

> 
> Because window.document seems like a totally reasonable place to store this
> information.  You can ether use an expando (over Xrays) with a suitably
> chosen symbol name or string name of some sort, or a weakmap with the
> document as the key.  But obviously that's only a good idea in the content
> process.  Of course poking at the docshell in the chrome process is bad too,
> so I really hope you're in the content process.

No, I'm not on the content process :(  I can access the tab's window.document, tho.  Only problem is that it doesn't reset (drop) my added custom attribute when reload/navigation happens in the tab.

Note that the counter must not be accessible to content.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> I can access the tab's window.document, tho

Ok, but then that's a CPOW if the window is the content window, so we don't want to be doing that.

But if you're in TabParent::GetAuthPrompt then none of that is relevant: the "window" variable there is the browser window, not the webpage window, and I expect that's what gets passed to nsLoginManagerPrompter.js.  You can double-check what its .document.URL is, but I expect it's chrome://whatever/browser.xul.

So we need to either implement this counter down lower where we still know which exact content window (or I guess toplevel content window is the part we really care about) is doing the prompting or we need to propagate the information about the toplevel content window that's doing the prompting up to nsLoginManagerPrompter.js.

Comment 16

[Honza Bambas (:mayhemer)]

Thanks Boris, you are right, it's browser.xul.  Hence, I don't think this is going to be that easy.  

I could store the counter there but I didn't find a spot where I could reset it on reload/navigation.  My knowledge of remote browser is thin.

Comment 17

[Boris Zbarsky [:bzbarsky]]

The right answer is to either propagate the inner window id of the toplevel window along with requests (so you can key things off it), or to maintain the counter and its checks in the content process, before you ever get into that code...

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: v1 (carry inner window id to httpchannelparent, limit cancled auth dialogs)

two parts

http:
- carry top-document inner window id to http channel parent
- it has to be done manually, since for document loads the id is not present on load info (weird or expected?) and when I qi/gi it from the child tab's document (yes, it works!) I don't want to modify the existing load info
- in non-e10s we do this lazily in http channel base

pwd manager:
- pwd manager then tracks the number of auth dialogs canceled in a row withing a single page and when there are 3 auth dialogs canceled by the user, it will block any auth dialog within coming from context of the page (iframes/xhr/resources)
- the counter is stored on the parent browser document (broswer.xul) as an attribute
- the limit is preferable and can be turned off
- works in e10s and non-e10s

Justin, please check the pwd manager parts, Patrick, please check the http/ipc changes.

Boris, thanks for guidance!


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

Comment 19

[Boris Zbarsky [:bzbarsky]]

> the id is not present on load info

That's odd in general, but can probably happen in some cases.

> and when I qi/gi it from the child tab's document

That is ok, but that's not what happens in the non-e10s case in your patch, right?  That case won't do the right thing, because it will get different ids for a page and its subframes.  So a page can just create multiple iframes to bypass this.

I think you want to get the nsILoadContext, then get its topWindow (this gives you an outer window), then get the current inner's ID from it, or something along those lines.

Also, this bit:

>+    if (document) {
>+      nsPIDOMWindowOuter* outer = document->GetWindow();
>+      nsCOMPtr<nsIInterfaceRequestor> windowIR = do_QueryInterface(outer);
>+      if (windowIR) {
>+        nsCOMPtr<nsIDOMWindowUtils> windowUtils = do_GetInterface(windowIR);
>+        if (windowUtils) {
>+          windowUtils->GetCurrentInnerWindowID(&contentWindowId);
>+        }
>+      }

should instead be:

  if (document) {
    contentWindowId = document->InnerWindowID();
  }

Comment 20

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #19)
> > the id is not present on load info
> 
> That's odd in general, but can probably happen in some cases.
> 
> > and when I qi/gi it from the child tab's document
> 
> That is ok, but that's not what happens in the non-e10s case in your patch,
> right?  

I copied the code from somewhere else when looking around how to get the inner window id when having an outer window in hands.

I will update the patch tomorrow according your suggestions, thanks!

Comment 21

[Boris Zbarsky [:bzbarsky]]

> when looking around how to get the inner window id when having an outer window in hands

Sure, but the point is you want the "top" outer window, not the one the channel is in.

Comment 22

[Honza Bambas (:mayhemer)]

Attachment: v2 (carry inner window id to httpchannelparent, limit cancled auth dialogs)

Updated according Boris' comments (hopefully correctly) + few more fixes.  Thanks again, Boris :)

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 8807535 [details] [diff] [review]
v2 (carry inner window id to httpchannelparent, limit cancled auth dialogs)

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

Comment 24

[Justin Dolske [:Dolske]]

(In reply to :Gijs Kruitbosch from comment #5)

> Honza, can you think of anything else that would (help)?

Just FTR -- although I don't think it will help greatly -- but bug 1230462 made cross-origin auth prompts no longer show the Realm info ("The site says: xxx" stuff in attachment 8803672 [details]). Assuming that sticks, it would be interesting to see if we can just remove it from all HTTP auth prompts. This doesn't help directly with trapping, but makes it less useful when the site can't put any text in the dialog. (We removed the site-specified text from onbeforeunload prompts too, for the same reason.)

Comment 25

[Justin Dolske [:Dolske]]

Comment on attachment 8807535 [details] [diff] [review]
v2 (carry inner window id to httpchannelparent, limit cancled auth dialogs)

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

r- but on the right track. :)

::: modules/libpref/init/all.js
@@ +5522,5 @@
> +// When a user cancels this number of authentication dialogs coming from
> +// a sigle web page in a row, all following authentication dialogs will
> +// be blocked (automatically canceled) for that page. The counter resets
> +// when the page is reloaded. To turn this feature off, just set the limit to 0.
> +pref("general.authentication-dialog-bloat-limit", 3);

At risk of bikeshedding a little:

1) How about just making this a simple boolean? I think it's pretty unlike that this is something people would ever need to tune. Just give it a reasonable internal default (3), and let people disable it via pref if needed. (I think that's unlikely too, although it's still good to have so we can hotfix disable it if needed.)

2) prompts.block_abusive_http_auth_requests? ...meh, I don't have a substantially better suggestion.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ +89,5 @@
> +
> +    // Allow only limited number of authentication dialogs when they are all
> +    // canceled by the user.
> +    var cancelationCounter = { count: 0, id: 0 };
> +    var chromeDoc = prompter._chromeWindow.document.documentElement;

You won't always have a _chromeWindow available, unfortunately.

But other than needing to deal with this being undefined, I don't think that's a problem... AFAIK this can only happen with extension related code (the comment in init() suggests a JSM or Sandbox), so that's probably ok to not worry about traps occurring in such cases?

MattN, that sound reasonable?

@@ +106,5 @@
> +    }
> +
> +    var cancelDialogLimit = 0;
> +    try {
> +      cancelDialogLimit = Services.prefs.getIntPref("general.authentication-dialog-bloat-limit");

This doesn't need to be in a try-catch, since the pref will always exist (since it's in all.js).

@@ +111,5 @@
> +    } catch (ex) { }
> +
> +    this.log("cancelationCounter = " + JSON.stringify(cancelationCounter));
> +    if (cancelDialogLimit && cancelationCounter.count >= cancelDialogLimit) {
> +      this.log("Blocking on limit");

"Blocking auth dialog, due to exceeding dialog bloat limit" (or whatever, just want to make this a little more descriptive since it's the first log message about what might be happening.)

@@ +135,5 @@
> +      }
> +
> +      Services.tm.mainThread.dispatch(cancel, Ci.nsIThread.DISPATCH_NORMAL);
> +      return;
> +    }

Instead of duplicating all this code from the existing runnable (a few lines down), I think it would simpler to just set a "shouldLimit" flag when the cancel limit is exceeded, and tweak the existing runnable code to skip showing an actual prompt when that flag was set.

For the master password uiBusy stuff on line 149, you could also add a check for shouldSkip so we don't try to bypass the prompt (since we know we'd just skip it anyway)... Although things would work just fine without doing this, so you don't actually need to do this.

@@ +178,5 @@
>  
> +        if (ok) {
> +          cancelationCounter.count = 0;
> +        } else {
> +          ++cancelationCounter.count;

Nit: A post-increment is more common style, and slightly clearer.

@@ +180,5 @@
> +          cancelationCounter.count = 0;
> +        } else {
> +          ++cancelationCounter.count;
> +        }
> +        chromeDoc.setAttribute("canceled-authentication-prompt-counter", JSON.stringify(cancelationCounter));

See above about _chromeWin.

Comment 26

[Justin Dolske [:Dolske]]

MattN: question for in last comment (search for MattN :).

Comment 27

[Honza Bambas (:mayhemer)]

(In reply to Justin Dolske [:Dolske] from comment #25)
> Comment on attachment 8807535 [details] [diff] [review]
> v2 (carry inner window id to httpchannelparent, limit cancled auth dialogs)
> 
> Review of attachment 8807535 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- but on the right track. :)

Thanks :)

> 
> ::: modules/libpref/init/all.js
> @@ +5522,5 @@
> > +// When a user cancels this number of authentication dialogs coming from
> > +// a sigle web page in a row, all following authentication dialogs will
> > +// be blocked (automatically canceled) for that page. The counter resets
> > +// when the page is reloaded. To turn this feature off, just set the limit to 0.
> > +pref("general.authentication-dialog-bloat-limit", 3);
> 
> At risk of bikeshedding a little:
> 
> 1) How about just making this a simple boolean? I think it's pretty unlike
> that this is something people would ever need to tune. Just give it a
> reasonable internal default (3), and let people disable it via pref if
> needed. (I think that's unlikely too, although it's still good to have so we
> can hotfix disable it if needed.)
> 
> 2) prompts.block_abusive_http_auth_requests? ...meh, I don't have a
> substantially better suggestion.

I suspect we will hit users that may want to cancel 3 dialogs and expect the fourth to give it it's credentials.  There are weird server configuration in the wild, also think of multiple auth challenges etc..  Hence, I'd rather keep the counter pref.  We can turn it off easily with setting it to 0.

I'll gladly rename it to be closer to your suggestion.

Comment 28

[Honza Bambas (:mayhemer)]

Attachment: v2.1 (carry inner window id to httpchannelparent, limit cancled auth dialogs)

- nits in the prompter fixed
- preference renamed, but left as a counter
- http parts unchanged

Comment 29

[Matthew N. [:MattN]]

(In reply to Justin Dolske [:Dolske] from comment #25)
> ::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
> @@ +89,5 @@
> > +
> > +    // Allow only limited number of authentication dialogs when they are all
> > +    // canceled by the user.
> > +    var cancelationCounter = { count: 0, id: 0 };
> > +    var chromeDoc = prompter._chromeWindow.document.documentElement;
> 
> You won't always have a _chromeWindow available, unfortunately.
> 
> But other than needing to deal with this being undefined, I don't think
> that's a problem... AFAIK this can only happen with extension related code
> (the comment in init() suggests a JSM or Sandbox), so that's probably ok to
> not worry about traps occurring in such cases?
> 
> MattN, that sound reasonable?

Right, I think it's fine to not block repeated requests if there is no chromeWindow.

From a quick skim of the bug I'm not sure why the counter is on `prompter._chromeWindow.document…` since that is referring to browser.xul. I would have expected that the counter would be on something specific to the tab/contentDocument/contentWindow. Apologies if I'm missing context.

Comment 30

[Honza Bambas (:mayhemer)]

(In reply to Matthew N. [:MattN] (away Nov. 3–7) from comment #29)
> From a quick skim of the bug I'm not sure why the counter is on
> `prompter._chromeWindow.document…` since that is referring to browser.xul. I
> would have expected that the counter would be on something specific to the
> tab/contentDocument/contentWindow. Apologies if I'm missing context.

the code is running on parent.  don't want to use CPOWs to get to the content.  and this is a perfect place.  there is just a single attribute per tab (exactly what I want).  content window is id'ed only (all the other IPC stuff in the patch)

Comment 31

[:Gijs (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #30)
> (In reply to Matthew N. [:MattN] (away Nov. 3–7) from comment #29)
> > From a quick skim of the bug I'm not sure why the counter is on
> > `prompter._chromeWindow.document…` since that is referring to browser.xul. I
> > would have expected that the counter would be on something specific to the
> > tab/contentDocument/contentWindow. Apologies if I'm missing context.
> 
> the code is running on parent.  don't want to use CPOWs to get to the
> content.  and this is a perfect place.  there is just a single attribute per
> tab (exactly what I want).  content window is id'ed only (all the other IPC
> stuff in the patch)

_chromeWindow, as Matt already said, is browser.xul. I'm pretty sure that in your code there isn't "a single attribute per tab" - there is one attribute for all the tabs in the window, because (unless Matt and I are both mistaken...) you're setting it on the chrome XUL <window> element. If multiple tabs are navigating they would clobber the value you're storing (because the inner window IDs of the different tabs are different, of course).

At a guess, you may want to store this on the <browser> elements, or you may want to use a Map() from inner window IDs to counts, (which would of course "leak" that data until the chrome window goes away, which may or may not be something we care about).

Either way, I'm not sure why you're using an attribute and JSON serialization - you can just store an expando on the DOM element or the _chromeWindow (effectively a global var in the chrome window), which makes using a single structure to cover all the tabs in the window more palatable.

Comment 32

[Matthew N. [:MattN]]

(In reply to Honza Bambas (:mayhemer) from comment #30)
> (In reply to Matthew N. [:MattN] (away Nov. 3–7) from comment #29)
> > From a quick skim of the bug I'm not sure why the counter is on
> > `prompter._chromeWindow.document…` since that is referring to browser.xul. I
> > would have expected that the counter would be on something specific to the
> > tab/contentDocument/contentWindow. Apologies if I'm missing context.
> 
> the code is running on parent.  don't want to use CPOWs to get to the
> content.  and this is a perfect place.  there is just a single attribute per
> tab (exactly what I want).  content window is id'ed only (all the other IPC
> stuff in the patch)

You don't need a CPOW to store per-browser state as you can store it on the <browser> like Gijs said.

Your response isn't convincing me that you're handling multiple tabs properly. There is one browser.xul per visible window. I agree with everything Gijs says.

Comment 33

[Matthew N. [:MattN]]

Side note: In code related to content which deals with ChromeWindow's I always like the variable names to be explicit about content vs. chrome and my insistence on renaming this variable lately (IIRC) is why I easily caught this bug.

Comment 34

[Patrick McManus [:mcmanus]]

Comment on attachment 8809862 [details] [diff] [review]
v2.1 (carry inner window id to httpchannelparent, limit cancled auth dialogs)

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

I'm ok with the changes to the necko data structures, but I'm not really the right reviewer for the correct use of those windowID calls. But when dolske has r+'d it that would seem right.

Comment 35

[Honza Bambas (:mayhemer)]

Matt, Gijs, you are right.

I did a simple test: a top level page (no authentication) referring an iframe which requires auth.  The top level page refreshes the iframe (location.reload()) every 4 seconds.  If I open just one tab, the patch works.  If I open two tabs, the attribute apparently refers to the _windows_ browser.xul_ and not to the tab's entity (if there is such), since the id is apparently always different.

(In reply to :Gijs Kruitbosch (PTO Nov. 17-20 inclusive) from comment #31)
> At a guess, you may want to store this on the <browser> elements, 

How do I reach it?  I never worked with this before, sorry.

And expando is better, right.  I somehow was convinced only acceptable value to setAttribute was a string.

Comment 36

[:Gijs (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #35)
> (In reply to :Gijs Kruitbosch (PTO Nov. 17-20 inclusive) from comment #31)
> > At a guess, you may want to store this on the <browser> elements, 
> 
> How do I reach it?  I never worked with this before, sorry.

From a quick look at the code, I think prompter._browser refers to the correct browser element, though it does look like that might be relying on CPOWs... Matt would be able to confirm whether this works correctly.

> And expando is better, right.  I somehow was convinced only acceptable value
> to setAttribute was a string.

setAttribute *does* only take strings, so you're right about that. What I meant is just setting

_browser._someProp = someObject

which should work.

Comment 37

[Honza Bambas (:mayhemer)]

(In reply to :Gijs Kruitbosch (PTO Nov. 17-20 inclusive) from comment #36)
> From a quick look at the code, I think prompter._browser refers to the

Yes, it works.  I can't confirm if it is or is not CPOWs, tho.

Comment 38

[Boris Zbarsky [:bzbarsky]]

> I can't confirm if it is or is not CPOWs, tho.

It's not.

Comment 39

[Honza Bambas (:mayhemer)]

Attachment: v2.2 (carry inner window id to httpchannelparent, limit cancled auth dialogs in pwdmngr)

- using prompter._browser.canceledAuthenticationPromptCounter expando
- otherwise the same except some small cleanup

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

Comment 40

[Honza Bambas (:mayhemer)]

Comment on attachment 8809862 [details] [diff] [review]
v2.1 (carry inner window id to httpchannelparent, limit cancled auth dialogs)

Sorry, forgot to obsolete this.

Comment 41

[Honza Bambas (:mayhemer)]

Comment on attachment 8811336 [details] [diff] [review]
v2.2 (carry inner window id to httpchannelparent, limit cancled auth dialogs in pwdmngr)

No response from Justin for 10 days.  Gijs, can you take a look please?  Feel free to bounce or suggest someone else.  I wanted to get this to 52 ASAP (esr).  Thanks.

Comment 42

[:Gijs (he/him)]

Comment on attachment 8811336 [details] [diff] [review]
v2.2 (carry inner window id to httpchannelparent, limit cancled auth dialogs in pwdmngr)

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

I don't have a lot of time right now. I might have more time on Monday, but I would suggest that Matt would be a better reviewer than me, if dolske is too busy. I've offered a few suggestions below, though...

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ +96,5 @@
>        this.log("_doAsyncPrompt:run bypassed, master password UI busy");
>        return;
>      }
>  
> +    // Allow only limited number of authentication dialogs when they are all

Nit: Allow only *a* limited number of ...

@@ +103,5 @@
> +    if (prompt.channel) {
> +      var httpchannel = prompt.channel.QueryInterface(Ci.nsIHttpChannel);
> +      if (httpchannel) {
> +        var windowId = httpchannel.contentWindowId;
> +        if (windowId != cancelationCounter.id) {

From a quick look, I was confused by this. Can we make sure that the property on the http channel is explicitly called *toplevel* content window ID? Because AIUI that's what it reflects, not the content window ID of the individual frame that might be causing the auth prompt.

@@ +104,5 @@
> +      var httpchannel = prompt.channel.QueryInterface(Ci.nsIHttpChannel);
> +      if (httpchannel) {
> +        var windowId = httpchannel.contentWindowId;
> +        if (windowId != cancelationCounter.id) {
> +          // window content have been reloaded or navigated, reset the counter

nit: window has been reloaded or navigated, reset the counter

@@ +172,5 @@
> +    if (cancelDialogLimit && cancelationCounter.count >= cancelDialogLimit) {
> +      this.log("Blocking auth dialog, due to exceeding dialog bloat limit");
> +      delete this._asyncPrompts[hashKey];
> +      prompt.inProgress = false;
> +      this._asyncPromptInProgress = false;

Do we actually need to set these to false? How would they have been set to true before now?

@@ +181,5 @@
> +      return;
> +    }
> +
> +    this._asyncPromptInProgress = true;
> +    prompt.inProgress = true;

Feels like it'd be a little simpler (and keep the .log) to put this in an else, and kill the early return, letting us fall through rather than duplicating the runnable dispatch.

Comment 43

[Honza Bambas (:mayhemer)]

Thanks for such a quick feedback!  I'll update and ask Matt.

Comment 44

[Honza Bambas (:mayhemer)]

Attachment: v2.3 (carry inner window id to httpchannelparent, limit cancled auth dialogs in pwdmngr)

Updated according comment 42.

BTW, there is still one way to perform the attack.  When a page has an iframe asking for auth and the top level page reloads itself constantly, we are in the same situation again.

Comment 45

[Honza Bambas (:mayhemer)]

Matt, ping on review.

Comment 46

[Matthew N. [:MattN]]

Comment on attachment 8814466 [details] [diff] [review]
v2.3 (carry inner window id to httpchannelparent, limit cancled auth dialogs in pwdmngr)

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

r=me but this (like any security/abuse prevention bug) really deserves a test (especially when we have existing ones to base it on). See toolkit/components/passwordmgr/test/mochitest/test_prompt_http.html for one example. I think the `handlePrompt` test helper should make this not very painful.

::: modules/libpref/init/all.js
@@ +5534,5 @@
>  pref("dom.storageManager.enabled", false);
>  #endif
>  
> +// When a user cancels this number of authentication dialogs coming from
> +// a sigle web page in a row, all following authentication dialogs will

Nit: "sigle"

@@ +5537,5 @@
> +// When a user cancels this number of authentication dialogs coming from
> +// a sigle web page in a row, all following authentication dialogs will
> +// be blocked (automatically canceled) for that page. The counter resets
> +// when the page is reloaded. To turn this feature off, just set the limit to 0.
> +pref("prompt.authentication_dialog_abuse_limit", 3);

Nit: We have another pref that starts with "prompts." (plural) so it would be good to make this plural too.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ +101,5 @@
> +    // 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) {

Nit: Use camelCase e.g. `httpChannel`

@@ +167,5 @@
>      };
>  
> +    var cancelDialogLimit = Services.prefs.getIntPref("prompt.authentication_dialog_abuse_limit");
> +
> +    this.log("cancelationCounter = " + JSON.stringify(cancelationCounter));

Nit: You should't need the JSON.stringify if you pass the object as the 2nd argument IIRC. Without it you will get an inspectable object in the DevTools and a different color-coding (which helps it stand out).
`this.log("cancelationCounter =", cancelationCounter);`

Comment 48

[Honza Bambas (:mayhemer)]

Attachment: v2.4 (r addressed, no test)

I'm going to land this now w/o a test and ask for a+ on m-a as I want this be in 52 (ESR).  I'll write the test later in a different bug and ask a+ to m-a for it too.  Sorry, I'm in a time press now and the test is not that easy to write as you may think.

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e077cda0a979
Block authentication dialog pop-ups by cancling 3 of them in a row, r=Dolske+mcmanus

Comment 50

[Matthew N. [:MattN]]

(In reply to Pulsebot from comment #49)
> r=Dolske+mcmanus

The reviewer list was stale

Comment 51

[Honza Bambas (:mayhemer)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #50)
> (In reply to Pulsebot from comment #49)
> > r=Dolske+mcmanus
> 
> The reviewer list was stale

Sorry, thanks!

Comment 52

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

https://hg.mozilla.org/mozilla-central/rev/e077cda0a979

Comment 54

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
This affects our users in the wild. While it's probably too late in the game for 51, we should try to take this for 52 if we can.

Comment 55

[Honza Bambas (:mayhemer)]

Comment on attachment 8819273 [details] [diff] [review]
v2.4 (r addressed, no test)

Approval Request Comment
[Feature/Bug causing the regression]: since ever
[User impact if declined]: complete blockade of UI through a number of modal auth dialogs
[Is this code covered by automated tests?]: somewhat, we have tests for these parts of code, but not for this particular issue (filed a followup bug 1323973)
[Has the fix been verified in Nightly?]: locally, yes
[Needs manual test from QE? If yes, steps to reproduce]: before bug 1323973 lands (not sooner than Jan 2017), maybe
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: I think not
[Why is the change risky/not risky?]: reload of a page discards any auth dialog limits this patch introduces, it can also be turned off by a preference
[String changes made/needed]: none

Comment 56

[Julien Cristau [:jcristau]]

Is there a test URL that QE could use for verification?

Comment 57

[Honza Bambas (:mayhemer)]

(In reply to Julien Cristau [:jcristau] from comment #56)
> Is there a test URL that QE could use for verification?

I think here are some URLs to test with: https://bugzilla.mozilla.org/show_bug.cgi?id=1324226#c1

Comment 58

[Julien Cristau [:jcristau]]

Comment on attachment 8819273 [details] [diff] [review]
v2.4 (r addressed, no test)

limit the number of auth pop-ups shown for a page to block abuse, take in aurora52

Comment 59

[Julien Cristau [:jcristau]]

Hopefully comment 57 can help with verification, though it looks like the URLs mentioned there no longer exist.

Comment 60

[Carsten Book [:Tomcat]]

need rebasing

Comment 61

[Honza Bambas (:mayhemer)]

Attachment: v2.4 for m-a

Here it is (build checked locally, debug only)

Comment 62

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fec44289a5e2

Comment 63

[Bogdan Maris, Desktop Test Engineering]

The links from comment 57 does not work anymore so I had a look at the duplicate bugs here but was unable to find some link or a testcase in order to reproduce/verify this bug.
I used the data URI attached in bug 1312836 comment 1 but under Windows 7 and macOS 10.12.3 I see the same behavior on Fx 49.0 (which should be not fixed) as on Fx 52 beta 8 (which should be fixed), with every click on that webpage Fx will open window after window.

Chris, I know it's been a long time but could you reliable reproduce this? If yes can you make public the website you used? 
If anyone else knows how I can reproduce (in order to verify) this please let me know, it will be much appreciated.

Comment 64

[Chris]

Sorry The site appears to be assist.support-care-uk.in (from the image I posted) but it no longer exists. 

Because of the nature of the exploit I was not able to copy the data URL.

I have searched through my history and have a gap around the time of the incident. Which would be because I had to kill the browser so its recent history was not flushed to disk.

The only places in my history I visited around that time and date were netflix and google. I do not recall what the original site was but I was pulled into a rapid circle jerk which finished at the data URL. I could not reproduce it if I tried.

Comment 65

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

As the links from comment 57 and from the duplicate bugs don't work anymore, I tried to find another valid links that can trigger this issue, but with no luck. 
With https://bug1312836.bmoattachments.org/attachment.cgi?id=8804398, I noticed the same behavior on Windows 10 x64, using 52.0a1 (2016-10-22), 53.0b1 build1 (20170307064827) and 52.0 build2 (20170302120751) (like Bogdan did - see comment 63) - with every click on that webpage, Firefox will open window after window (about:blank). 
Chris, Honza, any thoughts about this behavior? Also, any chance to find a valid link for reproducing / verifying this issue?

Comment 66

[Honza Bambas (:mayhemer)]

Note that this fixes only a case when a page contains an iframe that 1) asks for auth 2) is reloaded by the top level page on and on.  If such the page would reload itself (not just the iframe), the patch would not help.

On Nightly and Beta I _can't_ repro with https://bug1312836.bmoattachments.org/attachment.cgi?id=8804398

Comment 67

[Honza Bambas (:mayhemer)]

And another note that this only protects against abuse auth prompts, nothing else.

Comment 68

[ga48]

(In reply to Iulia Cristescu, QA [:JuliaC] from comment #65)

> Also, any chance to find a valid link for reproducing / verifying this issue?

https://bugzilla.mozilla.org/show_bug.cgi?id=1439157

Comment 69

[Daniel Bodea [:danibodea]]

Some steps to reproduce are still required in order to correctly verify this bug. Honza, Do you think this can still be verified at this point or should we ignore it in the future?

Comment 70

[Honza Bambas (:mayhemer)]

Is your request to me to provide the STR? This is hard to test in an automated fashion, yes, so a static web page for testing would be good to have, right?

Comment 71

[Daniel Bodea [:danibodea]]

Yes, please. That would be very helpful. Thank you.

Comment 72

[Honza Bambas (:mayhemer)]

Deadline?

Comment 73

[Daniel Bodea [:danibodea]]

Considering the fact that this "qe-verify+" tag was set about 3 years ago, I don't think there's any hurry to it... :)

Comment 74

[Daniel Bodea [:danibodea]]

Just a reminder for Honza :)

Comment 75

[Honza Bambas (:mayhemer)]

Is the test in bug 377496 enough?

Comment 76

[Daniel Bodea [:danibodea]]

In bug 377496, there are 2 test cases:

1. A test case in the description: opening a page (http://gobase.org/studying/articles/mioch/7/index.html) and the authentification prompt would appear by default. The authentification prompt does not appear anymore.

2. Another test case in the comment section (https://bugzilla.mozilla.org/show_bug.cgi?id=377496#c36): which has a test page (http://www.httpwatch.com/httpgallery/authentication/#showExample10). Clickin the "DISPLAY IMAGE" button will trigger an authentification prompt. In this case, the prompt is displayed, and the page is still somewhat unusable (only scrolling it is possible while the prompt is not dismissed).

• Clicking the prompt's OK button two times in a row will dismiss the prompt and the page becomes usable.
• Clicking the prompt's "Cancel" or "X" buttons will instantly dismiss the authentification prompt and leave the page usable.

I don't know what this bug fixed/what are the previous expected results and the newly expected results. How exactly is this supposed to work?

Comment 77

[Honza Bambas (:mayhemer)]

As there is some work happening in bug 613785, this is no longer serious enough to spend time verifying. Bug 613785 will actually nullify this issue completely.