Closed
Bug 791943
Opened 12 years ago
Closed 12 years ago
navigator.mozApps.install can be used to enumerate local file names
Categories
(Core Graveyard :: DOM: Apps, defect)
Core Graveyard
DOM: Apps
Tracking
(blocking-basecamp:+, firefox15 disabled, firefox16+ fixed, firefox17+ fixed, firefox18+ fixed, firefox-esr10 unaffected)
People
(Reporter: pauljt, Assigned: myk)
Details
(Keywords: privacy, sec-high, Whiteboard: [qa-][advisory-tracking-])
Attachments
(3 files, 4 obsolete files)
2.23 KB,
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
3.54 KB,
patch
|
akeybl
:
approval-mozilla-beta+
myk
:
checkin+
|
Details | Diff | Splinter Review |
3.59 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
myk
:
checkin+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Hmm...could this problem affect FF Android and B2G? Or will this only affect desktop?
Comment 2•12 years ago
|
||
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.
Updated•12 years ago
|
Comment 3•12 years ago
|
||
Bill and Myk - We should find an owner to fix this. This sounds bad.
Assignee | ||
Comment 4•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
(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.
blocking-basecamp: --- → ?
Comment 7•12 years ago
|
||
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.
Assignee: nobody → myk
Assignee | ||
Comment 8•12 years ago
|
||
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.
Attachment #663120 -
Flags: review?(fabrice)
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Comment 9•12 years ago
|
||
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.
Attachment #663120 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #663141 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #663142 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 12•12 years ago
|
||
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.)?
Assignee | ||
Comment 13•12 years ago
|
||
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.
Attachment #663120 -
Flags: sec-approval?
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #663201 -
Flags: approval-mozilla-release?
Comment 15•12 years ago
|
||
(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?
Assignee | ||
Comment 16•12 years ago
|
||
(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.
Updated•12 years ago
|
blocking-basecamp: ? → +
Updated•12 years ago
|
status-firefox-esr10:
--- → unaffected
status-firefox15:
--- → disabled
status-firefox16:
--- → affected
status-firefox17:
--- → affected
status-firefox18:
--- → affected
Comment 17•12 years ago
|
||
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.
Attachment #663201 -
Flags: approval-mozilla-release? → approval-mozilla-release-
Comment 18•12 years ago
|
||
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.
Attachment #663120 -
Flags: sec-approval? → sec-approval+
Comment 19•12 years ago
|
||
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).
Attachment #663141 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #663142 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•12 years ago
|
Keywords: checkin-needed
Comment 20•12 years ago
|
||
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
Updated•12 years ago
|
Updated•12 years ago
|
Whiteboard: [qa-]
Comment 21•12 years ago
|
||
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•12 years ago
|
||
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
Assignee | ||
Comment 23•12 years ago
|
||
(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:///.
Attachment #663120 -
Attachment is obsolete: true
Attachment #663998 -
Flags: review?(fabrice)
Assignee | ||
Comment 24•12 years ago
|
||
Attachment #663142 -
Attachment is obsolete: true
Assignee | ||
Comment 25•12 years ago
|
||
Attachment #663141 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #663201 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 26•12 years ago
|
||
(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).
Assignee | ||
Comment 27•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 29•12 years ago
|
||
TryServer tests are looking good (modulo the usual orangefactors).
Updated•12 years ago
|
Attachment #663998 -
Flags: review?(fabrice) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #663999 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•12 years ago
|
Attachment #664001 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 30•12 years ago
|
||
All builds/tests on the TryServer run came up green or orangefactor:
https://tbpl.mozilla.org/?tree=Try&rev=b700cd94718c
Comment 31•12 years ago
|
||
Assignee | ||
Comment 32•12 years ago
|
||
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•12 years ago
|
||
(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
Flags: in-testsuite+
Comment 34•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Updated•12 years ago
|
Attachment #663999 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•12 years ago
|
Attachment #664001 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 35•12 years ago
|
||
Comment on attachment 664001 [details] [diff] [review]
patch v2 for aurora branch
https://hg.mozilla.org/releases/mozilla-aurora/rev/229a0c3a8f95
Attachment #664001 -
Flags: checkin+
Assignee | ||
Comment 36•12 years ago
|
||
Comment on attachment 663999 [details] [diff] [review]
patch v2 for beta branch
https://hg.mozilla.org/releases/mozilla-beta/rev/ba26736aadf2
Attachment #663999 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Updated•12 years ago
|
Whiteboard: [qa-] → [qa-][advisory-tracking-]
Updated•12 years ago
|
Group: core-security
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•