873810 - Background tabs can force an HTTP auth prompt over the active tab to steal the user's credentials for the currently-active website

Status: RESOLVED DUPLICATE of bug 791594

(Firefox :: Security, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Florian Bender from comment #16)
> (In reply to Tom Schuster [:evilpie] from comment #14)
> > This tweet (https://twitter.com/RSnake/status/328908680097574912)
> 
> Is there a bug covering the "delayed reload and auth" attack?

The code for this is dead simple:

<a href="javascript:clickit();">Go to your the bank's HTTPS site.</A>
<script>
function clickit() {
  var w = window.open('http://www.bankofamerica.com/');
  setTimeout(function () {
    w.location = 'http://ha.ckers.org/401/';
  }, 8000);
}
</script>

Marking sec-high given it meets the criteria "Spoofing of full URL bar or bypass of SSL integrity checks".

This will probably be fixed by fixing bug 613785.

I am not sure this is the best component for this bug. Feel free to correct it.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I tested this in Chrome and Chrome has the same problem, though there seems to be a race condition that causes it to not to happen 100%. I am not sure why the cited tweet mentioned only Firefox and MSIE. Other twitter replies also confirm that.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Sid any ideas who might be able to take this one?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

We need to figure out a proposal for what should happen for that test case. I don't have any great ideas offhand, and I don't see how bug 613785 would help.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

I'm also not really sure where this bug belongs. Doug any idea who might want to dig into this one?

Comment 5

[O. Atsushi (Torisugari)]

For bug tracking guys:
This bug is a simple combination of bug 709892 and bug 656343, if I understand correctly.

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #3)
> We need to figure out a proposal for what should happen for that test case.
> I don't have any great ideas offhand, and I don't see how bug 613785 would
> help.

My proposal:
1. Use a new error page, say, <about:authprompt> (xhtml or xul).
2. Delete URLbar like <about:addons>.
3. Render another (and readonly) URLbar inside of <about:authprompt>.

Opinion?

Comment 6

[Florian Bender]

(In reply to O. Atsushi (Torisugari) from comment #5)
> 2. Delete URLbar like <about:addons>.

This special view is going away, i. e. the addon page (et al.) will show the URL bar. 

However, there's a new identity indicator for in-content chrome pages (everything about:*) which you can see in current nightlies.

Comment 7

[O. Atsushi (Torisugari)]

(In reply to Florian Bender from comment #6)
> This special view is going away, i. e. the addon page (et al.) will show the
> URL bar. 
> 
> However, there's a new identity indicator for in-content chrome pages
> (everything about:*) which you can see in current nightlies.

Hmm.

If the browser unloads the previous page (i.e. www.backofamerica.com) completely, it's fine (at least for me) to call onLocationChange(...). In that case, the URLbar will display "http://ha.ckers.org/401/".

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

(In reply to David Bolter [:davidb] from comment #2)
> Sid any ideas who might be able to take this one?

Sorry for the long delay here.  I think the right approach is to do as comment 0 says: make it tab modal (fix bug 613785).

Comment 9

[O. Atsushi (Torisugari)]

I'm afraid tab-modal dialog won't fix this bug, as Gavin pointed out.

Comment 10

[Florian Bender]

How about unloading the current document and displaying the new URL in the location bar (as suggested in Comment 7) before showing the dialogue, making this a first step to lessen the severity of this problem?

Comment 11

[Florian Bender]

Attachment: Proposed new authentication dialogue

Also, redesigning/rewording the authetication dialogue a bit, you can make it clearer which site requests the authetication. I created a quick mockup: 

1) Rearranged elements: The website name is presented to the user in a more attention-grabbing way. 
2) The URL is highlighted similarly to the URL in the location bar: 1st and 2nd level is written in bold and the rest is written in a regular font. 
3) Small rewording to make it clearer what the user needs to do and what info he gets from the website. 

I think this is another quick fix which should be fairly easy to implement, increases UX while raising user awareness in a subtle way. This of course does not resolve the underlying issue, but helping mitigating such attacks anyway.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Florian Bender from comment #10)
> How about unloading the current document and displaying the new URL in the
> location bar (as suggested in Comment 7) before showing the dialogue, making
> this a first step to lessen the severity of this problem?

This seems like the best idea so far.

(In reply to Florian Bender from comment #11)
> Created attachment 775132 [details]
> Proposed new authentication dialogue
> 
> Also, redesigning/rewording the authetication dialogue a bit, you can make
> it clearer which site requests the authetication. I created a quick mockup: 
> 
> 1) Rearranged elements: The website name is presented to the user in a more
> attention-grabbing way. 
> 2) The URL is highlighted similarly to the URL in the location bar: 1st and
> 2nd level is written in bold and the rest is written in a regular font. 
> 3) Small rewording to make it clearer what the user needs to do and what
> info he gets from the website. 
> 
> I think this is another quick fix which should be fairly easy to implement,
> increases UX while raising user awareness in a subtle way. This of course
> does not resolve the underlying issue, but helping mitigating such attacks
> anyway.

I am not opposed to your changes in general (It is better to make the UI more readable), but I don't think that rearranging that dialog box is going to help much. One suggestion on your design though: we should draw the URL the same way we would draw it in the address bar: highlighting the eTLD+1 and with a security indicator (lock or no).

(In reply to O. Atsushi (Torisugari) from comment #9)
> I'm afraid tab-modal dialog won't fix this bug, as Gavin pointed out.

Thanks for pointing this out guys. If I understand you correctly, you are saying that window.open isn't actually the problem, but rather the delayed reload is. I was thinking the problem was related to the window.open for some reason.

Comment 13

[Florian Bender]

(In reply to Brian Smith (:briansmith), was [e-mail] (:bsmith) from comment #12)
> I am not opposed to your changes in general (It is better to make the UI
> more readable), but I don't think that rearranging that dialog box is going
> to help much. 

As I said this won't resolve the underlying issue but it may raise user awareness. This is a good first step until we have a proper solution. 

> One suggestion on your design though: we should draw the URL the same way
> we would draw it in the address bar: highlighting the eTLD+1 and with a
> security indicator (lock or no).

My mockup does have a eTLD+1 highlight, it's not very visible though ;). The addition of a lock icon and/or website owner is a good idea. 

Speaking of that, some bug said it's impossible to check the website's encryption info when faced with an auth prompt because you cannot click the identity icon (due to the prompt being modal). So if we can't make the auth dialogue non-blocking, we might as well present more info and action in the prompt (like lock icon + website owner and/or certificate issuer with a button to show the certificate or similar) even if that means doubling information sources. Or fix Bug 613785 or revive Bug 567804 instead … but the prompt should at least show the icon and website owner (if available). 


Either way, how about just removing any setTimeouts and setIntervals originating from an unloaded page or fail any user-facing APIs (showing UI or manipulating a document's location)?

Comment 14

[Andrew McCreight [:mccr8]]

What's the next step here?  It has been sitting untouched for three months.

Comment 15

[Justin Dolske [:Dolske]]

Yeah, adding tabmodalness isn't going to help, since the root problem is that the chrome prompt (whatever form it takes) is basically independent from the tab it's supposed to be associated with. In fact this problem is even worse with a tab-modal prompt -- it's more tightly coupled, visually, to the page, and so a user is even less likely to notice that the prompt says it's actually from ha.ckers.org.

Note that the testcase works just as well in reverse -- open a window to ha.ckers.org (triggering a prompt), and use a setTimeout to navigate it to bank.com a second later.

I think we have to fix two separate problems here:

For the existing test case, the issue is that we've made a successful HTTP request for the new page (thus triggering the prompt), but haven't torn down the existing page or changed the location in the URL bar. I'm not sure how this stuff works under the hood, probably need some help from bz for docshell stuff. I suspect this is intentional for other situations, so one can cancel the operation (eg redirects?) and remain on the previous page. HTTP Auth is a little unique in that it can trigger UI before the page starts loading... I think what we need here is for an HTTP 401 to trigger tearing down the old page -- when the prompt comes up, you should see a blank page and the URL for the 401.

For the reversed test case, we basically just need to have onLocationChange take care of explicitly closing any HTTP auth prompts associated with the window that is navigating. Or have the prompts handle listening themselves, not sure which will be easier. I'd kinda prefer the former, since ISTR some other stuff is making use of the tabprompt overlay, and those things might also want to know about this kind of thing.

Dao, can you take a look at this?

Comment 16

[O. Atsushi (Torisugari)]

(In reply to Justin Dolske [:Dolske] from comment #15)
> For the existing test case, the issue is that we've made a successful HTTP
> request for the new page (thus triggering the prompt), but haven't torn down
> the existing page or changed the location in the URL bar. I'm not sure how
> this stuff works under the hood, probably need some help from bz for
> docshell stuff. I suspect this is intentional for other situations, so one
> can cancel the operation (eg redirects?) and remain on the previous page.
> HTTP Auth is a little unique in that it can trigger UI before the page
> starts loading... I think what we need here is for an HTTP 401 to trigger
> tearing down the old page -- when the prompt comes up, you should see a
> blank page and the URL for the 401.

If I understand correctly, that's rather httpchannel's problem. When httpchanngel is processing the auth-prompt, its stage is (failed) onStartRequest. That is, the channel never fires onStartRequest/onDataAvailable against docshell. So probably docloader can't receive http BODY (HTML data which says "401 Not authenticated!") even if necko has completely downloaded it. 

I agree Firefox should load "401" or something new so as to unload the old document.

Comment 17

[Justin Dolske [:Dolske]]

(In reply to O. Atsushi (Torisugari) from comment #16)

> I agree Firefox should load "401" or something new so as to unload the old
> document.

A simple blank page would be best, since having an auth prompt shown over an error page would be kind of confusing. The fail-prompt-retry may be how HTTP works under the hood, but likely isn't the mental model users have.

Comment 18

[Johnny Stenback (:jst)]

Any updates here?

Comment 19

[Dão Gottwald [:dao]]

Attachment: patch

I looked into tearing down the old document somewhere in necko, but didn't get far. It occurred to me that when get the 401 response, we must already have sent onStateChange notifications and initiating another load (for about:blank) in the middle of that in order to tear down the old document seems like it might create quite a mess. So here's a front-end-only fix instead. It's kind of a hack.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 828724 [details] [diff] [review]
patch

bz, please see comment 19. Let me know if you think actually unloading the previous document is feasible and sensible.

Comment 21

[Boris Zbarsky [:bzbarsky]]

If we rejiggered necko enough that it didn't do its own rerequesting but rather left that up to the consumer, we could do that.  In fact, we could make the "error page" for a 401 basically be a login page that then starts the new necko request, right?  That seems like the simplest approach to the teardown problem to me.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 828724 [details] [diff] [review]
patch

This seems like an OK workaround, yeah.  Though shouldn't we restore the url bar?

Comment 23

[Dão Gottwald [:dao]]

(In reply to Boris Zbarsky [:bz] from comment #21)
> If we rejiggered necko enough that it didn't do its own rerequesting but
> rather left that up to the consumer, we could do that.  In fact, we could
> make the "error page" for a 401 basically be a login page that then starts
> the new necko request, right?  That seems like the simplest approach to the
> teardown problem to me.

Ok, I'll file this as a new bug if my current patch gets accepted.

(In reply to Boris Zbarsky [:bz] from comment #22)
> Comment on attachment 828724 [details] [diff] [review]
> patch
> 
> This seems like an OK workaround, yeah.  Though shouldn't we restore the url
> bar?

The next onLocationChange should do that automatically, but yeah, it's probably better to do it explicitly.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 828724 [details] [diff] [review]
patch

>diff --git a/toolkit/components/prompts/content/commonDialog.js b/toolkit/components/prompts/content/commonDialog.js

>+        webProgress.addProgressListener(progressListener, Ci.nsIWebProgress.NOTIFY_ALL);

NOTIFY_STATE_ALL?

>+let progressListener = {
>+    onLocationChange: function () {},
>+    onProgressChange: function () {},
>+    onSecurityChange: function () {},
>+    onStateChange: function () {
>+        window.close();
>+    },

Hmm, are there any edge cases that this would hit where we want to avoid closing the dialog?

Comment 25

[Justin Dolske [:Dolske]]

Comment on attachment 828724 [details] [diff] [review]
patch

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

I think there are a couple of things to fix here, but it's possible I'm crazy. :)

::: browser/base/content/tabbrowser.xml
@@ +3206,5 @@
> +        var tab = this._getTabForContentWindow(event.target.top);
> +
> +        this.selectedTab = tab;
> +        URLBarSetURI(makeURI("about:blank"));
> +

This approach generally seems fine as a front-end workaround for the "existing test case" in comment 15 (an auth prompt coming up for a new page that's starting to load, before the old page/URL has been removed).

Can we narrow this change to when we know the prompt is coming from a new top-level document load (as opposed to a sub-resource on a page triggering an auth prompt)? Hmm, well, that would make for a minimal / least-risk change here... But... Maybe that just makes sense to do as a way to help mitigate foo.com having a <img src="bar.com/authspoof.jpg"> injected. The flashing might be kind of annoying, and you lose the context for the top-document's location... But I'm not sure that actually concerns me.

::: toolkit/components/prompts/content/commonDialog.js
@@ +30,5 @@
> +                            .getInterface(Ci.nsIWebNavigation)
> +                            .QueryInterface(Ci.nsIInterfaceRequestor)
> +                            .getInterface(Ci.nsIWebProgress);
> +        webProgress.addProgressListener(progressListener, Ci.nsIWebProgress.NOTIFY_ALL);
> +    }

Hmm, I wonder if it's possible to trigger a prompt and navigate the page before this onload fires (ie, the thing we want to listen for here has already happened).

Not sure if this can happen, or how to check for it.

@@ +90,5 @@
> +    onProgressChange: function () {},
> +    onSecurityChange: function () {},
> +    onStateChange: function () {
> +        window.close();
> +    },

Shouldn't this be in onLocationChange? Presumably a prompt could come up before STATE_STOP, so it wouldn't make sense to have that trigger the close.

This is only fixing the case of a window navigating under an existing auth prompt, right? (ie, "reversed test case" in comment 15)

Do we need to check for this being a top-level doc? I forget. (i.e., I think we don't care security-wise if it's an iframe navigating) Or more specifically, having an iframe navigate shouldn't close an authprompt from some parent document. Oh, but I guess that can't happen, since if a request is 401, it shouldn't have a document yet (and, thus, no subframes to navigate).

...oh, but you're doing this for _all_ prompts, so that is a problem. Should probably check args.promptType to make this only apply to auth prompts, but a general fix might be nice. Not sure what happens right now if a page navigates under an alert().

::: toolkit/components/prompts/src/nsPrompter.js
@@ +776,5 @@
>  
>          let userParam = { value: username };
>          let passParam = { value: password };
>  
> +        PromptUtils.fireDialogEvent(this.domWin, "MozBeforeAuthPrompt");

I just spent 15 minutes trying to remember where we added a way to fire such events only at chrome, so we could use it here... Only to find this is the exact function I was thinking of, and the exact place we added it. Sigh!

Comment 26

[Johnny Stenback (:jst)]

Any updates here guys?

Comment 27

[Dão Gottwald [:dao]]

(In reply to Justin Dolske [:Dolske] from comment #25)
> Can we narrow this change to when we know the prompt is coming from a new
> top-level document load (as opposed to a sub-resource on a page triggering
> an auth prompt)? Hmm, well, that would make for a minimal / least-risk
> change here... But... Maybe that just makes sense to do as a way to help
> mitigate foo.com having a <img src="bar.com/authspoof.jpg"> injected. The
> flashing might be kind of annoying, and you lose the context for the
> top-document's location... But I'm not sure that actually concerns me.

Right, it's not clear to me that we want to narrow it down. I assumed that we wouldn't.

Comment 28

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 29

[Johnny Stenback (:jst)]

Dolske, review ping?

Comment 30

[Justin Dolske [:Dolske]]

Comment on attachment 8359669 [details] [diff] [review]
patch v2

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

Can we get a couple of tests here? Some of the existing prompt / pwmgr tests should be easy enough to modify for such if you're looking for something to cut'n'paste. In addition to the usual "tests are good" mantra, it would be good to have something that explicitly fails to remind us about this if/when we switch to tab-modal auth prompts.

r+ with the fix for restoring the URI and a test.

::: browser/base/content/tabbrowser.xml
@@ +3194,5 @@
> +
> +        var tab = this._getTabForContentWindow(event.target.top);
> +
> +        this.selectedTab = tab;
> +        URLBarSetURI(makeURI("about:blank"));

You need to restore the URI in MozAfterAuthPrompt, otherwise the URL bar stays empty. (eg http://dolske.net/mozilla/tests/httpauth/img.html)

::: toolkit/components/prompts/content/commonDialog.js
@@ +90,5 @@
> +    onLocationChange: function () {
> +        if (args.promptType == "promptPassword" ||
> +            args.promptType == "promptUserAndPass")
> +            window.close();
> +    },

(Note to future self): It would be nice to have this in CommonDialog, as part of a generic shutdown thing for all type of prompts, as well as tab-modal prompts. Since alert() and friends will be needlessly tying up a nested event loop. Should be harmless/rare, but would be nice if it was easy to fix. OTOH, this would have to be a lot more accurate at matching up prompts to subframes that navigate, and that feels like a lot of extra effort that probably isn't needed for HTTP Auth.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

sec-moderate seems appropriate, given the interaction required, the fact that it's not a complete "URL bar spoof".

Is this just waiting on a test?

Comment 34

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 35

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Comment 36

[Paul Zühlcke [:pbz]]

Fixed in Bug 791594