771294 - (CVE-2013-5611) Install App doorhanger remains visible when installing page immediately loads another page

Status: RESOLVED FIXED

(Firefox Graveyard :: Web Apps, defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

If a page loads another page right after calling navigator.mozApps.install, the Install App doorhanger doesn't disappear when the browser tab's location changes, which could enable a malicious page to fool a user into thinking they're installing an app from a different page than the one that actually triggered the install.

Steps To Reproduce:

1. go to http://www.mykzilla.org/app/
2. press the "install, then redirect immediately" button

Expected Results: the doorhanger appears and then immediately disappears (perhaps too quickly to see it at all) as the tab starts loading about:blank.

Actual Results: the doorhanger appears, the tab loads about:blank, and the doorhanger remains visible.

In theory, the doorhanger should disappear when the location changes, because XULBrowserWindow.onLocationChange calls PopupNotifications.locationChange, which closes doorhangers unless they set persistWhileVisible, and webappsUI doesn't do that <http://hg.mozilla.org/mozilla-central/file/b39f4007be5a/browser/modules/webappsUI.jsm#l139>.

So perhaps the location change races the notification?  If the installing page waits a second before redirecting the user to about:blank, the doorhanger closes as expected (see the "call install, then redirect after one second" button in my test app for an example of this).

Comment 1

[:Felipe Gomes (needinfo for replies!)]

It doesn't even race, the location change always arrives before the webapps-ask-install notification. I've got a patch and I'm writing a test for it.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

(In reply to Felipe Gomes (:felipe) from comment #1)
> It doesn't even race, the location change always arrives before the
> webapps-ask-install notification.

Erm, that's what I meant by racing!  The web page calls install() first, but the browser observes the location change first.


> I've got a patch and I'm writing a test for it.

Awesome!

Comment 3

[Jason Smith [:jsmith]]

https://bugzilla.mozilla.org/buglist.cgi?keywords=productwanted%20qawanted%20meta;keywords_type=nowords;priority=--;resolution=---;component=Web%20Apps;component=Webapp%20Runtime;product=Firefox;order=bug_id;list_id=3635911

Comment 4

[Jason Smith [:jsmith]]

Copy and paste fail on my part on comment 3. Meant to say let's due a k9o review of this bug and then reassess if this blocks the 1st release of the runtime or not.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

So fixing this bug was more involved than initially thought. Since the location change arrives before the ask-install notification, the approach I was taking was to compare the URL from the original request to the current browser URL. This however would prevent mozApps from working inside iframes [1], so I couldn't use that.

The solution was to get the innerWindowId from the original request and check if that window still exists, because if it navigates away the id will change. GetInnerWindowID existed in nsGlobalWindow but wasn't exposed in nsIDOMWindowUtils like it's sibling function GetOuterWindowID, so I had to add that, and also send the inner id with the install request.

Patches incoming.


[1] It's possible that we in fact want to block that, but that would be a discussion for a different bug, and certainly not a block to be done by the UI code (as this is a bug about how doorhangers interact with the ask-install notification).

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 1 - fix the prob

This is the bug fix, where we get the window using the inner id and check if it still exists and also do a sanity check for the URL.

I still find it a bit disturbing that the location change happens first and that we need to do this check since we don't have an original reference to the window that requested this (because the messages go through the message manager), but it seems a proper fix.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 2 - add nsIDOMWindowUtils.getInnerWindowId

This adds nsIDOMWindowUtils.getInnerWindowId which we need to use to fix this problem (to get a reference to the iframe that requested the app install).

That function already existed in nsGlobalWindow, like getOuter..., but it wasn't exposed in domwinutils

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 3 - tests

Myk, can you take a look at these tests change? The new test is similar to the one you wrote for bug 765063. I also had to modify your test, because this bug superseded the test for the other one (because a redirect won't show up the install doorhanger anymore). However, the change from bug 765063 is still valid by itself, so I found a different way to test it, using history.pushState instead of a redirect. Also I changed the listener from the observer to a message listener because the object received by the listener has more info than the one received by the observer.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 640969 [details] [diff] [review]
Part 3 - tests

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

::: dom/tests/mochitest/webapps/test_bug_771294.xul
@@ +21,5 @@
> +
> +popupNotifications.panel.addEventListener("popupshown", panelListener, false);
> +
> +setTimeout(function verify() {
> +  ok(true, "Install panel was blocked after immediate redirect");

This test should directly observe the panel being blocked (or shown), as timeouts like these have a history of unreliability.  Based on your change to webappsUI.jsm and my reading of webappsUI.doInstall(), PopupNotifications.show(), and various bits of platform code, it looks like the test should be able to observe "webapps-ask-install", spin the event loop once (i.e. set a zero millisecond timeout), and then fail if a "popupshowing" event has been dispatched.

(If that doesn't work, then another route would be to add support to PopupNotifications.jsm for determining that a doorhanger is going to be shown, just as it produces backgroundShow and updateNotShowing notifications for help in testing.)

Comment 10

[:Felipe Gomes (needinfo for replies!)]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #9)
> This test should directly observe the panel being blocked (or shown), as
> timeouts like these have a history of unreliability.  Based on your change
> to webappsUI.jsm and my reading of webappsUI.doInstall(),
> PopupNotifications.show(), and various bits of platform code, it looks like
> the test should be able to observe "webapps-ask-install", spin the event
> loop once (i.e. set a zero millisecond timeout), and then fail if a
> "popupshowing" event has been dispatched.

I tried to do this, but spinning the event loop once doesn't work, presumably because there are more events queued during a page redirect. I had to spin it 3 times to make the test work but then that becomes fragile.  The problem is that we are testing that nothing happened, so that's why I chose the 1sec timeout over calling setTimeout 3 times.

> 
> (If that doesn't work, then another route would be to add support to
> PopupNotifications.jsm for determining that a doorhanger is going to be
> shown, just as it produces backgroundShow and updateNotShowing notifications
> for help in testing.)

This won't work either because with the fix, doInstall() and consequently PopupNotifications.show() won't even be called, so there's no information available in PopupNotifications.jsm that will tell that the popup is about to be displayed.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 640966 [details] [diff] [review]
Part 2 - add nsIDOMWindowUtils.getInnerWindowId

One of the core properties of inner windows as designed when we originally implemented them was that an inner window itself is *never* referenceable from JS. Any time we wrap an inner window and expose it to JS, we "outerize", and expose its outer window. This patch attempts to violate that, and the fact that this the fix for this bug (that uses this patch) "works", assuming it does, means we're violating that assumption, and that's not something I want to change w/o significant reason. Granted this is a chrome only API, but still...

Blake/Bobby, whether or not we take this fix as is, and I'm arguing we don't, does this show we have a problem where we're able to wrap an inner window w/o actually outerizing in the wrapping process? Or is that no longer necessary with the new wrappers etc?

Felipe, could we fix this some other way? If nothing else, a narrower API seems like it would work here, i.e. windowUtils.isWindowIdFrom(windowID, url), or something like that.

Comment 12

[Bobby Holley (:bholley)]

jst is right - what this patch attempts to do is verboten. But I don't think it actually does anything, and I'm pretty sure that we properly outerize the window when it gets reflected into JS in NativeInterface2JSObject:

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#1014

The reason I don't believe that this "works" is that the only thing the patch does with the inner window is access the location object, and the location object actually corresponds to the _outer_ window. There's one location object per inner window, but at any given time they all behave the exact same way (because window.location is a property of navigation, which corresponds to the outer window). So I'm pretty sure that the check in this patch could be identically written as:

if (data.from == chromeWin.location.href)

Comment 13

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

Comment on attachment 640965 [details] [diff] [review]
Part 1 - fix the prob

Sounds like we need to address jst/bholley's comments here.

I think you mentioned that this approach was intended to properly handle installation prompts inside subframes - do we need to support those at all? It's a bit of a spoofing risk, and I'm not sure that we support it for other types of permission prompts.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Attachment: New fix

Thanks for the explanation, turns out I didn't need the inner window. That chromeWin variable confused me so I was associating outer window = top level, inner window = iframe, which is obviously not the case. The outer window id received in the message is still the id for the subframe, so I can use that.

I simplified the code around here a bit and removed the _getBrowserForId function

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Just to elaborate: I had tried that exact line as bholley suggested, but it didn't work. I too was expecting it to work, but looking more into it, chromeWin, as now I realize the name clearly says, is the chrome window (i.e. browser.xul) not the window object from where navigator.mozApps was called.


Myk: any ideas of alternate approaches for the test, or would you reconsider review given comment 10?

Comment 16

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

Comment on attachment 641576 [details] [diff] [review]
New fix

Might be a bit more readable to keep a _getWindowForId helper.

two other nits:
- not related to this bug, but bundle.getFormattedString doesn't take a third argument (<stringbundle>.getFormattedString takes two arguments, nsIStringBundle.formatStringFromName takes 3 - horribly confusing)
- could adjust the indentation to go along with the chromeWin.PopupNotifications change

Nice cleanup!

One stupid idea for testing this is that you can have the bailout in webappsUI_observe (when win.location.href != data.from) fire an observer event, and have the test watch for that. A little bit gross to have test-only code like that, but perhaps the benefits of reliable test coverage outweigh the grossness :)

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Attachment: alternative test

(In reply to Felipe Gomes (:felipe) from comment #10)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #9)
> > (If that doesn't work, then another route would be to add support to
> > PopupNotifications.jsm for determining that a doorhanger is going to be
> > shown, just as it produces backgroundShow and updateNotShowing notifications
> > for help in testing.)
> 
> This won't work either because with the fix, doInstall() and consequently
> PopupNotifications.show() won't even be called, so there's no information
> available in PopupNotifications.jsm that will tell that the popup is about
> to be displayed.

But that's ok, because then you can test that PopupNotifications.show() wasn't called.  Here's a patch that demonstrates what I'm suggesting.  In my tests, it correctly fails without the fix.  I haven't yet confirmed that it passes with the fix, although I would expect it to.

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 641660 [details] [diff] [review]
alternative test

I verified that the test works. Gavin, Myk's test is similar to what you suggested, but instead the notification is dispatched from PopupNotifications.jsm instead of webappsUI.jsm. Does it sound fine to you?

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Attachment: Fix to land

Patch that I will land, fixed all the nits. Carrying forward r+.

Comment 20

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

Comment on attachment 641660 [details] [diff] [review]
alternative test

r=me on the popupnotifications.jsm change, and the general approach. I didn't look at the rest of the test.

Comment 21

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69a1fb2ddca3

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/20a09c21d3d5 due to making test_cross_domain.xul fail. The test probably needs a fix similar to the fix to test_bug_765063.xul that was included in this patch.

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Relanded. The test failure that happened on the previous push was actually an existing orange. I pushed the same patch to try and ran the tests twice and the random failure didn't happen, so I don't think it is the patch that caused it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1924fe55fb6e

Comment 24

[Ed Morley [:emorley]]

(In reply to :Felipe Gomes from comment #23)
> Relanded. The test failure that happened on the previous push was actually
> an existing orange. 

Unfortunately it wasn't an existing orange, the only bug that shows up in TBPL is:
"Bug 754578 - [SeaMonkey] mochitest-chrome: "test_cross_domain.xul | Test timed out" (+ 3+ more)"

Which is for seamonkey only and for a different failure. The failure we're seeing due to this bug is:

9590 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_cross_domain.xul | #1343972016077# is expected to be true per template # > Date.now() - 3000#

> I pushed the same patch to try and ran the tests twice
> and the random failure didn't happen, so I don't think it is the patch that
> caused it.

The failures seem more sporadic than that, so I suspect there would need to be half a dozen retriggers on {osx 10.6 debug,osx 10.7 debug, winxp opt} (judging by the most frequently failing platforms) on try before this can reland.

Some logs from failing runs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=14085274&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=14084611&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=14087171&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=14086812&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=14087030&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=14088291&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=14088746&tree=Mozilla-Inbound
WinXP: https://tbpl.mozilla.org/php/getParsedLog.php?id=14089886&tree=Mozilla-Inbound

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f633b07099d9

Comment 25

[Ed Morley [:emorley]]

Just spotted that one of the tests that landed in this bug appears to be intermittent:
{
10216 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_bug_771294.xul | Install panel was blocked after immediate redirect 
}

https://tbpl.mozilla.org/php/getParsedLog.php?id=14090474&tree=Mozilla-Inbound

Comment 26

[Myk Melez [:myk] [@mykmelez]]

Felipe: are you planning to pick this back up at some point?  (Just wondering whether or not to look for someone else to do so.)

Comment 27

[Myk Melez [:myk] [@mykmelez]]

Marco: can you look at this?

Comment 28

[Marco Castelluccio [:marco]]

Attachment: Patch

I've basically used Fabrice's patch, I've triggered a try run and it works. I've just modified the tests a bit.

Comment 29

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 783205 [details] [diff] [review]
Patch

(In reply to Marco Castelluccio [:marco] from comment #28)
> I've basically used Fabrice's patch, I've triggered a try run and it works.
> I've just modified the tests a bit.

Erm, do you mean Felipe's patch?

This patch doesn't apply; does it depend on a patch in another bug?

We'll need a toolkit peer to review the PopupNotifications.jsm change.

Comment 30

[Marco Castelluccio [:marco]]

> Erm, do you mean Felipe's patch?

Yes, lapsus :D

> This patch doesn't apply; does it depend on a patch in another bug?

Probably the changes in webappsUI.jsm, I've a really long local queue and I'm basically working as if bug 777402 already landed. I'll post an updated patch soon.

> We'll need a toolkit peer to review the PopupNotifications.jsm change.

In the meantime a toolkit peer could review that small change.

Comment 31

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 783205 [details] [diff] [review]
Patch

Felipe: you reviewed the last three changes to PopupNotifications.jsm; perhaps you can review this change to that file?  I can review the rest of the patch; I just need review of that toolkit/ change from a Toolkit peer.

Comment 32

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

Why is the PopupNotifications.jsm change needed? Can't you just use a popupshowing observer on the relevant panel instead?

Comment 33

[Marco Castelluccio [:marco]]

Attachment: Patch v2

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #32)
> Why is the PopupNotifications.jsm change needed? Can't you just use a
> popupshowing observer on the relevant panel instead?

You're right, thank you!
So this doesn't need a toolkit peer review anymore.

Comment 34

[Marco Castelluccio [:marco]]

(Note that the patch still depends on other changes, so it doesn't cleanly apply)

Comment 35

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 783880 [details] [diff] [review]
Patch v2

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

(In reply to Marco Castelluccio [:marco] from comment #34)
> (Note that the patch still depends on other changes, so it doesn't cleanly
> apply)

Those other changes appear to have landed, as it now applies cleanly.  And it looks good and works as expected: the test fails without the change and passes with it. r=myk

::: dom/tests/mochitest/webapps/test_bug_765063.xul
@@ +28,3 @@
>  
> +    var msg = aMessage.json;
> +    var iOService = Components.classes["@mozilla.org/network/io-service;1"]

Nit: iOService -> ioService per convention.

Comment 36

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Carrying r+

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3879c642ac51

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3879c642ac51

Comment 39

[Al Billings [:abillings - ex-MoCo]]

I assume ESR24 and ESR17 are unaffected by this?

How about 24 and 25 in general?

Comment 40

[Marco Castelluccio [:marco]]

They should all be affected.

Comment 41

[Alex Keybl [:akeybl]]

This is sec-moderate, so we're going to pass for ESR24. If there's any reason to reconsider, please let us know.

Comment 42

[Daniel Veditz [:dveditz]]

navigator.mozApps is not enabled on ESR-24