960135 - (CVE-2014-1501) local file access via Open Link in new Tab

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[is101013]

Attachment: clickj3.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

Create a link which is using the file:/// uri
<a href="file:///">clickme</a>

Now keep the finger on the hyperlink until a pop appears. Click on open in new tab.
A new tab which points to file:/// will be opened


Actual results:

A new tab was openend with the local file path.
If you know the path of a local file (as an example by dropping a prepared html file via a download) an attacker would be able to open and execute a local file.


Expected results:

The expected result should be either an empty (about:blank) new tab or no tab at all.
If you right click on the same hyperlink on the the desktop version of firefox and you choose open in new tab, nothing happens.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Confirmed on Nexus 5, this happens on all releases.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Also reproducible with "Open Link in Private Tab".

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: (WIP) Security check 'Open in New Tab' urls.

This WIP correctly prevents "file:///" links from opening by mimicking the
approach desktop takes [1]. However, I'm unsure of a few things in my
implementation (left as TODO items in the code):
  1) Desktop takes the principal, and "unremotes" it if it was sent by a child
process [2] (which I think means plugins). However, there's no precedent for
similar code in fennec's browser.js (i.e. no use of gContextMenuContentData to
determine if the code was remote and such). I assume we should also be
concerned about this, however, is gContextMenuContentData also accessible from
fennec's browser.js? Additionally, I wouldn't know how to set up a test case
for this.
  2) A previous use of urlSecurityCheck in fennec's browser.js used the
Ci.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL flag [3] [4]. I don't
really understand the implications - is this flag necessary for my usage? I
presume not as desktop does not use it.
  3) The GeckoConsole output when tapping on links is:

01-16 15:11:43.608 E/GeckoConsole( 1290): Security Error: Content at http://10.251.26.149:8000/file.html may not load or link to file:///default.prop.

The GeckoConsole output when long pressing for "Open Link in New Tab" is:

01-16 15:10:08.546 E/GeckoConsole( 1290): [JavaScript Error: "[Exception... "'Load of file:///default.prop from http://10.251.26.149:8000/file.html denied.' when calling method: [nsIObserver::observe]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]"]

Since I do not catch the thrown exception from urlSecurityCheck. Should I catch
the exception to prevent this information from being leaked?

[1]: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js?rev=94a8be105e2d#842
[2]: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js?rev=94a8be105e2d#793
[3]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=f5ce7ba93846#3635
[4]: https://mxr.mozilla.org/mozilla-central/source/caps/idl/nsIScriptSecurityManager.idl?rev=595fd5dfbb5a#59

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8361357 [details] [diff] [review]
(WIP) Security check 'Open in New Tab' urls.

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

Please help with the questions in comment 3. Feel free to pass them off if you think there's someone else to better handle them.

Comment 5

[Mark Goodwin [:mgoodwin]]

(In reply to Michael Comella (:mcomella) from comment #3)
> Created attachment 8361357 [details] [diff] [review]

Re. questions 1 and 2; I can guess but I don't know. I'll needinfo dan.

>   3) The GeckoConsole output when tapping on links is:
> 
> 01-16 15:11:43.608 E/GeckoConsole( 1290): Security Error: Content at
> http://10.251.26.149:8000/file.html may not load or link to
> file:///default.prop.
> 
> The GeckoConsole output when long pressing for "Open Link in New Tab" is:
> 
> 01-16 15:10:08.546 E/GeckoConsole( 1290): [JavaScript Error: "[Exception...
> "'Load of file:///default.prop from http://10.251.26.149:8000/file.html
> denied.' when calling method: [nsIObserver::observe]"  nsresult: "0x8057001e
> (NS_ERROR_XPC_JS_THREW_STRING)"  location: "native frame :: <unknown
> filename> :: <TOP_LEVEL> :: line 0"  data: no]"]
> 
> Since I do not catch the thrown exception from urlSecurityCheck. Should I
> catch
> the exception to prevent this information from being leaked?

I don't think so.

Comment 6

[Daniel Veditz [:dveditz]]

(In reply to Michael Comella (:mcomella) from comment #3)
>   1) Desktop takes the principal, and "unremotes" it if it was sent by a
> child process [2] (which I think means plugins).

No, it means child process. This was added for e10s in bug 516753. unremoting is just trying to build a principal from information passed from the other process since the principal itself can't be passed directly. I don't think you need to worry about it.

>   2) A previous use of urlSecurityCheck in fennec's browser.js used the
> Ci.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL flag [3] [4]. I don't
> really understand the implications - is this flag necessary for my usage? I
> presume not as desktop does not use it.

This in practice means URLs that are data: or javascript:, though coded generically so that if some addon or future code invents a new scheme with similar properties we can handle them safely without having to rewrite lots of places in Firefox. There are certainly cases where loading those URLs will cause security problems and should not be allowed, but opening a link (whether in a new tab or not) is a case that is still expected to work.

Comment 7

[bhavana bajaj [:bajaj]]

I want to highlight that our final beta  for mobile goes to build on MOnday(1/27) so unless the upcoming patch is absolutely low risk and well tested by then we should wontfix this in Firefox27 and maintain the status-quo here and target to be resolved in Fx28 instead.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8361357 [details] [diff] [review]
(WIP) Security check 'Open in New Tab' urls.

I don't think we should put the urlSecurityCheck in _getLinkURL, which should stay single purposed.

I think we should call urlSecurityCheck in the two places you want to do the check. This matches the way nsContentMenu seems to work as well:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#834
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#844

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Security check 'Open in New Tab' urls.

(In reply to Mark Finkle (:mfinkle) from comment #8)
> I don't think we should put the urlSecurityCheck in _getLinkURL, which should
> stay single purposed.

Yeah, I guess that was silly of me. :)

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8364712 [details] [diff] [review]
Security check 'Open in New Tab' urls.

Good for me

Comment 11

[bhavana bajaj [:bajaj]]

We are going to build with our final mobile beta and our RC candidate soon today so this will have to be a wontfix this cycle, but please uplift the fix to aurora to resolve this on Fx28 once ready.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/14dcf72485b5

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8364712 [details] [diff] [review]
Security check 'Open in New Tab' urls.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unknown

User impact if declined:
  Malicious servers can access arbitrary files on the device if the user long presses and selects "Open in new tab" or "Open in new Private Tab". Presumably, these files may also be executed by Firefox (e.g. javascript).
 
Testing completed (on m-c, etc.):
  Local testing
 
Risk to taking this patch (and alternatives if risky):
  Minimal - we follow desktop's paradigm, reassuring us that the code is less likely to break, and if something broke, we would likely only breaking these two context menu items.
 
String or IDL/UUID changes made by this patch: None

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

See comment 11.

Comment 15

[Carsten Book [:Tomcat]]

landed on central as https://hg.mozilla.org/mozilla-central/rev/14dcf72485b5

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cecfc49900e7

Comment 17

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed bug in Fennec 29, 2014-01-05.
Verified fixed in Fennec 29, 2014-04-15.