791943 - navigator.mozApps.install can be used to enumerate local file names

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Calling navigator.mozApps.install with a file:// URI behaves differently depending on whether the file exists or not. A malicious webpage could use this enumerate local file names.

navigator.mozApps.install('file:///non-existant') throws a file not found error, where as if the file exists but can't be parsed, the onerror method of the domrequest is fired.

Its pretty slow though so a real world attack isn't too likely. There is a hacky PoC below which attempts to guess your username on a mac, but it probably only works if you have a three letter username ;)

http://untrusted.creativemisuse.com/testMozAppFileURI.html

Tested in 18.0a1 (2012-09-17)

Comment 1

[Jason Smith [:jsmith]]

Hmm...could this problem affect FF Android and B2G? Or will this only affect desktop?

Comment 2

[Daniel Veditz [:dveditz]]

this feels be higher than moderate -- if it can be speeded up you could spider someone's whole disk which might yield all kinds of juicy details. And there's probably some way to parallelize this.

Comment 3

[Jason Smith [:jsmith]]

Bill and Myk - We should find an owner to fix this. This sounds bad.

Comment 4

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

I suspect this does affect Android and B2G, although I haven't tested.  cc:ing a few more folks who can provide insight and/or a fix.

Comment 5

[Anant Narayanan [:anant]]

We should disallow installs for a manifest served by any protocol other than http or https on all platforms. The drawback of this is that it might make development a little harder as developers will have to setup a local server to test installs, but I think that's an acceptable tradeoff.

Comment 6

[Jason Smith [:jsmith]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #4)
> I suspect this does affect Android and B2G, although I haven't tested. 
> cc:ing a few more folks who can provide insight and/or a fix.

Okay. I'll put this on the nom list just in case for basecamp triage.

Comment 7

[Alex Keybl [:akeybl]]

It sounds like we'd like to fix this before FF16's release, and we may wontfix if a patch is ready later than Tuesday (beta 5's go to build) given the fact that this was internally found. Sending over to myk to help prioritize.

Comment 8

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

Attachment: patch v1: validate manifest/package URL scheme

Here's a patch that does what Anant suggests: restricts schemes from which apps can be installed to 'http' and 'https' by throwing an exception if the scheme is not one of those values.  The patch does this for both install() and installPackage().

Unfortunately, I don't know how to test installPackage(), neither automatically nor manually, but the patch includes mochitests for install().  I also ran other apps mochitests I know about (dom/tests/mochitest/webapps/, dom/apps/tests/, and extensions/cookie/test/), and they all passed.

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 663120 [details] [diff] [review]
patch v1: validate manifest/package URL scheme

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

I'm not sure if should throw an exception or fire an error on the DOMRequest, but let's land that and revisit that later if we need.

Comment 10

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

Attachment: patch v1 for aurora branch

Comment 11

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

Attachment: patch v1 for beta branch

Comment 12

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

dveditz: my proposed vague commit comment is:

  bug 791943 - validate install URL scheme; r=fabrice

Any suggestions on wording from a security perspective?  Also, is there a best practice for landing such patches on multiple branches (in terms of order in which patches are landed, timing of landings relative to spinning of builds, etc.)?

Comment 13

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

Comment on attachment 663120 [details] [diff] [review]
patch v1: validate manifest/package URL scheme

The new "security bug approval process" doesn't go into effect until the end of the month, but the flag is already live, and perhaps setting it will get this bug on the radar of security folks who can provide input into how best to land its patches.

[Security approval request comment]
How easily can the security issue be deduced from the patch?
-> It seems fairly easy to figure out the problem from the patch.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
-> The code comments and my proposed vague commit comment are ok, but the tests are fairly inculpatory.

Which older supported branches are affected by this flaw?
-> I think they all are, including the current stable release (Firefox 15).

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
-> I have backports for Aurora and Beta.  I imagine that a backport for Release would be easy to cook up.

How likely is this patch to cause regressions; how much testing does it need?
-> This patch is unlikely to cause real regressions, although I could imagine it causing a failure in some apps test I'm not aware of.  I ran all the apps tests I'm aware of, but I haven't pushed it to TryServer yet because it seems insecure to do so.

Comment 14

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

Attachment: patch v1 for release branch

Comment 15

[Jason Smith [:jsmith]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #14)
> Created attachment 663201 [details] [diff] [review]
> patch v1 for release branch

? release-branch? Why the release branch?

Comment 16

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

(In reply to Jason Smith [:jsmith] from comment #15)
> ? release-branch? Why the release branch?

The bug appears to exist in all branches, including the release branch.  Bug 772638 disabled the mozApps API in Firefox 15 for Desktop, so the bug doesn't affect that product.  But it isn't clear to me if it affects any other products and whether or not it's severe enough to warrant a hotfix release.

Nor am I the right person to make those determinations.  So I created the patch and requested approval to land it to get it on the radar of release and security drivers who can do so.

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 663201 [details] [diff] [review]
patch v1 for release branch

We're 2 weeks from release and the feature is disabled in 15, so no need to land or chemspill.

Comment 18

[Alex Keybl [:akeybl]]

Comment on attachment 663120 [details] [diff] [review]
patch v1: validate manifest/package URL scheme

Good to land on m-c, in preparation for m-a and m-b.

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 663141 [details] [diff] [review]
patch v1 for aurora branch

[Triage Comment]
Approving for Aurora 17 and Beta 16 given this is a low risk fix for a sec-critical bug (and we need to get this into beta 5 to fix in FF16).

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63643fbbde88
https://hg.mozilla.org/releases/mozilla-aurora/rev/4c6a240afef7
https://hg.mozilla.org/releases/mozilla-beta/rev/fa2b95b5030f

Please make sure that hg is configured to generate all the necessary metadata needed for checkin. It makes life easier for those checking in on your behalf.
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

Comment 21

[Ryan VanderMeulen [:RyanVM]]

http://mozillamemes.tumblr.com/post/21637966463/yes-i-have-a-condition-that-forces-me-to-insert
"Land it on inbound first, they said. Make sure it sticks there before landing bustage on other branches, they said..." Well, that was fun. Backed out on all branches for mochitest-other failures.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a2f1680ac8e
https://hg.mozilla.org/releases/mozilla-aurora/rev/a776a022281b
https://hg.mozilla.org/releases/mozilla-beta/rev/c71e253025fc

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

9461 INFO TEST-START | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul
9462 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | install returned Not enough arguments [mozIDOMApplicationRegistry.install]
9463 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | the error callback was called
9464 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | #MANIFEST_PARSE_ERROR# is expected to be true per template #== "MANIFEST_PARSE_ERROR"#
9465 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | the error callback was called
9466 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | #INVALID_MANIFEST# is expected to be true per template #== "INVALID_MANIFEST"#
9467 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | at least one notification displayed
9468 INFO TEST-KNOWN-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | the success callback was called
9469 INFO TEST-KNOWN-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | name is undefined as expected
9470 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | at least one notification displayed
9471 INFO TEST-KNOWN-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | the success callback was called
9472 INFO TEST-KNOWN-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | name is undefined as expected
9473 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | installPackage not in navigator.mozApps
9474 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | the error callback was called
9475 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | #DENIED# is expected to be true per template #== "DENIED"#
9476 INFO TEST-KNOWN-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | navigator.mozApps.mgmt.addEventListener is not a function
9477 INFO TEST-KNOWN-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | navigator.mozApps.mgmt.removeEventListener is not a function
9478 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | attempt to install nonexistent file: URL throws exception - INVALID_URL_SCHEME: 'file'; must be 'http' or 'https' should equal INVALID_URL_SCHEME: 'file'; must be 'http' or 'https'
NOTE: child process received `Goodbye', closing down
9479 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | Test timed out.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

The failure on aurora and beta was the same. It was different on inbound, however.

8635 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | an unexpected uncaught JS exception reported through window.onerror - NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIFileURL.file] at chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul:159

Comment 23

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

Attachment: patch v2: test existent file: URL behavior using file:///

(In reply to Ryan VanderMeulen from comment #20)
> Please make sure that hg is configured to generate all the necessary
> metadata needed for checkin. It makes life easier for those checking in on
> your behalf.
> https://developer.mozilla.org/en-US/docs/
> Creating_a_patch_that_can_be_checked_in

I generate patches using Git, and I'm not sure how to embed that information into a patch generated by Git, but I'll look into it!

In any case, I almost always commit my own changes, and I would have done so in this case too, if hg.mozilla.org hadn't repeatedly aborted my attempts to pull the various branches on Friday evening in preparation for landing the changes.


(In reply to Ryan VanderMeulen from comment #21)
> http://mozillamemes.tumblr.com/post/21637966463/yes-i-have-a-condition-that-
> forces-me-to-insert

Yes, I agree, and normally I do test changes on TryServer.  I only didn't do so in this case because it seems insecure.  I'm certainly happy to entertain ideas for how to do so securely!

Nevertheless, I'm sorry you had to go to the trouble of committing and then backing out the changes. :-/


> 9479 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/
> test_install_errors.xul | Test timed out.

(In reply to Ryan VanderMeulen from comment #22)
> The failure on aurora and beta was the same. It was different on inbound,
> however.
> 
> 8635 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/
> test_install_errors.xul | an unexpected uncaught JS exception reported
> through window.onerror - NS_NOINTERFACE: Component returned failure code:
> 0x80004002 (NS_NOINTERFACE) [nsIFileURL.file] at
> chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/
> test_install_errors.xul:159

Hmm, I can't reproduce this problem, neither on central nor on beta (still building aurora), on the Windows machine I'm currently using.  And the same tests also worked when I ran them on my Mac on Thursday.  So I don't know why they failed on the test machines.

Nevertheless, I can remove the code in question while still testing the behavior of a file: URL to an existent file by using the file: URL file:///, which points to the existent root of the filesystem (it doesn't matter that the URL points to a directory; its existence is sufficient to cause the behavior of the API to differ from its behavior for a nonexistent file).

Here's a patch for central that tests file:///.

Comment 24

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

Attachment: patch v2 for beta branch

Comment 25

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

Attachment: patch v2 for aurora branch

Comment 26

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #23)
> Hmm, I can't reproduce this problem, neither on central nor on beta (still
> building aurora), on the Windows machine I'm currently using.  And the same
> tests also worked when I ran them on my Mac on Thursday.  So I don't know
> why they failed on the test machines.

One possibility is that the tests are run against builds in which the JSMs have been packaged into the omnijar, and their resource: URLs are aliases for jar: URLs that don't implement nsIFileURL.file (but do still claim to implement nsIFileURL, for some reason).

Comment 27

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

Since the change was already pushed to public branches, I pushed the new patch to TryServer:

https://tbpl.mozilla.org/?tree=Try&rev=b700cd94718c

Comment 28

[Ryan VanderMeulen [:RyanVM]]

FYI, there was a recent newsgroup discussion about running s-s bugs through Try, and the belief was that the signal to noise ratio is low enough to make watching it for hacking fodder unproductive.

Comment 29

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

TryServer tests are looking good (modulo the usual orangefactors).

Comment 30

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

All builds/tests on the TryServer run came up green or orangefactor:

https://tbpl.mozilla.org/?tree=Try&rev=b700cd94718c

Comment 31

[Lukas Blakk [:lsblakk] use ?needinfo]

http://hg.mozilla.org/mozilla-central/rev/63643fbbde88

Comment 32

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

lsblakk: umm, that's a link to yesterday's landing (comment 20), which was then backed out (comment 21).  Did you mean to provide a different link?

Comment 33

[Ryan VanderMeulen [:RyanVM]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #30)
> https://tbpl.mozilla.org/?tree=Try&rev=b700cd94718c

Green on Try (music to my ears).

https://hg.mozilla.org/integration/mozilla-inbound/rev/59877fe3f1e2

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/59877fe3f1e2

Comment 35

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

Comment on attachment 664001 [details] [diff] [review]
patch v2 for aurora branch

https://hg.mozilla.org/releases/mozilla-aurora/rev/229a0c3a8f95

Comment 36

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

Comment on attachment 663999 [details] [diff] [review]
patch v2 for beta branch

https://hg.mozilla.org/releases/mozilla-beta/rev/ba26736aadf2