1435908 - (CVE-2018-5182) Web content can open local files and local folders using drag & drop event

Status: VERIFIED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Jordi Chancel]

Attachment: testcase-drag-drop-local-file-opening.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180128191252

Steps to reproduce:

-1: Open the Test case in Attachments with Firefox.
-2: Try to Drag and drop the link https://www.bankofamerica.com/ into the Addressbar or Tab bar.



Actual results:

Firefox loads the local folder: File:///etc/ .

(I will upload a demonstration video)


Expected results:

The testcase works on Firefox 58.0.1 and also on Firefox Beta 59.0b6.

Comment 1

[Jordi Chancel]

Attachment: testcase-drag-drop-local-file-opening-[reduced testcase].html

reduced testcase

Comment 2

[Jordi Chancel]

Attachment: video-demo.html

Here's the demo video.

Comment 3

[Daniel Veditz [:dveditz]]

flagging for a bounty as asked, but this rings a bell. Need to check whether this is a dupe of one qab submitted.

Comment 4

[Jordi Chancel]

More detailed information about Steps to reproduce of this local files / local folders access vulnerability:

A selected content 
(like these: // , //etc/ , //etc/ , //etc///////// , ...) 
on the attacker's webpage can open local files or local folders when this content is dragged and dropped into the addressbar or tab bar.

Comment 5

[Daniel Veditz [:dveditz]]

I was thinking of bug 1424107 and bug 1379842. Both are now fixed so obviously this is a separate case. Tooru probably knows what's going on here.

Comment 6

[Tooru Fujisawa [:arai]]

Currently we're checking the access based on the original string,
but apparently URIFixup that is applied later fixes "//etc/" to "file:///etc/".
so, I think we should check the access based on fixed-up string, inside contentAreaDropListener.
The change in bug 1435910 will help making this process easier.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Apply URL fixup in contentAreaDropListener.

Now ContentAreaDropListener.prototype._validateURI applies URI fixup before checking the uri.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8953059 [details] [diff] [review]
Apply URL fixup in contentAreaDropListener.

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

I added some comments to the approach in this patch.

However, I'm worried about this more generally. checkLoadURIStr tries to use fixup anyway (see https://searchfox.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#1048 and lower). But it apparently doesn't attempt to do fixup if creating the URI initially fails. I think that's the problem we should fix instead. That may also help bug 1427448. Unfortunately I don't know how much breaks, and pushing to try risks exposing this set of bugs.

::: dom/base/contentAreaDropListener.js
@@ +134,5 @@
> +        Ci.nsIURIFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP;
> +    let info = Services.uriFixup.getFixupURIInfo(uriString, fixupFlags);
> +    if (info.fixedURI) {
> +      uriStringToCheck = info.fixedURI.spec;
> +    }

Instead, can you define `uri` earlier and assign info.fixedURI to it, and then not call ioService.newURI if `uri` is non-null?

@@ +142,5 @@
>                        .getService(Components.interfaces.nsIIOService);
>      try {
>        // Check that the uri is valid first and return an empty string if not.
>        // It may just be plain text and should be ignored here
> +      uri = ioService.newURI(uriStringToCheck);

If we're here anyway, use Services.io instead and delete the manual getting of the ioservice.

@@ +158,5 @@
>      if (disallowInherit)
>        flags |= secMan.DISALLOW_INHERIT_PRINCIPAL;
>  
>      let principal = this._getTriggeringPrincipalFromDataTransfer(dataTransfer, false);
> +    secMan.checkLoadURIStrWithPrincipal(principal, uriStringToCheck, flags);

If we're guaranteed a uri instance here, shouldn't we be calling checkLoadURIWithPrincipal?

Comment 9

[:Gijs (he/him)]

To rephrase the top of comment 8 - I am not sure that only fixing the dnd case here is the best solution. The long-term solution is passing the right principal along so that docshell stops the load and then we can stop doing all these manual checks. :jkt is working on this elsewhere already, and we :arai recently fixed bug 1424107 - does that not help here? Do we not pass the triggering principal along correctly, or something?

Comment 10

[:Gijs (he/him)]

(sorry for spam)
The generic triggering principal bug :jkt is working on is bug 1374741.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Check non-URI items in nsScriptSecurityManager::CheckLoadURIStrWithPrincipal.

Okay, stopping early-return for non-URI case fixes the issue.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8953753 [details] [diff] [review]
Check non-URI items in nsScriptSecurityManager::CheckLoadURIStrWithPrincipal.

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

I'm really sorry, I think I must have been confused when I wrote my previous comment.

The nsScriptSecurityManager code will throw if you pass it something that isn't a URI. That means it'll already deny these loads. The only thing that changing nsScriptSecurityManager can do for non-valid URIs is be less strict than it is already (logically speaking - right now it throws, the thing we'd accomplish here is... making it also throw, just later. The change may still make sense for the webextension bug, I'm not sure, but that's a separate discussion.

The reason this patch works for the dnd case is that by removing the URI construction in contentAreaDropListener (which catches the cases where the URI isn't valid and returns early without doing the security check), we stop returning early. So now that code will throw (because checkLoadURI throws). That helps for this case, but it also means _validateURI will start throwing for all the things that aren't intended to be URIs, per the comment ("It may just be plain text and should be ignored here").

In other words, we could fix this bug by just removing only that block and leaving it at that... but what would that break? Is that comment just outdated and do we already catch errors from validateURI and go "huh, I guess this isn't a URI" ? Or how does it work for the cases where the user drops/pastes plaintext?

(:bz, sorry, I CC'd you because I intended to pass this review on given I'd suggested the approach, and then realized the approach was bogus anyway. Un-CC'ing you myself now would be confusing because you'd get bugmail and then still not be able to access the bug, so leaving you here both for clarity and in case you have suggestions/ideas. Feel free to un-cc.)

Comment 13

[Tooru Fujisawa [:arai]]

here's the list of consumers of _validateURI (reversed call tree)

ContentAreaDropListener.prototype._validateURI
  ContentAreaDropListener.prototype.dropLinks
    browserDragAndDrop.dropLinks
      homeButtonObserver.onDrop => sets links as homepage
      newTabButtonObserver.onDrop => opens links in new tabs
      newWindowButtonObserver.onDrop => opens links in new windows
      tabbrowser-tabs.drop handler => loads links in tabs
      urlbar._getDroppableItem => returns null if dropLinks throws
        urlbar.onDragOver => sets aEvent.dataTransfer.dropEffect to none
        urlbar.onDrop => sets the first item to the URL bar text
    DownloadsPlacesView.prototype.onDrop => downloads links
    DownloadsIndicatorView.onDrop => save links
    nsDocShellTreeOwner::HandleEvent => opens the first link in current tab
                                        and opens the other links in new tabs
                                        (done by parent. another validation is
                                         done on parent side)
    browser.drop handler => passes to this.droppedLinkHandler
                            (that is handleDroppedLink,
                             that opens links in new tabs)


So, in almost all cases the callers are going to treat the returned links as URL.
and in that case validating non-URL as URL by fixup is fine.

The only problematic case is urlbar, that sets the dropped text as URL bar content, that can be non-URL at that moment.
I'll try fixing that case.
so that we can just fix ContentAreaDropListener.prototype._validateURI to apply fixup in all cases.

Comment 14

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #13)
>       urlbar._getDroppableItem => returns null if dropLinks throws
>         urlbar.onDragOver => sets aEvent.dataTransfer.dropEffect to none
>         urlbar.onDrop => sets the first item to the URL bar text
>     DownloadsPlacesView.prototype.onDrop => downloads links
> ...
> The only problematic case is urlbar, that sets the dropped text as URL bar
> content, that can be non-URL at that moment.
> I'll try fixing that case.
> so that we can just fix ContentAreaDropListener.prototype._validateURI to
> apply fixup in all cases.

err, sorry I overlooked that it triggers command after setting the value.
so, just throwing there should also be fine.

https://searchfox.org/mozilla-central/rev/ecb86060b4c5a9808798b81a57e79e821bb47082/browser/base/content/urlbarBindings.xml#1057-1060
>       <method name="onDrop">
>         <parameter name="aEvent"/>
>         <body><![CDATA[
>           let droppedItem = this._getDroppableItem(aEvent);
>           if (droppedItem) {
>             this.value = droppedItem instanceof URL ? droppedItem.href : droppedItem;
>             SetPageProxyState("invalid");
>             this.focus();
>             this.handleCommand();

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Validate non-URI items when dropping items.

Removed the change to nsScriptSecurityManager.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8953828 [details] [diff] [review]
Validate non-URI items when dropping items.

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

I'm sorry, I think my previous review was not clear:

(In reply to :Gijs from comment #12)
> In other words, we could fix this bug by just removing only that block and
> leaving it at that... but what would that break? Is that comment just
> outdated and do we already catch errors from validateURI and go "huh, I
> guess this isn't a URI" ? Or how does it work for the cases where the user
> drops/pastes plaintext?

As I suspected, this breaks e.g. dragging "ab cd" to the tabstrip, and having it open a web search (at least, that's what I'm seeing testing this change locally). I imagine similar things break for the other cases. We shouldn't break that.

Taking several steps back...

The proper thing to do here is to check the final URI that's loaded against the original principal that triggered such a load. In Nightly, I think this already happens for the tabstrip ( https://dxr.mozilla.org/mozilla-central/rev/7208b6a7b11c3ed8c87a7f17c9c30a8f9583e791/browser/base/content/tabbrowser.xml#7656,7673,7681 ) note that comment #0 omits 60 - did this get fixed for 60 by bug 1424107?

I am less sure what's happening for the URL bar off-hand. We should ensure that the right triggering principal is passed there, too. Note that I think https://dxr.mozilla.org/mozilla-central/rev/7208b6a7b11c3ed8c87a7f17c9c30a8f9583e791/browser/base/content/urlbarBindings.xml#1019-1034 should be updated to get the triggering principal off the event instead of checking browser.contentPrincipal. We should also somehow pass the triggering principal to handleCommand when executing the load.

We should probably check other callers to see if they execute loads (off-hand based on the reverse calltree in the comment, I expect e.g. the downloads button will do this too) and have similar fixes there.

There are some cases where this isn't a complete fix. Notably, dragging to some other UI elements will not actually execute a load but create a bookmark or similar.

In that case, we still need to do some kind of manual check, I guess. The problem is that in some cases we just want to paste plain text, it seems? We should avoid breaking bug 1435910 . If we just fix up every line allowing for search bar fixup, we will always find a link (you can always do a search, of course) which would regress that.

So what I think we should do is both:
1) fix up callsites like the url bar, downloads button etc. to ensure they pass the right triggering principal. This will stop the load at the docshell level.
2) something similar to the first patch on this bug (now marked obsolete) that I r-'d (sorry!). Instead of checking Services.io.newURI, use fixup to get a URI. Pass the typo and search flags like the patch did. However, if the fixup info's `keywordProviderName` is non-empty, pass back the original string. This will mean that we're going to execute a search if allowed, and otherwise we won't find a URL (or, if being very liberal, we'll create a URL from a single word, but that will necessarily be an http URI so loading it should be OK). If we the `keywordProviderName` is empty we've fixed up to have a URI object in `fixedURI`. Pass the URI object (and not the uristring) to scriptSecurityManager.checkLoadURIWithPrincipal (note: not checkLoadURI*String*WithPrincipal), and if successful, pass back that URI's spec (not the original input).

Note that (2) is overly complicated to handle the case where a line just contains a single word. If people drag 'security' to the tabstrip or similar, they'll want a search for 'security', not loading "http://security/", even if that's what fixup will suggest if we don't pass the search keyword flag.

:arai, does that seem sensible? Happy to discuss this more on IRC / vidyo / bugzilla if that's helpful.

Comment 17

[Tooru Fujisawa [:arai]]

Yes, that plan sounds good.
I've modified code and looks like they're working.


here's the same tree from (comment #13) with how principal is/was used for each case:

(reverse call tree)
ContentAreaDropListener.prototype._validateURI
  ContentAreaDropListener.prototype.dropLinks
    browserDragAndDrop.dropLinks
      homeButtonObserver.onDrop => sets links as homepage
        ** doesn't load immediately, we should rely on validation in ContentAreaDropListener **
      newTabButtonObserver.onDrop => opens links in new tabs
        ** wasn't using triggering principal, FIXED **
      newWindowButtonObserver.onDrop => opens links in new windows
        ** wasn't using triggering principal, FIXED **
      tabbrowser-tabs.drop handler => loads links in tabs
        ** is using triggering principal, OK **
      urlbar._getDroppableItem => returns null if dropLinks throws
        urlbar.onDragOver => sets aEvent.dataTransfer.dropEffect to none
          ** doesn't use links, OK **
        urlbar.onDrop => loads a link
          ** wasn't using triggering principal, FIXED **
    DownloadsPlacesView.prototype.onDrop => downloads links
      ** seems to be using system principal, but I cannot locate the actual call path, NEEDS FIX? **
    DownloadsIndicatorView.onDrop => save links
      ** is using system principal, NEEDS FIX **
    nsDocShellTreeOwner::HandleEvent => opens the first link in current tab
                                        and opens the other links in new tabs
                                        (done by parent. another validation is
                                         done on parent side)
      ** is using triggering principal for normal case, OK **
      ** wasn't using triggering principal for non-tab browser, FIXED **
    browser.drop handler => passes to this.droppedLinkHandler
                            (that is handleDroppedLink,
                             that opens links in new tabs)
      ** is using triggering principal, OK **

Now almost all cases use triggering principal.

Even if we don't validate the URL in ContentAreaDropListener, they can block the loading.
(but the timing is very late, like, new blank tab or new window can be opened, so it's better keeping the validation in ContentAreaDropListener anyway)


The remaining cases are download:
  * library window (DownloadsPlacesView)
  * download indicator button (DownloadsIndicatorView)

for download indicator, it seems to be possible to pass triggering principal
(it's long way tho)

(non-reverse call tree)
DownloadsIndicatorView.onDrop
  saveURL
    internalSave
      internalPersist
        nsWebBrowserPersist::SavePrivacyAwareURI
          nsWebBrowserPersist::SaveURIInternal
            ** uses system principal when creating channel **

for library window, it seems to be hard.
currently download item doesn't have triggering principal information.


then, apart from ContentAreaDropListener,
creating bookmark surely doesn't validate the URL. I'll look into it (but maybe we should leave it to other bug?).


for now, I'll post patches for other already-fixed cases.

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Get triggering principal before loop, and use URI fixup to create URI from given string.

This is the main fix that uses fixed-up URI for validation.

Now it does validation only if fixup returns non-search URL.

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Pass the triggering principal when opening an URI from new tab button and new window button drag and drop.

This fixes New Tab button and New Window button,

openNewTabWith and openNewWindowWith are modified to receive parameter object,
since some parameters are not used and adding more makes it complicated.

now triggering principal is passed to openLinkIn via them.

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Pass the triggering principal when opening an URI from location bar drag and drop.

This fixed URL bar.

now it passes triggering principal to openUILinkIn.
and also it uses triggering principal for urlSecurityCheck.

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Pass the triggering principal when opening an URI in non-tab browser drag and drop.

I cannot locate the case that this becomes really a problem tho, just fixed at the same time.

If user drops item to xul:browser without tabbrowser, this code will be used
(but there seems to be no such case.  sidebar doesn't accept drop)
now it passes the triggering principal to LoadURI.


For now, that's all I've fixed.

Comment 22

[:Gijs (he/him)]

Comment on attachment 8954278 [details] [diff] [review]
Part 1: Get triggering principal before loop, and use URI fixup to create URI from given string.

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

Thanks, and sorry for the several rounds of review here...

Comment 23

[:Gijs (he/him)]

Comment on attachment 8954279 [details] [diff] [review]
Part 2: Pass the triggering principal when opening an URI from new tab button and new window button drag and drop.

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

Nice. Can you file a follow-up to just get rid of openNewTab/WindowWith? They seem like useless wrappers at this point. We have too many ways of opening tabs and windows, losing these won't hurt anyone.

::: browser/base/content/utilityOverlay.js
@@ +858,4 @@
>   * @param aShiftKey
>   *        True if shift key is held.  This value is used for the purpose of
>   *        determining whether to open in the background.
> + * @param aParam

Nit: match up with the actual parameter name please. :-)

@@ +868,2 @@
>    if (document.documentElement.getAttribute("windowtype") == "navigator:browser")
> +    aParams.charset = gBrowser.selectedBrowser.characterSet;

The test in https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_utilityOverlay.js#93 won't like this because `aParams` will be null. So maybe just set a default in the signature, ie `aParams = {}`

Comment 24

[:Gijs (he/him)]

(In reply to :Gijs from comment #23)
> The test in
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> general/browser_utilityOverlay.js#93 won't like this because `aParams` will
> be null. So maybe just set a default in the signature, ie `aParams = {}`

Oh, and same thing for the window case, of course.

Comment 25

[:Gijs (he/him)]

I think it's fine to deal with the download button / view and library in a follow-up bug given:
- comment #0 doesn't mention them
- this is sec-low anyway
- fixing the issue in the validation (which I think should help for those consumers, too?)
- number of patches already here,
- how this relates to the openUILinkIn patches + bug 1435910 + bug 1319157

so yeah, feel free to land and deal with any remaining issues in follow-ups.

Comment 26

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e15580a14db7fabbbf106b008e1055b54e020ad8
Bug 1435908 - Part 1: Get triggering principal before loop, and use URI fixup to create URI from given string. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/c04b1745f4ae3fd4f0fcbfcf7733bb6be5f2587f
Bug 1435908 - Part 2: Pass the triggering principal when opening an URI from new tab button and new window button drag and drop. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c38100787fc4c3a88d89cdb247b6b9e81c9e796
Bug 1435908 - Part 3: Pass the triggering principal when opening an URI from location bar drag and drop. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba48fdeecd533e32fdc8c8f65d8f3c77aaad557
Bug 1435908 - Part 4: Pass the triggering principal when opening an URI in non-tab browser drag and drop. r=Gijs

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e15580a14db7
https://hg.mozilla.org/mozilla-central/rev/c04b1745f4ae
https://hg.mozilla.org/mozilla-central/rev/5c38100787fc
https://hg.mozilla.org/mozilla-central/rev/9ba48fdeecd5

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Letting this ride the trains since it's a sec-low.

Comment 29

[Daniel Veditz [:dveditz]]

Although the effects of this bug are sec-low and does not qualify for a bug bounty, it did trigger us rewriting this code to a safer model that will potentially prevent future security issues. Accordingly we are paying a small bounty as an exception.

Comment 30

[Emil Ghitta, QA [:emilghitta]]

Hi Tooru!

It seems that I can still reproduce the issue described in comment 0 (while using the attachment from comment 1) using Firefox 61.0a1 (BuildId:20180506220517) and Firefox 60.0 (BuildId:20180430165945) on macOS 10.12.6 and macOS 10.13.3. 

What is the expected behavior after this fix? Am I missing something?

Thanks!

Comment 31

[Tooru Fujisawa [:arai]]

(In reply to Emil Ghitta, QA [:emilghitta] from comment #30)
> Hi Tooru!
> 
> It seems that I can still reproduce the issue described in comment 0 (while
> using the attachment from comment 1) using Firefox 61.0a1
> (BuildId:20180506220517) and Firefox 60.0 (BuildId:20180430165945) on macOS
> 10.12.6 and macOS 10.13.3. 

I cannot reproduce.


> What is the expected behavior after this fix?

Nothing happens.


> Am I missing something?

any chance you've saved the attachment and opening the local file?
this issue is specific to http/https content.

Comment 32

[Emil Ghitta, QA [:emilghitta]]

This issue is verified fixed using Firefox 61.0a1 (BuildId:20180506220517) and Firefox 60.0 (BuildId:20180430165945) on macOS 10.12.6 and Ubuntu 16.04 64bit.

Sorry Tooru for raising the "alarm", my bad!