1333599 - URL Spoofing by using onbeforeunload and opening a new window

Status: RESOLVED DUPLICATE of bug 1481994

(Firefox :: Address Bar, defect, P2)

Description

[Luan Herrera]

I have noticed that the onbeforeunload dialog is being dismissed whenever a new window is opened. The bug lies in the fact that the URL displayed in the address bar is not being returned to the true URL of the page after the dialog is dismissed.
By chaining a popup blocker bypass [1] and a window.close bug [2] it's possible to spoof the address bar.

Reproduced on Windows 7 and Ubuntu:
Version -> 50.1.0. 
Build ID -> 20161209095719.

Steps to reproduce:
1. Access https://lbherrera.github.io/lab/firefox/index.html
2. Interact with the page (click anywhere) so onbeforeunload is allowed to fire.
3. Try to access https://www.google.com through the address bar (by typing, selecting or copy pasting),
4. A fake page should load under Google's URL.

Reproducible: Always.

Actual results:  
The URL in the address bar displays what was typed in it (in this case https://www.google.com).

Expected results:  
When the new window was opened, the onbeforeunload dialog shouldn't have been dismissed. It would only make sense to dismiss the dialog if the URL in the address bar returned to its original state given no navigation was initiated.

References:
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1266397
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1316857

Comment 1

[:Gijs (he/him)]

Testing on 51 rc and nightly, with e10s on and off, on os x and windows, I can't reproduce this part:

(In reply to Luan Herrera from comment #0)
> I have noticed that the onbeforeunload dialog is being dismissed whenever a
> new window is opened.

I get the beforeunload dialog as expected, and it is NOT dismissed automatically, I have to make a choice.

Can you reproduce this automatic dismissal on newer versions than 50 (we're releasing 51 this week - you can use https://beta.mozilla.org and https://nightly.mozilla.org to get copies of 51rc and nightly, respectively) ? On a clean profile? Maybe I need to allow popups for the site, or allow flash, or something else you've omitted from the steps to reproduce?

> The bug lies in the fact that the URL displayed in the
> address bar is not being returned to the true URL of the page after the
> dialog is dismissed.

This might still be a good idea, though we can open up the bug if the security issue is fixed and that would be helpful. Marco, what do you think?

Comment 2

[Marco Bonardo [:mak]]

(In reply to :Gijs from comment #1)
> (In reply to Luan Herrera from comment #0)
> > I have noticed that the onbeforeunload dialog is being dismissed whenever a
> > new window is opened.
> 
> I get the beforeunload dialog as expected, and it is NOT dismissed
> automatically, I have to make a choice.

If I allow Flash and click on the page, then I can reproduce the automatic dismissal.
The steps for me are:
1. load the page
2. click on Allow plug-in and confirm
3. paste & go "https://www.google.com"

If later I reload the page with Flash already allowed, I can't reproduce. As well as I can't reproduce if I don't allow Flash.
As it is, I don't know how much of this depends on the "allow plugin" behavior, but a good part of it seems to depend on some special handling of plug-ins.

There is clearly a bug if the page can anyway dismiss the onbeforeunload dialog without interaction, regardless of what we do with the urlbar contents.

> > The bug lies in the fact that the URL displayed in the
> > address bar is not being returned to the true URL of the page after the
> > dialog is dismissed.
> 
> This might still be a good idea, though we can open up the bug if the
> security issue is fixed and that would be helpful. Marco, what do you think?

I don't think we should open this, until we figure out what allows to dismiss the unload dialog automatically.
I see 2 bugs here:
1. it should not be possible to bypass the dialog
2. we can and should revert the urlbar value if the load is canceled by "Stay on this page".

Comment 3

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #2)
> (In reply to :Gijs from comment #1)
> > (In reply to Luan Herrera from comment #0)
> > > I have noticed that the onbeforeunload dialog is being dismissed whenever a
> > > new window is opened.
> > 
> > I get the beforeunload dialog as expected, and it is NOT dismissed
> > automatically, I have to make a choice.
> 
> If I allow Flash and click on the page, then I can reproduce the automatic
> dismissal.
> The steps for me are:
> 1. load the page
> 2. click on Allow plug-in and confirm
> 3. paste & go "https://www.google.com"
> 
> If later I reload the page with Flash already allowed, I can't reproduce. As
> well as I can't reproduce if I don't allow Flash.
> As it is, I don't know how much of this depends on the "allow plugin"
> behavior, but a good part of it seems to depend on some special handling of
> plug-ins.
> 
> There is clearly a bug if the page can anyway dismiss the onbeforeunload
> dialog without interaction, regardless of what we do with the urlbar
> contents.

I think this is effectively bug 1266397, because navigation is otherwise disabled while the dialog is up, which prevents normal JS doing this (without using flash), AIUI, but I haven't done any additional testing to verify this.

Comment 4

[Luan Herrera]

(In reply to :Gijs from comment #1)
> Can you reproduce this automatic dismissal on newer versions than 50 (we're
> releasing 51 this week - you can use https://beta.mozilla.org and
> https://nightly.mozilla.org to get copies of 51rc and nightly, respectively)
> ? On a clean profile? Maybe I need to allow popups for the site, or allow
> flash, or something else you've omitted from the steps to reproduce?

Flash needs to be allowed given that for the dialog to be dismissed automatically a new window has to be opened (but for me it's allowed by default, or am I missing something?). I am using the popup blocker bypass reported on bug 1266397 to achieve this and then closing the window using bug 1316857, sorry for not being clearer.
I also downloaded and tested on Beta and Nightly. It works fine on Beta but not on Nightly (I am going to do more tests and try to make it work there as well).

Comment 5

[:Gijs (he/him)]

(In reply to Luan Herrera from comment #4)
> (In reply to :Gijs from comment #1)
> > Can you reproduce this automatic dismissal on newer versions than 50 (we're
> > releasing 51 this week - you can use https://beta.mozilla.org and
> > https://nightly.mozilla.org to get copies of 51rc and nightly, respectively)
> > ? On a clean profile? Maybe I need to allow popups for the site, or allow
> > flash, or something else you've omitted from the steps to reproduce?
> 
> Flash needs to be allowed given that for the dialog to be dismissed
> automatically a new window has to be opened (but for me it's allowed by
> default, or am I missing something?). I am using the popup blocker bypass
> reported on bug 1266397 to achieve this and then closing the window using
> bug 1316857, sorry for not being clearer.
> I also downloaded and tested on Beta and Nightly. It works fine on Beta but
> not on Nightly (I am going to do more tests and try to make it work there as
> well).

Flash won't be enabled by default (for everyone) on current Nightly, and I had manually disabled it on the 51 profiles on which I tested, for security reasons. Oops. Thanks for clarifying.

Comment 6

[Luan Herrera]

(In reply to :Gijs from comment #3)

> I think this is effectively bug 1266397, because navigation is otherwise
> disabled while the dialog is up, which prevents normal JS doing this
> (without using flash), AIUI, but I haven't done any additional testing to
> verify this.

I did a test on Beta and Nightly and if the user allows popups in the site the bug also works.
My guess would be that because window.open is called first, the window will be opened before the dialog can prevent JS from executing. So if one can find a JS popup blocker bypass he would still be able to exploit this in future builds. I am going to do more tests and report what I find later.

window.onbeforeunload = function() {
     window.open("javascript:window.close();")
     return "";
}

You can test it here: https://lbherrera.github.io/lab/firefox/version2.html

Comment 7

[Frederik Braun [:freddy]]

Mak, you're the triage owner. Can you find someone to work on this?

Comment 8

[Marco Bonardo [:mak]]

(In reply to Frederik Braun [:freddyb] from comment #7)
> Mak, you're the triage owner. Can you find someone to work on this?

I'm not sure whether it's responsibility of Triage owners to find people, is not that responsibility of engineering management? I don't have nor manage a team, so the only answer I could give would be myself. And Gijs is already working on other security fixes, afaik.

Afaict, this bug is just the result of pre-existing bugs, bug 1266397 is currently assigned to :jimm, while bug 1316857 is unassigned in Core :: Document Navigation, for which I don't have a deep knowledge.

As expressed previously, what we could try to do here additionally, is to revert the urlbar when the onbeforeunload dialog is dismissed. I don't have cycles atm, but I could try to give a look if no one else can.
Fixing at least one of the dependencies would help a lot, since then we could open the bug and then maybe get more volunteers.

Comment 9

[:Gijs (he/him)]

FWIW, I concur with Marco's assessment here that as engineers / triagers it isn't necessarily within our purview or even abilities to find owners. I have a number of sec bugs I'd like to fix and I'm not even getting around to those, never mind additional ones. If the security team has people who have cycles, I can help them work on this bug by pointing to relevant code. It's not actually hard to fix this by means of what Marco's suggesting, but I just don't have time.

Here's an educated guess:

1) make DOMModalDialogClosed pass in whether it's a beforeunload dialog (the 'open' event already does this, a few lines down):

https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/browser/modules/RemotePrompt.jsm#54

(IIRC there's a non-remote dispatch of this thing as well (nsPrompter.js) which might also need adjusting, but I could be wrong)

2) also pass in whether the dialog was cancelled (the newPrompt ref should have a Dialog member which points to an instance of CommonDialog, see https://dxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/CommonDialog.jsm , which should store the user action taken)

3) add a DOMModalDialogClosed handler to tabbrowser.xml (the 'open' one is here: https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/browser/base/content/tabbrowser.xml#5174 )

4) in the handler, if we're closing the dialog and it's a permitunload one, call URLBarSetURI().

Comment 10

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

Florian, do you have any cycles to take this?

Comment 11

[:Gijs (he/him)]

I think Frederik volunteered to take this on IRC. :-)

Comment 12

[Frederik Braun [:freddy]]

Yeah, I'm taking a look.

Comment 13

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1333599-reset-URL-after-onbefureunload-modal-dia.patch

I think this is it. The event fires (note my dump() statement), but it does not update the awesome bar.
I've been testing the website mentioned in comment 6 manually, I'll follow up with a browser-mochitest soon.

Comment 14

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1333599-test-correct-URL-after-onbefureunload-.patch

This (as well as the previous patch) is a pretty immature and there are some loose ends that I need to tie up, that'll probably involve some more questions for you :)

I'm putting this up here in case someone wants to look at it over the weekend, but I'll continue on Monday either way.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8838644 [details] [diff] [review]
0001-Bug-1333599-reset-URL-after-onbefureunload-modal-dia.patch

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

Yes, this looks like the right approach. Thanks for picking this up!

::: browser/base/content/tabbrowser.xml
@@ +5156,5 @@
> +            return;
> +
> +          if (event.detail.inPermitUnload && event.detail.wasCancelled) {
> +            dump("yay our case is being hit. lets reset the awesomebar\n"); // FREDDY WILL REMOVE THIS BEFORE LANDING
> +            URLBarSetURI();

Err, oops, I just realized, we should re-set the browser's userTypedValue (which basically tracks the value the user typed / dragged / copied / whatever into the URL bar, on a per-tab basis). I'm not sure if this DOMModalDialogClosed event is dispatched synchronously after user action (and before web content gets a say) in both remote and non-remote cases, so to be sure that we affect the right browser if this races with a tab-switch (which in some cases could be caused by web content...) we should probably re-set it for the specific browser associated with the event. See this code (for the DOMModalDialogOpen case) for how to get the right browser object (I expect, at least, that this works for the close case, too):

https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/browser/base/content/tabbrowser.xml#5181-5188

then you can just set its .userTypedValue to null, and *then* call URLBarSetURI(). :-)

::: browser/modules/RemotePrompt.jsm
@@ +55,5 @@
>          needRemove = true;
>  
> +      let eventDetail = {
> +        inPermitUnload: args.inPermitUnload,
> +        wasCancelled: wasCancelled

Nit: in ES-whatever-version-we're-up-to-now, when the name of the key in an object matches the identifier of the value you're assigning to it, you can just mention only the key name - think of it as the opposite of object/array deconstruction. So just 'wasCancelled,' will do here. In nsPrompter.js, this goes for both properties.

(Trailing comma after each key is normal browser/ style to make it easy to add extra properties later without touching blame)

::: toolkit/components/prompts/src/CommonDialog.jsm
@@ +220,4 @@
>              aNode.accessKey = accessKey;
>      },
>  
> +    getOK() {

Wonder if we can come up with a better name here.

@@ +221,5 @@
>      },
>  
> +    getOK() {
> +      return this.args.ok;
> +    },

Nit: space between methods

Comment 16

[:Gijs (he/him)]

Comment on attachment 8838688 [details] [diff] [review]
0001-Bug-1333599-test-correct-URL-after-onbefureunload-.patch

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

It looks like you didn't hg add the test or its support file so I can't review this. :-(

Comment 17

[Frederik Braun [:freddy]]

I've been able to address your comments, though I was rather unsure about the code at https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/browser/base/content/tabbrowser.xml#5181-5188 you were pointing to.
This seems to be about switching context to the relevant tab and switching focus, right?
AFAIU this is for a visual change that does not need user attention and we just want to ensure to switch context so URLBarSetURI(); touches the correct URL Bar. I wasn't sure which was which, so I might have done the wrong thing as I couldn't completely figure out what it's doing.

The next iteration comes with a test that's actually _in the patch file_. I tried starting with the test you suggested (https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/urlbar/browser_urlbar_locationchange_urlbar_edit_dos.js), but it's really immature. I've been testing manually for the patch, so far.
I'm getting some failures about unexpected tabs that are ought to be closed, which I find rather confusing, given this test does not close the tabs it opens either. I'm pretty confused, I think I could use some additional guidance.

Comment 18

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1333599-reset-URL-after-onbefureunload-modal-dia.patch

Comment 19

[Frederik Braun [:freddy]]

Attachment: 0002-Bug-1333599-test-URL-after-onbefureunload-modal-dial.patch

Comment 20

[:Gijs (he/him)]

Comment on attachment 8839091 [details] [diff] [review]
0001-Bug-1333599-reset-URL-after-onbefureunload-modal-dia.patch

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

::: browser/base/content/tabbrowser.xml
@@ +5162,5 @@
> +
> +            // Focus window for beforeunload dialog so it is seen but don't
> +            // steal focus from other applications.
> +            if (Services.focus.activeWindow)
> +              window.focus();

I don't think you need the focus stuff, sorry - I meant to highlight only the lines above to get the tab (and thereby browser) for the event.

@@ +5168,5 @@
> +            // FREDDY WILL REMOVE THIS BEFORE LANDING
> +            dump("\nZZZZZZZ yay our case is being hit. lets reset the awesomebar\n");
> +            // FREDDY WILL REMOVE THIS BEFORE LANDING
> +
> +            event.originalTarget.userTypedValue = null;

You'll want to set tabForEvent.linkedBrowser.userTypedValue - originalTarget only points to the browser when in e10s mode.

::: browser/modules/RemotePrompt.jsm
@@ +46,4 @@
>        // appendPrompt call below. In that case, newPrompt will be
>        // undefined. We set the needRemove flag to remember to remove
>        // it right after we've finished adding it.
> +      let wasCancelled = true; //CommonDialog defaults to OK being false

Nit: space after '//'

@@ +52,2 @@
>          tabPrompt.removePrompt(newPrompt);
> +      }

Nit: if we brace the if() part, we also brace the else part.

::: toolkit/components/prompts/src/nsPrompter.js
@@ +384,4 @@
>          if (!newPrompt && !forceCleanup)
>              return;
>          callbackInvoked = true;
> +        let wasCancelled = true; //CommonDialog defaults to OK being false

Nit: space after '//'.

Comment 21

[:Gijs (he/him)]

Comment on attachment 8839092 [details] [diff] [review]
0002-Bug-1333599-test-URL-after-onbefureunload-modal-dial.patch

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

I think your test will need:

SpecialPowers.pushPrefEnv({"set": [["dom.require_user_interaction_for_beforeunload", false]]});

to allow the beforeunload dialog to come up.

As for the extra tabs, I think the window.open() stuff is probably creating an extra tab. Browser tests often open tabs, and they're supposed to close all of them again before the next test runs to avoid leaving 'dirty state' for the next test that runs (we don't restart the browser between tests as that would be too slow).

::: browser/base/content/test/urlbar/browser_bug1333599.js
@@ +1,4 @@
> +"use strict";
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "BrowserTestUtils",
> +  "resource://testing-common/BrowserTestUtils.jsm");

BTU is always available, so you don't need this.

@@ +23,5 @@
> +        resolve();
> +      }, "tabmodal-dialog-loaded", false);
> +    });
> +
> +    yield promptsOpenedPromise;

I think you can use TestUtils.topicObserved() to simplify this a little (from https://dxr.mozilla.org/mozilla-central/source/testing/modules/TestUtils.jsm which should also be loaded by default). Then if you use:

let [dialogWin] = yield TestUtils.topicObserved("tabmodal-dialog-loaded");
dialogWin.ui.button0.click(); // should cancel the dialog, I think, see https://dxr.mozilla.org/mozilla-central/source/layout/base/tests/browser_disableDialogs_onbeforeunload.js -- if not, try button1 :-)

and then check the location bar value, e.g. is(gURLBar.value, url, "Should be showing old URL instead of 'mochi.test' again");

or something along those lines? Best doublecheck that that actually 'fails' if you disable the code your patch is adding though.

Comment 22

[Frederik Braun [:freddy]]

Hm, I tested manually and with the mochitest, but went back to testing manually because the mochitest wasn't working so well.
Until I realized that the whole onbeforeunload prompt was gone when testing manually.
I then tried to get the prompt back by modifying my patch, which failed and probalby took way to long.
In the end, I went back to mozilla-central without any patches. This als does not give me any prompt.
I then supposed there was a config entry I was missing. Turns out my "production" nightly, that I use daily also does not give me the modal prompt.

Some recent work must have broken the onbeforeunload prompt.

Comment 23

[:Gijs (he/him)]

(In reply to Frederik Braun [:freddyb] from comment #22)
> Hm, I tested manually and with the mochitest, but went back to testing
> manually because the mochitest wasn't working so well.
> Until I realized that the whole onbeforeunload prompt was gone when testing
> manually.
> I then tried to get the prompt back by modifying my patch, which failed and
> probalby took way to long.
> In the end, I went back to mozilla-central without any patches. This als
> does not give me any prompt.
> I then supposed there was a config entry I was missing. Turns out my
> "production" nightly, that I use daily also does not give me the modal
> prompt.
> 
> Some recent work must have broken the onbeforeunload prompt.

We have a lot of tests for that prompt... both with and without e10s, on my nightly on OS X, just loading:

data:text/html,<body onbeforeunload="return false">

and clicking on the empty page (to satisfy dom.require_user_interaction_for_beforeunload ), I can see a dialog.

Comment 24

[Frederik Braun [:freddy]]

I'm off for the rest of this week and the week after. I'd be glad if someone could take this simple bug forward in the meantime.
If not, I'll pick it up again next week.

Comment 25

[:Gijs (he/him)]

(In reply to Frederik Braun [:freddyb] from comment #24)
> I'm off for the rest of this week and the week after. I'd be glad if someone
> could take this simple bug forward in the meantime.
> If not, I'll pick it up again next week.

Hi Freddy, sorry I haven't had cycles to take this on myself. Are you OK to pick this back up again?

Comment 26

[Frederik Braun [:freddy]]

Not before the end of the quarter, sorry.

Comment 27

[Frederik Braun [:freddy]]

(In reply to Frederik Braun [:freddyb] from comment #26)
> Not before the end of the quarter, sorry.

Comment 28

[:Gijs (he/him)]

This was disclosed via twitter by the reporter, https://twitter.com/lbherrera_/status/1280617786088329220, using a different testcase: https://spoof.lbherrera.me/ .

I'm about to sign off for the day so I can't investigate this right now, but I'll just say that that testcase works very well (the only giveaway is the lack of an identity box icon), whereas the testcase from comment #0 is a 404 and the testcase from comment 6 doesn't work, even if I allow popups - the beforeunload dialog is not automatically dismissed, and if I click "leave page" I just go to google.

The new testcase works by forcing an image href to change so that a load event fires on it, at which point the page navigates to a redirect. This was more recently reported somewhere else, I think, but I don't have a bug reference handy.

Comment 29

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #28)

The new testcase works by forcing an image href to change so that a load event fires on it, at which point the page navigates to a redirect. This was more recently reported somewhere else, I think, but I don't have a bug reference handy.

Oh, here we go, bug 1481994, from the same reporter.

Pinging Dan to radar this, I guess. Maybe we should just dupe forward at this point?

Comment 30

[Daniel Veditz [:dveditz]]

Attachment: saved testcase from comment 6

Saving the testcase from comment 6 so it doesn't 404 like the one from comment 0

Comment 31

[Daniel Veditz [:dveditz]]

Kinda "worksforme" and still kind of broken.

even if I allow popups - the beforeunload dialog is not automatically dismissed, and if I click "leave page" I just go to google.

If you click "Stay on page" then the URL bar is not updated so that's still spoofy, but that's captured more specifically in bug 1651636 now. For purposes of this spoof it breaks the illusion to have the user click "Stay" and then try to make them believe they've navigated, although I bet some people would assume they clicked the wrong button or the button was broken.

In terms of separate things to fix this bug isn't doing us any good so sure, duping to the bug with the currently-better testcase is fine.