765063 - navigator.mozApps.install can be fooled into using an incorrect installing origin

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

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

Attachment: screenshot

See bug 745928 comment 4. The "origin" used to determine the installation origin is retrieved from the window after the XHR to retrieve the manifest has been fetched, during which the window could have been navigated to anywhere.

There's a working testcase at http://app.g4v.org/, see screenshot of the produced doorhanger attached.

That page just does:
var request = navigator.mozApps.install("http://app.g4v.org/app.webapp");
document.location = 'about:blank';

(I chose about:blank because it's quick to load, but obviously that could be anything, and you could play tricks with delaying the manifest load to control things even further.)

Comment 1

[Jason Smith [:jsmith]]

Confirmed on desktop. Does not reproduce on Android though (cause Android does not show the location of the origin of where the app was installed from.

Comment 2

[Jason Smith [:jsmith]]

Also - this is a front-end bug with desktop web apps (so this belongs in firefox --> web apps).

Comment 3

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

No, this is a fundamental issue with the API - the specific symptom I mention in comment 0 isn't the only issue (and probably isn't the worst).

This is a core bug in the DOM API, so it belongs in DOM.

Comment 4

[Jason Smith [:jsmith]]

Let me backup and ask this question: What's affected by this then? What ends up being fooled here outside of the web apps doorhanger? Does this affect B2G? Does this affect the return values of the mozapps API after a call to install?

Better overarching question - what's the variable being referenced that holds the value of about:blank in that screenshot?

Comment 5

[Ian Bicking (:ianbicking)]

I believe because of this bug an attacking page can install (with the user's confirmation) any arbitrary manifest it hosts, and receipts passed to install(), for any origin (i.e., not just for the attacking page's origin).

Comment 6

[Jason Smith [:jsmith]]

We need to fix this for the 1st release of desktop web runtime

Comment 7

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

Attachment: patch v1: set install URL/origin before initiating XHR

I'm having trouble reproducing the problem automatically.  I wrote a test that should do so, but when I run the test without fixing the bug, installation fails with the error:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Failure arg 0 [nsIDOMRequestService.fireError]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///Users/myk/Mozilla/x86_64-apple-darwin11.4.0-dbg/dist/NightlyDebug.app/Contents/MacOS/components/Webapps.js :: <TOP_LEVEL> :: line 146"  data: no]
************************************************************

The original exception that triggers that one is:

  TypeError: this._window is null

Which is presumably triggered by the reference to this._window.location.href on line 130 of dom/apps/src/Webapps.js.


Nevertheless, the fix in this patch looks correct, and the test passes with it, and it also works in my manual testing using Gavin's testcase, so it seems to fix the bug.

After building the appropriate directories, I run the automated test with:

make mochitest-chrome TEST_PATH=dom/tests/mochitest/webapps/test_bug_765063.xul


Note: it'd also be worth investigating why the doorhanger doesn't disappear as soon as the page location changes, which would prevent users from installing the app in the first place and seems worth doing, since otherwise the malicious site might still fool the user into thinking they're installing an app from a different site, even if this patch prevents the malicious site from fooling the webapp registry.

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>.

Perhaps the location change races the notification?  I added two buttons to my test app:

  call install, then redirect immediately
  call install, then redirect after one second

  - http://www.mykzilla.org/app/

And the doorhanger sticks around when the first one redirects immediately, but it goes away when the second one redirects after one second.

Comment 8

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

Comment on attachment 638948 [details] [diff] [review]
patch v1: set install URL/origin before initiating XHR

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa750527a43b

Comment 9

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #7)
> Note: it'd also be worth investigating why the doorhanger doesn't disappear
> as soon as the page location changes, which would prevent users from
> installing the app in the first place and seems worth doing, since otherwise
> the malicious site might still fool the user into thinking they're
> installing an app from a different site, even if this patch prevents the
> malicious site from fooling the webapp registry.

I filed bug 771294 on this issue.  It too is marked Security-Sensitive, and I cc:ed a few folks who are cc:ed to this bug, but let me know if you aren't cc:ed and would like to be (and can't cc: yourself).

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/654181827f06

(resolve test bustage caused by fix for this bug)

Comment 11

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

Landed on central.  Will request uplift to aurora after it bakes a couple days.

https://hg.mozilla.org/mozilla-central/rev/fa750527a43b
https://hg.mozilla.org/mozilla-central/rev/654181827f06

Comment 12

[Jason Smith [:jsmith]]

Verified on Nightly.

Comment 13

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

I was going to request uplifting this to Aurora, but in bug 772638 the desktop runtime drivers propose disabling webapps in Firefox 15, which would make uplift unnecessary, so I'm going to wait on the outcome of that.

Comment 14

[Jason Smith [:jsmith]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #13)
> I was going to request uplifting this to Aurora, but in bug 772638 the
> desktop runtime drivers propose disabling webapps in Firefox 15, which would
> make uplift unnecessary, so I'm going to wait on the outcome of that.

What about B2G? Do they need the uplift to FF 15? Jonas do you know?

Note - this patch affects multiple platforms, so we have to check to see if the B2G team wants this uplifted or not.

Comment 15

[Mounir Lamouri (:mounir)]

(In reply to Jason Smith [:jsmith] from comment #14)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #13)
> > I was going to request uplifting this to Aurora, but in bug 772638 the
> > desktop runtime drivers propose disabling webapps in Firefox 15, which would
> > make uplift unnecessary, so I'm going to wait on the outcome of that.
> 
> What about B2G? Do they need the uplift to FF 15? Jonas do you know?

B2G isn't a released product yet. There is no point in uplifting this for it. B2G will use a Gecko version higher than 15 anyway.

Comment 16

[Alex Keybl [:akeybl]]

Marking as unaffected for 15 given bug 772638.

Comment 17

[Josh Aas]

Can we make this bug publicly visible now?