Bug 1435908 (CVE-2018-5182)

Web content can open local files and local folders using drag & drop event

VERIFIED FIXED in Firefox 60

Status

()

defect
VERIFIED FIXED
Last year
7 months ago

People

(Reporter: jordi.chancel, Assigned: arai)

Tracking

({sec-low})

58 Branch
mozilla60
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 verified, firefox61 verified)

Details

(Whiteboard: [adv-main60+])

Attachments

(6 attachments, 4 obsolete attachments)

1.59 KB, text/html
Details
315 bytes, text/html
Details
4.67 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
6.26 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
6.67 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
3.48 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
Reporter

Description

Last year
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.
Reporter

Comment 1

Last year
reduced testcase
Attachment #8948559 - Attachment is obsolete: true
Reporter

Comment 2

Last year
Posted file video-demo.html
Here's the demo video.
flagging for a bounty as asked, but this rings a bell. Need to check whether this is a dupe of one qab submitted.
Flags: sec-bounty?
Reporter

Comment 4

Last year
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.
Flags: needinfo?(dveditz)
Reporter

Updated

Last year
Flags: needinfo?(jordi.chancel)
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.
Component: General → Drag and Drop
Flags: needinfo?(dveditz) → needinfo?(arai.unmht)
Keywords: sec-low
Group: core-security → dom-core-security
Assignee

Comment 6

Last year
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.
Assignee: nobody → arai.unmht
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee

Comment 7

Last year
Now ContentAreaDropListener.prototype._validateURI applies URI fixup before checking the uri.
Flags: needinfo?(arai.unmht)
Attachment #8953059 - Flags: review?(gijskruitbosch+bugs)

Comment 8

Last year
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?
Attachment #8953059 - Flags: review?(gijskruitbosch+bugs) → review-

Comment 9

Last year
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?
(sorry for spam)
The generic triggering principal bug :jkt is working on is bug 1374741.
Assignee

Comment 11

Last year
Okay, stopping early-return for non-URI case fixes the issue.
Attachment #8953059 - Attachment is obsolete: true
Attachment #8953753 - Flags: review?(gijskruitbosch+bugs)
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.)
Attachment #8953753 - Flags: review?(gijskruitbosch+bugs)
Assignee

Comment 13

Last year
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.
Assignee

Comment 14

Last year
(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();
Assignee

Comment 15

Last year
Removed the change to nsScriptSecurityManager.
Attachment #8953753 - Attachment is obsolete: true
Attachment #8953828 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8953828 - Flags: review?(gijskruitbosch+bugs)
Assignee

Comment 17

Last year
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.
Assignee

Comment 18

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

Now it does validation only if fixup returns non-search URL.
Attachment #8953828 - Attachment is obsolete: true
Attachment #8954278 - Flags: review?(gijskruitbosch+bugs)
Assignee

Comment 19

Last year
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.
Attachment #8954279 - Flags: review?(gijskruitbosch+bugs)
Assignee

Comment 20

Last year
This fixed URL bar.

now it passes triggering principal to openUILinkIn.
and also it uses triggering principal for urlSecurityCheck.
Attachment #8954280 - Flags: review?(gijskruitbosch+bugs)
Assignee

Comment 21

Last year
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.
Attachment #8954281 - Flags: review?(gijskruitbosch+bugs)
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...
Attachment #8954278 - Flags: review?(gijskruitbosch+bugs) → review+
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 = {}`
Attachment #8954279 - Flags: review?(gijskruitbosch+bugs) → review+
(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.

Updated

Last year
Attachment #8954280 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

Last year
Attachment #8954281 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Assignee

Comment 26

Last year
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
Group: dom-core-security → core-security-release
Letting this ride the trains since it's a sec-low.
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.
Flags: sec-bounty? → sec-bounty+
Reporter

Updated

Last year
Flags: needinfo?(jordi.chancel)
Whiteboard: [adv-main60+]
Alias: CVE-2018-5182
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!
Flags: needinfo?(arai.unmht)
Assignee

Comment 31

Last year
(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.
Flags: needinfo?(arai.unmht)
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!
Status: RESOLVED → VERIFIED
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.