Closed Bug 1329822 Opened 7 years ago Closed 7 years ago

file:// documents can't use <a download=foo.txt> to set a download name/force a download

Categories

(Core :: Security: Process Sandboxing, defect)

Product:
Core
Component:
Security: Process Sandboxing
Version:
53 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- disabled
firefox54 --- verified
firefox55 --- verified

People

(Reporter: jgilbert, Assigned: haik)

References

Details

(Keywords: regression, Whiteboard: sbmc2 sbwc2 sblc3)

Attachments

(2 files)

download-file.html
451 bytes, text/html
Details
Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a download name/force a download;
59 bytes, text/x-review-board-request
Gijs
: review+
baku
: review+
Details
Attached file download-file.html — 
This means scripts which generate downloads in response to user commands need to be run through a server, and do not function correctly as file:// urls.

May be related to bug 1321020.
This doesn't reproduce if you turn off the separate process for file: using the pref, so it seems fairly clear it's a regression from that change. Bob, I'm not sure the cause of this one is the same as the view-source: and/or missing file ones that are already on file, so I'm flagging this up in case it piques your interest.
Blocks: 1147911, 1321430
Flags: needinfo?(bobowencode)
Keywords: regression
Assignee: nobody → haftandilian
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → unaffected
status-firefox53: --- → affected
Version: unspecified → 53 Branch
Whiteboard: sbmc2 sbwc2 sblc3
Still debugging, but here's my progress so far.

When clicking on the click-me link in the test case, we switch from a file content process to a web content process and display 1 2 3 in the tab. 

     1 <body>
     2 <a onclick="DownloadText('foo.txt', ['1','2','3']);">click-me</a>
     3 <script>
     4 function DownloadText(filename, textArr, mimetype='text/plain') {
     5     var blob = new Blob(textArr, {type: mimetype});
     6     var url = URL.createObjectURL(blob);
     7 
     8     var link = document.createElement('a');
     9     link.href = url;
    10     link.download = filename;
    11 
    12     document.body.appendChild(link);
    13     link.click();
    14     document.body.removeChild(link);
    15 }
    16 </script>
    17 </body>
    
After line 6, "url" holds

  "blob:null/af8d5526-243d-8a46-95c3-669f6e41217c"

When the link is clicked we enter this code to decide if it should be loaded in the current process.

  JS validatedWebRemoteType() at E10SUtils.jsm:38
  JS getRemoteTypeForURI("blob:null/...", true, "file") at E10SUtils.jsm:144 
  JS shouldLoadURIInThisProcess() at E10SUtils.jsm: 149
  JS shouldLoadURI() at E10SUtils.jsm:158
  JS shouldLoadURI() at tab-content.js:708
  nsDocShell::InternalLoad() at nsDocShell.cpp:10690
  nsDocShell::OnLinkClickSync() at nsDocShell.cpp:14054
  OnLinkClickEvent::Run() at nsDocShell.cpp:13820
  
In getRemoteTypeForURI(), we have tests for these URI protocol prefixes, but not "blob:".

  if (aURL.startsWith("javascript:")) { ...
  if (aURL.startsWith("data:")) { ...
  if (aURL.startsWith("file:")) { ...
  if (aURL.startsWith("about:")) { ...
  if (aURL.startsWith("chrome:")) { ...
  if (aURL.startsWith("moz-extension:")) { ...
  
These each return a remoteType which must be "file" in order to load the URI in the file content process. The existing window remote type is passed in and that is returned for data: and javascript: URI's.
  
For blob: URI's, we fall through to validatedWebRemoteType()

  function validatedWebRemoteType(aPreferredRemoteType) {
    return aPreferredRemoteType && aPreferredRemoteType.startsWith(WEB_REMOTE_TYPE)
           ? aPreferredRemoteType : WEB_REMOTE_TYPE;
  }
  
which only returns WEB_REMOTE_TYPE or types that start with WEB_REMOTE_TYPE ("web").

I suspect we should treat "blob:" URI's the same way we treat "data:" URI's by opening them in the existing process type, but I'm wondering if we should instead change validatedWebRemoteType() to be catchall for any other protocol.

The "null" in the blob URL comes from this stack because the URI has no host.

  frame #0 XUL`mozilla::net::nsStandardURL::Host() at nsStandardURL.h:387
  frame #1 XUL`mozilla::net::nsStandardURL::GetAsciiHost() at nsStandardURL.cpp:1436
  frame #2 XUL`nsContentUtils::GetASCIIOrigin() at nsContentUtils.cpp:6023
  frame #3 XUL`nsContentUtils::GetASCIIOrigin() at nsContentUtils.cpp:5986
  frame #4 XUL`nsHostObjectProtocolHandler::GenerateURIString() at nsHostObjectProtocolHandler.cpp:671
  frame #5 XUL`nsHostObjectProtocolHandler::GenerateURIStringForBlobURL() at nsHostObjectProtocolHandler.cpp:690
  frame #6 XUL`nsHostObjectProtocolHandler::AddDataEntry() at nsHostObjectProtocolHandler.cpp:517
  frame #7 XUL` namespace)::CreateObjectURLInternal<moz::CreateObjectURLInternal<mozilla::dom::BlobImpl*>() at URL.cpp:52
  frame #8 XUL`mozilla::dom::(anonymous namespace)::URLMainThread::CreateObjectURL() at URL.cpp:87
  frame #9 XUL`mozilla::dom::URL::CreateObjectURL() at URL.cpp:1727
  ...
Flags: needinfo?(bobowencode)
(In reply to Haik Aftandilian [:haik] from comment #2)

Thanks for looking into this.

> I suspect we should treat "blob:" URI's the same way we treat "data:" URI's
> by opening them in the existing process type, but I'm wondering if we should
> instead change validatedWebRemoteType() to be catchall for any other
> protocol.

(Chrome also returns blob:null/... for file URIs by the way. The spec seems to say it's up to the user agent for non-http(s) URIs.)

Yes we might have to special case blob, but in that case we should possibly only allow in non-web remote type if they are blob:null.
Hmm, but what if a nested http(s) page did this, then presumably we would get the normal blob URI and we would still need this to work.

We should get someone who knows the blob code to review or possibly provide advice up front, as to how this all works.

Also, I guess we would only allow them in remote browsers (i.e. not in the parent), the same as data ones.

We shouldn't change validatedWebRemoteType, that is specifically for testing if it is a special type of web remote type.
There is only the large allocation one at the moment.
(In reply to Bob Owen (:bobowen) from comment #3)
> (In reply to Haik Aftandilian [:haik] from comment #2)
> 
> Thanks for looking into this.
> 
> > I suspect we should treat "blob:" URI's the same way we treat "data:" URI's
> > by opening them in the existing process type, but I'm wondering if we should
> > instead change validatedWebRemoteType() to be catchall for any other
> > protocol.
> 
> (Chrome also returns blob:null/... for file URIs by the way. The spec seems
> to say it's up to the user agent for non-http(s) URIs.)
> 
> Yes we might have to special case blob, but in that case we should possibly
> only allow in non-web remote type if they are blob:null.
> Hmm, but what if a nested http(s) page did this, then presumably we would
> get the normal blob URI and we would still need this to work.

Can you explain what you mean by "nested http page"? Out of interest, what happens right now if you have a file page with an iframe pointing to http:// ? Where do we load the page, and does it work?

It feels like blob: URIs when passed to this method should just always use the process they're currently using, like data: and javascript: .

> We should get someone who knows the blob code to review or possibly provide
> advice up front, as to how this all works.

:baku might be a good person to do this.

> 
> Also, I guess we would only allow them in remote browsers (i.e. not in the
> parent), the same as data ones.

It's possible for chrome-privileged parent process things to create and point to blob: things.
(In reply to :Gijs from comment #4)

> Can you explain what you mean by "nested http page"? Out of interest, what
> happens right now if you have a file page with an iframe pointing to http://
> ? Where do we load the page, and does it work?

By nested http page I mean something like the example that you give.
This does work, but we have to load these in the file content process at the moment.
 
> > Also, I guess we would only allow them in remote browsers (i.e. not in the
> > parent), the same as data ones.
> 
> It's possible for chrome-privileged parent process things to create and
> point to blob: things.

Then I would have thought this might break in a similar way (and would have done before my changes).
baku, do you have any input on this?
Flags: needinfo?(amarchesini)
Blob URL are 'owned' by a global and they are broadcasted to any process when they are created and when they are unregistered. This means that, if we try to load a blob URL, after the click (on because of the click) from another process (the parent one, for instance), that blob URL doesn't exist anymore.
What you need to know, is that, those blob URLs, are still 'valid' for the current process (where the global belongs to) for 1 additional second. This extra timing is meant to be used exactly for what you want to do: if there is some <a>, or some other object, who has a reference to one of those blob URLs, everything works fine for it.
So, yes, the blob URL _must_ be loaded by the content process.
Flags: needinfo?(amarchesini)
(In reply to Haik Aftandilian [:haik] from comment #9)
> Try push:
>  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7826d0cfb4114ce7a59bfbbac7e59d314c35f0a8
> 
> Try push with browser.tabs.remote.separateFileUriProcess=false:
>  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=36c5f0a3baacdcf570f5b1e84a49805c74dfa066

Why don't you add both configurations to one test suite?
(In reply to Masatoshi Kimura [:emk] from comment #10)
> (In reply to Haik Aftandilian [:haik] from comment #9)
> > Try push:
> >  
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7826d0cfb4114ce7a59bfbbac7e59d314c35f0a8
> > 
> > Try push with browser.tabs.remote.separateFileUriProcess=false:
> >  
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=36c5f0a3baacdcf570f5b1e84a49805c74dfa066
> 
> Why don't you add both configurations to one test suite?

That's a great idea. I'll look into doing that. I don't have much experience writing tests so any pointers would be appreciated. I do see some blob URI tests in the tree and I'll look at those.
Comment on attachment 8829747 [details]
Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a download name/force a download;

https://reviewboard.mozilla.org/r/106710/#review107810

::: browser/modules/E10SUtils.jsm:83
(Diff revision 1)
>                                                  : aPreferredRemoteType;
>      }
>  
> +    // Let blob URI's load in the current process
> +    if (aURL.startsWith("blob:")) {
> +      return aPreferredRemoteType;

You should really get a Firefox peer to review this (maybe gijs as he reviewed the file content process stuff).

I think you are changing the behaviour here for the parent/non-remote process.

We should probably do the same as for data: I think.
Given that before the file content process changes, I think we would have always loaded in the child.
(This might actually make no difference if the URI isn't valid in that process, but still best to be safe.)
Attachment #8829747 - Flags: review?(bobowencode)
Comment on attachment 8829747 [details]
Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a download name/force a download;

https://reviewboard.mozilla.org/r/106710/#review107810

> You should really get a Firefox peer to review this (maybe gijs as he reviewed the file content process stuff).
> 
> I think you are changing the behaviour here for the parent/non-remote process.
> 
> We should probably do the same as for data: I think.
> Given that before the file content process changes, I think we would have always loaded in the child.
> (This might actually make no difference if the URI isn't valid in that process, but still best to be safe.)

Thanks. Updated the fix to treat blob: URI's like data: URI's, but I realize that this changes the behavior when the remote type is "extension". Before file content process, that would be loaded in "web". Will update the code to fix this.
(In reply to Haik Aftandilian [:haik] from comment #14)
> Thanks. Updated the fix to treat blob: URI's like data: URI's, but I realize
> that this changes the behavior when the remote type is "extension". Before
> file content process, that would be loaded in "web". Will update the code to
> fix this.

I didn't realize we added a remote type for add-ons. I would have expected using "extension" to be correct. Why do blobs created with extension principals need to be loaded in the web process?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(haftandilian)
They don't. They need to be loaded in the extension content process.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8829747 [details]
Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a download name/force a download;

https://reviewboard.mozilla.org/r/106710/#review108054

r- as this is wrong for add-on generated blob. I think we should just always return aPreferredRemoteType for `blob:`. Is there some reason that's a bad idea that I'm missing?
Attachment #8829747 - Flags: review?(gijskruitbosch+bugs) → review-
(In reply to :Gijs from comment #18)
> Comment on attachment 8829747 [details]
> Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a
> download name/force a download;
> 
> https://reviewboard.mozilla.org/r/106710/#review108054
> 
> r- as this is wrong for add-on generated blob. I think we should just always
> return aPreferredRemoteType for `blob:`. Is there some reason that's a bad
> idea that I'm missing?

No, that makes more sense. I was trying to preserve the original behavior. Before this fix, blob: URI's coming from EXTENSION_REMOTE_TYPE would fall through to validatedWebRemoteType() which would return WEB_REMOTE_TYPE. It sounds like that's a bug and I'll change the code to do the same thing as we do for data: URI's which will load them in the originating process as long as it isn't NOT_REMOTE.
Flags: needinfo?(haftandilian)
Sorry Haik, I perhaps should have made it clear when I said it should be treated the same as data:, that I meant that it was indeed broken for other remote types at the moment as well, so we wanted to change the behaviour for those.

(In reply to :Gijs from comment #18)

> r- as this is wrong for add-on generated blob. I think we should just always
> return aPreferredRemoteType for `blob:`. Is there some reason that's a bad
> idea that I'm missing?

Are you sure we don't want to return the same as for data:, i.e.:

 return aPreferredRemoteType == NOT_REMOTE ? DEFAULT_REMOTE_TYPE : aPreferredRemoteType;

Isn't this what we would have done before any of the file content process changes?
Of course this could also have been a bug, if we need to load blobs at the top level in the parent process for some reason.
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Bob Owen (:bobowen) from comment #20)
> (In reply to :Gijs from comment #18)
> 
> > r- as this is wrong for add-on generated blob. I think we should just always
> > return aPreferredRemoteType for `blob:`. Is there some reason that's a bad
> > idea that I'm missing?
> 
> Are you sure we don't want to return the same as for data:, i.e.:
> 
>  return aPreferredRemoteType == NOT_REMOTE ? DEFAULT_REMOTE_TYPE :
> aPreferredRemoteType;
> 
> Isn't this what we would have done before any of the file content process
> changes?
> Of course this could also have been a bug, if we need to load blobs at the
> top level in the parent process for some reason.

I'm not really very opinionated either way, though I'm not sure how often that would happen and if it would realistically break something. I imagine you could try with something like the following steps:

1. open about:newtab
2. open the web console
3. eval:

x = new Blob(["abc"], {type: "text/plain"});
a = document.createElement('a');
a.textContent = "Look at me, I'm a blob link";
a.href = URL.createObjectURL(x);
document.body.appendChild(a);

4. open the link you just appended in a new tab with the context menu / ctrl-click

I don't know if it's necessary to open those in the parent or in a non-parent explicitly, but I guess it makes sense to use a remote browser if we can just for security reasons, assuming this works (which per comment #7 sounds like it should be OK?). I'd trust :baku's judgment over mine, though.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(amarchesini)
Opening a blobURL in the parent process and/or in the content process works. The only issue is that, if you change process, the original document must stay alive, otherwise the blobURL is revoked everywhere. If you stay on the same process, the revoke, just for that particular process, happens with a delay.

This delay has been introduced exactly to run the code written by Gijs in comment 21.

In general, for a blobURL point of view, it's better to stay on the same process.
Flags: needinfo?(amarchesini)
So, this must have been broken before the file content process changes then.

Anyway, as long as we don't think that we might be introducing any new security bugs, then we should return aPreferredRemoteType for blob URIs.
Given that we have no known reason to _require_ blob URI's to load in the parent when coming from the parent, I'd rather err on the side of security and load them in a sandboxed process with fewer privileges.
Attachment #8829747 - Flags: review?(amarchesini)
Comment on attachment 8829747 [details]
Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a download name/force a download;

https://reviewboard.mozilla.org/r/106710/#review109570

Well, r=me if we're sure this works. r?baku to make sure that we're sure - I still don't feel that I fully understand if there are edgecases we would break this way.
Attachment #8829747 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #26)
> Comment on attachment 8829747 [details]
> Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a
> download name/force a download;
> 
> https://reviewboard.mozilla.org/r/106710/#review109570
> 
> Well, r=me if we're sure this works. r?baku to make sure that we're sure - I
> still don't feel that I fully understand if there are edgecases we would
> break this way.

This is what happens now I believe, so I think we should be OK (assuming it's not broken now on release).

If you:
* load about:robots
* run this in the Console:
   var blob = new Blob(['2','3','5'], {type: 'text/plain'}); URL.createObjectURL(blob);
* load the resulting URL in a new tab

It loads in the content process (if e10s is enabled).
Comment on attachment 8829747 [details]
Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a download name/force a download;

https://reviewboard.mozilla.org/r/106710/#review109580
Attachment #8829747 - Flags: review?(amarchesini) → review+
(In reply to Bob Owen (:bobowen) from comment #27)
> (In reply to :Gijs from comment #26)
> > Comment on attachment 8829747 [details]
> > Bug 1329822 - file:// documents can't use <a download=foo.txt> to set a
> > download name/force a download;
> > 
> > https://reviewboard.mozilla.org/r/106710/#review109570
> > 
> > Well, r=me if we're sure this works. r?baku to make sure that we're sure - I
> > still don't feel that I fully understand if there are edgecases we would
> > break this way.
> 
> This is what happens now I believe, so I think we should be OK (assuming
> it's not broken now on release).
> 
> If you:
> * load about:robots
> * run this in the Console:
>    var blob = new Blob(['2','3','5'], {type: 'text/plain'});
> URL.createObjectURL(blob);
> * load the resulting URL in a new tab
> 
> It loads in the content process (if e10s is enabled).

Thanks and, to reiterate, the current fix preserves this behavior.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9cecb0eb3c5b
file:// documents can't use <a download=foo.txt> to set a download name/force a download; r=baku,Gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9cecb0eb3c5b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Does this need to be uplifted to Aurora?
Flags: needinfo?(haftandilian)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #32)
> Does this need to be uplifted to Aurora?

No, it doesn't.

The fix is only applicable when the file-content process is enabled or the extensions process is enabled. The file-content process is pref'd on for Nightly only at this time. That's browser.tabs.remote.separateFileUriProcess set in modules/libpref/init/all.js. The extensions process is new for build 54 and disabled by default.

When neither are enabled, the fix doesn't change behavior for blob: loads from chrome or content.
Flags: needinfo?(haftandilian)
Reproduced using Nightly 54.0a1 (Build ID: 20170109030209) on Windows 10 x64.

Verified as fixed using the latest Nightly 55.0a1 (Build ID: 20170330030213) and Aurora 54.0a2 (Build ID: 20170330004004, having the preference "browser.tabs.remote.separateFileUriProcess" set to true) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
status-firefox55: --- → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: