1489312 - Browsing to a file does not activate matching add-ons

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[kobe]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180830143136

Steps to reproduce:

1. Load extensions "Markdown Viewer Webext" (1.4.0+) and "Asciidoctor.js Live Preview" which process *.md and *.adoc documents respectively.
2. Create dummy files test.md and test.adoc on your file system with contents "# Test".
3. In your browser, navigate to the file: URL of the files' parent folder. For example, on Windows if the files are in C:\Temp, go to URL "file:///C:/Temp"  You should see your files from step 2.
4. Click on either of the files in the directory listing.


Actual results:

The file is rendered as raw text. If you hit the refresh button, it is reloaded and still rendered as raw text.

1. Open developer tools with F12.
2. Put cursor into URL (with .md or .adoc URL) and hit Enter; the file will render.
3. Set a breakpoint at the top of the extension's content script. For Markdown use ext/content.js. For Asciidoctor, js/loader.js should do.
4. Reload the page and you should see the breakpoint is hit. Continue.
5. Follow "steps to reproduce" 3-4 again, and you will see the breakpoint is NOT hit.


Expected results:

After clicking on a file in a directory listing, its extension should activate, causing the file to render as beautified Markdown or Asciidoctor.  With a breakpoint set as described in "actual results", the breakpoint should be hit.

These extensions specify the extensions they match in their manifests in the content_scripts.matches lists. They should have activated when clicked in a directory listing.

Comment 1

[u554753]

I'm not entirely sure if this is a DevTools: Debugger or an add-ons issue. Please correct this component as necessary.

Comment 2

[Soledad Penades [:sole] [:spenades]]

I am not entirely sure the intended behaviour is for the extensions to run when you access URLs with file:/// protocols.

I don't think this is a DevTools bug.

Comment 3

[Soledad Penades [:sole] [:spenades]]

Jonathan, do you have any idea about this? Can addons access local files? (Long shoot, but worth trying!)

Comment 4

[Jonathan Kingston [:jkt] he/him]

I'm going to mark this as a duplicate of Bug 1266960.

More information is available here: https://bugzilla.mozilla.org/show_bug.cgi?id=1246236

Basically extensions aren't ever able to access file:// even with this in the manifest. The only ability is the usual mechanisms that web pages have like directory upload and file drag and dropping.

Unfortunately this breaks our security model and also has impacts on how we sandbox our code too so I'm not entirely sure we will ever permit file:// access.

Comment 5

[Rob Wu [:robwu]]

Not a duplicate. I will attach a test case.

(In reply to Jonathan Kingston [:jkt] from comment #4)
> Basically extensions aren't ever able to access file:// even with this in
> the manifest.

As the report shows, extensions can run content scripts at file:// if they have the permission to do so.
When a file:-URL is directly loaded via the location bar, the script executes.
When a file:-URL is loaded via a navigation from another file:-URL, the script somehow does not execute.

Comment 6

[Jonathan Kingston [:jkt] he/him]

> Not a duplicate. I will attach a test case.

Sorry lack of coffee :D

> When a file:-URL is loaded via a navigation from another file:-URL, the script somehow does not execute.

This is likely something I caused then. I can investigate also if you get a test case.

Comment 7

[Rob Wu [:robwu]]

Attachment: file-red-txt.zip

STR:

1. Create a ".txt" file if you don't already have one, e.g. test.txt.
2. Load attached extension at about:debugging.
3. Open test.txt in Firefox.
   [Confirm that the page is red]
4. Open its directory in Firefox, e.g by editing the URL in the location bar and removing "test.txt"
5. Click on the "test.txt" link in the directory listing.
   [Check whether the page is red]
6. Click on the extension button.
   [Check whetehr the page is PINK]

Expected:
- After step 3, the page should be red.
- After step 5, the page should be red.
- After step 6, the page should be pink.

Actual:
- After step 3, the page is red.
- After step 5, the page is blank (UNEXPECTED).
- After step 6, the page is pink.


Step 6 shows that the extension does have the permission to run content scripts, but step 5 shows that somehow the content script is not executed when a file:-URL is opened by a navigation from a directory listing.

Comment 8

[Rob Wu [:robwu]]

mozregression result:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c8514e653b71479ba22a307f58f79f563ed80e9d&tochange=07ab807639ee42a407a9bdb0d374206c0f17678d

Comment 9

[Rob Wu [:robwu]]

Open the Content Toolbox and run the following snippet after step 3 and step 5 (assuming that the test tab is the first tab).

tabs[0].content.document.nodePrincipal.baseDomain

- After step 3 (working): /tmp/
- After step 5 (broken): /tmp/test.txt

Comment 10

[Rob Wu [:robwu]]

Open the Content Toolbox and run the following snippet after step 3 and step 5 (assuming that the test tab is the first tab).

tabs[0].content.document.nodePrincipal.origin

- After step 3 (working): file:///tmp/test.txt
- After step 5 (broken): file:///tmp/

Comment 11

[Abdulrahman Alqabandi]

Attachment: tester.html

It looks like there is a special case when navigating to a file using the default folder explorer on FF vs directly opening a file.

There may be a slight security issue here, if you upload attached 'tester.html' to say 'C://tests/tester.html' then you open the file by copy pasting into new tab, you will get 'access denied'. But if you open 'C://tests/' (using firefox folder explorer) and then click on tester.html, tester.html will have full access to the contents of the folder explorer, revealing all the names of the files/folders.


Firefox allows local files to read files that are at the same folder. A mitigator is the fact that one needs to guess the names of these files. But if firefox allows access to the folder explorer then this slight mitigator is out the window.

Comment 12

[Rob Wu [:robwu]]

The principal for file:-URLs appear to be based on the initial file/URL, and only change if the directory of the file URL changes.

:ckerschb
Is this behavior expected?


I used the method from comment 10 to see the document's principal, and navigated via the DevTools in the tab (assigning to location.href).
Here are the navigations (first line) and the resulting principals (second line):

/tmp/x/file.txt (ENTER THIS IN THE LOCATION BAR)
/tmp/x/file.txt

/tmp/x/otherfile.txt (from /tmp/x/file.txt)
/tmp/x/file.txt (not changed)

/tmp/ (from /tmp/file.txt)
/tmp/

/tmp/x/ (navigated from /tmp/)
/tmp/x/

/tmp/x/file.txt (navigated from /tmp/x/)
/tmp/x/ (not changed)

/tmp/x/otherfile.txt (navigated from /tmp/x/file.txt)
/tmp/x/ (not changed)

/tmp/ (navigated from /tmp/x/otherfile.txt)
/tmp/

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Rob Wu [:robwu] from comment #12)
> The principal for file:-URLs appear to be based on the initial file/URL, and
> only change if the directory of the file URL changes.
> 
> :ckerschb
> Is this behavior expected?

We handle file:// URLs special and I am not entirely sure to be honest.
302 to dveditz who is the better person to ask.

@Dan: please see question in comment 12.

Also ni? for kmag because it seems that Bug 1436482 is involved here and might cause trouble for addons.

Comment 14

[Kris Maglione [:kmag]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> (In reply to Rob Wu [:robwu] from comment #12)
> > The principal for file:-URLs appear to be based on the initial file/URL, and
> > only change if the directory of the file URL changes.
> > 
> > :ckerschb
> > Is this behavior expected?
> 
> We handle file:// URLs special and I am not entirely sure to be honest.
> 302 to dveditz who is the better person to ask.

The same-origin policy for file URLs by default only allows access to files in sub-directories of the originally-opened page, which is why navigations to sub-directories inherit the principal of the page that opened them. That is obviously not ideal for content script matchers that want to be more specific.

I suppose we should special case file: URLs with reasonable file: URL principals. We already need to add another special case for loads with the CSP sandbox directive, since they wind up with unique principals that don't match any content script URIs.

Comment 15

[Daniel Veditz [:dveditz]]

(In reply to Abdulrahman Alqabandi from comment #11)
> It looks like there is a special case when navigating to a file using the
> default folder explorer on FF vs directly opening a file.
> 
> There may be a slight security issue here

Yes, somehow the directory origin is carrying over on navigation -- that shouldn't happen. This is also noted in bug 1477067, and it is a security issue

(In reply to Rob Wu [:robwu] from comment #12)
> The principal for file:-URLs appear to be based on the initial file/URL, and
> only change if the directory of the file URL changes.

The _intended_ behavior is for top-level file:/// documents to have an "origin" of their full URL, but be allowed to read and load files in the same directory and sub-directories as if they are same origin. If that document frames another file:// url that matches this odd "same-origin" definition then that framed document will have the same principal as the top-level document. This is looser than Chrome/Safari's definition, and stricter than IE's (I assume Edge matches IE on this but I haven't tested it specifically).

We have bug 803143 on making each file unique to match Chrome's behavior.


> :ckerschb
> Is this behavior expected?
> 
> I used the method from comment 10 to see the document's principal, and
> navigated via the DevTools in the tab (assigning to location.href).
> Here are the navigations (first line) and the resulting principals (second
> line):
> 
> /tmp/x/file.txt (ENTER THIS IN THE LOCATION BAR)
> /tmp/x/file.txt
> 
> /tmp/x/otherfile.txt (from /tmp/x/file.txt)
> /tmp/x/file.txt (not changed)

This is not intended: on navigations the principal should change to /tmp/x/otherfile.txt.

> /tmp/x/ (navigated from /tmp/)
> /tmp/x/
> 
> /tmp/x/file.txt (navigated from /tmp/x/)
> /tmp/x/ (not changed)

Why did it change from /tmp/ to /tmp/x/ (correct) but not from /tmp/x/ to /tmp/x/file.txt ?  This is the underlying bug (or at least one of them) in bug 1477067

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Daniel Veditz [:dveditz] from comment #15)
> Why did it change from /tmp/ to /tmp/x/ (correct) but not from /tmp/x/ to
> /tmp/x/file.txt ?  This is the underlying bug (or at least one of them) in
> bug 1477067

I think we should not only fix that particular problem described within this Bug, but ultimately start treating file: URIs as unique origins. Not only will that fix the bug described here, but also simplify our access control code within nsScriptSecurityManager.

I added a project to the DOM:Security Roadmap and hopefully we can get that started within Q4. I guess Bug 803143 is the bug one should follow for tracking that work.

Comment 17

[Vladimir Bostanov]

(In reply to Daniel Veditz [:dveditz] from comment #15)
> Why did it change from /tmp/ to /tmp/x/ (correct) but not from /tmp/x/ to
> /tmp/x/file.txt ?  This is the underlying bug (or at least one of them) in
> bug 1477067

Yes, now I see clearly that this is the "real bug". And it can be exploited by both XMLHttpRequest() and <iframe>. I tested it, see bug 803143, comment 27.

The origin policy for file:// is not a bug, but a desired and expected behaviour that is about to be changed soon for security reasons. Did I get that right?

The possibility to set cookies for file:// is also desired and expected, I suppose? -- But it facilitates the exploitation of the "real bug", as I explained in bug 1477067.

Would it be possible to fix the "real bug" ASAP? This would greatly reduce the security risk, while changing the origin policy for file:// will, certainly, need more time, won't it?

Cheers, V.

Comment 19

[Tyson Smith [:tsmith]]

Is there anything else needed to make progress on this bug?

Comment 20

[Andrew McCreight [:mccr8]]

Sounds potentially bad, so I'm going to mark this sec-moderate. Adjust as needed.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Due to the fix in bug 1558299 (I cannot see the bug) which landed here:

https://hg.mozilla.org/mozilla-central/rev/2ad059cc9e78

I can no longer reproduce the STRs from comment 7 or comment 10. The principal is always that of the file.

It seems this is fixed by either bug 1558299 or related work. Should we close this?

Comment 22

[Shane Caraveo (:mixedpuppy)]

I'm also unable to reproduce str in comment 7 on linux.