1344957 - Read file system access sandbox bypass using FileCreationRequest from PContent.ipdl

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Julian Hector [:tedd] [:jhector]]

Attachment: DO NOT COMMIT - PoC code for demonstrating sandbox bypass

Currently we are in the process of removing file read access for a sandboxed content process, I spoke with :jimm on IRC and the target release for Windows is 55, for Mac with have limited file system read access on nightly and for Linux we only have write access restriction so far, but next step will be file read access.

I found a bug similar to Bug 1344415, which allows to (eventually, if this stays unfixed) bypass the sandbox restrictions regarding file read access by leveraging the FileCreationRequest [1] message of the PContent protocol. It allows to read any file that the parent process has access to.

Attached you find a patch that exposes some functions to the web console which allows to test this issue.

Once you have compiled a version with the patch applied, start up firefox, navigate to a website, like google.com and start the Web Developer console. Run the following code:

> let fr = new FileReader();
> fr.onload = function(evt) {
>   console.log(evt.target.result);
> };
> 
> window.File.requestRemoteFile("/mnt/foobar", { existenceCheck: true }).then(function(file) {
>   fr.readAsText(file);
> }).catch(function(err) {
>   console.log(err);
> });

You will see in the attached patch, that I granted read access to everything except "/mnt" to test this issue. I verified that when the content process is trying to open anything in /mnt/ it fails.

You may have to adjust the path given as /mnt/foobar above.

Once you run the script, you should see the content of the file in your web console.

There is not security check performed on the parent side, so access to any existing file is possible.
I tried to find a way to write to files (similar to Bug 1344415) but I couldn't find any way to do that with the create PBlob, it seams I can only read using it. However, if someone knows a way, please let me know.

Also, there is a missing null check here [2] which allows me to crash the parent process in my debug build by changing |existenceCheck| above to false, because somehow the function returns null and when trying to serialize the actor it crashes due to a null pointer. (I don't know what the behavior is on release build, i.e. crash or no crash)

:haik verified that it works on Mac (thanks for that)

[1] http://searchfox.org/mozilla-central/rev/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/dom/ipc/PContent.ipdl#1178
[2] http://searchfox.org/mozilla-central/rev/1bc7720a434af3c13b68b8d69f92078cae8890c4/dom/ipc/ContentParent.cpp#5168

Comment 1

[Andrew McCreight [:mccr8]]

Baku, do you have any thoughts here?

Comment 2

[Andrea Marchesini [:baku]]

> I tried to find a way to write to files (similar to Bug 1344415) but I
> couldn't find any way to do that with the create PBlob, it seams I can only
> read using it. However, if someone knows a way, please let me know.

Using PBlob it's not possible to write data on the filesystem.

File.createFromNsIFile and File.createFromFileName are mainly used for testing and they can run on the parent process and on the child process.
But they are also used by other components:

. mobile FilePicker
. PropertyListUtils .plist mac things...
. MSMigrationUtils - cookie migration
. CrashReporter

All of these components look like to run on the parent process. We can maybe do not allow these 2 File methods to run on the child process if not in testing mode. Or limit them to work on tmpD and profD - probably this seems less aggressive.

Comment 3

[Julian Hector [:tedd] [:jhector]]

Note File.createFromNsIFile and File.createFromFileName are already marked as ChromeOnly in their webidl file, however the true problem is that IPDL still exposes the relevant calls to the child.

Also, :baku, what was the intention of |existenceCheck| if we can't create a file with with protocol? Name seems also a little misleading.

Comment 4

[Andrea Marchesini [:baku]]

> Also, :baku, what was the intention of |existenceCheck| if we can't create a
> file with with protocol? Name seems also a little misleading.

File.createFromNsIFile and File.createFromFileName do not create a 'file' from OS point of view, but a DOM File object. When you use these 2 methods, by default, we check that the OS file exists. But in some contexts (a filePicker that wants to save something) the OS file doesn't exist, and that check should not be done.
Here why we have 'existenceCheck' true/false. by default is true.

Comment 5

[Andrea Marchesini [:baku]]

I think this bug has similar security concern of bug 1344415.
I don't want to mark this as duplicated, but I think would be nice to have 1 single security fix for both.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: file1.patch

Comment 7

[(no longer active)]

Comment on attachment 8847635 [details] [diff] [review]
file1.patch

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

::: dom/ipc/ContentParent.cpp
@@ +5153,5 @@
>                                         const bool& aIsFromNsIFile)
>  {
> +  // We allow the creation of File via this IPC call only for the 'file' process
> +  // or for testing.
> +  if (!mRemoteType.EqualsLiteral(FILE_REMOTE_TYPE) &&

Shouldn't this respect the file content process pref?

Comment 8

[Andrea Marchesini [:baku]]

> > +  // We allow the creation of File via this IPC call only for the 'file' process
> > +  // or for testing.
> > +  if (!mRemoteType.EqualsLiteral(FILE_REMOTE_TYPE) &&
> 
> Shouldn't this respect the file content process pref?

Is it not the same? If the process type is 'file', that means we have we ha content process pref set to true.
Am I wrong?

Comment 9

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #8)
> > > +  // We allow the creation of File via this IPC call only for the 'file' process
> > > +  // or for testing.
> > > +  if (!mRemoteType.EqualsLiteral(FILE_REMOTE_TYPE) &&
> > 
> > Shouldn't this respect the file content process pref?
> 
> Is it not the same? If the process type is 'file', that means we have we ha
> content process pref set to true.
> Am I wrong?

I'm not 100% sure to be honest.  Let's check with Haik?  If you're right about that, consider the r- an r+.  :-)

Comment 10

[Haik Aftandilian [:haik]]

That's right. If we have a file process running, then the pref must already be set to allow file processes.

But why do we permit a file process to create files when dom.file.createInChild=false?

Forwarding :needinfo to Bob to give him a chance to respond because he's doing the work for file content processes.

Comment 11

[Bob Owen (:bobowen)]

(In reply to Haik Aftandilian [:haik] from comment #10)
> That's right. If we have a file process running, then the pref must already
> be set to allow file processes.
> 
> But why do we permit a file process to create files when
> dom.file.createInChild=false?

I just have the same question.
I haven't followed this bug too closely, but I thought this was just about read access from comment 2 (which the file content process already has), I don't think we should be creating files for it.

I'm also not too clear on why the file content process needs this API, if the normal web content one doesn't.

Comment 12

[Andrea Marchesini [:baku]]

The reason why we need this feature for file content process is because of XHR. When an XHR wants a blob as result and it opens a file: URL, instead reading the content and creating a Blob from that data, we create a Blob pointing directly to the file on the filesystem.

This optimization happens in the content process:
https://dxr.mozilla.org/mozilla-central/source/dom/xhr/XMLHttpRequestMainThread.cpp#2214

Comment 13

[Bob Owen (:bobowen)]

baku has explained this to me on IRC (thanks again) it is just to do with blob type XHRs for file:// URIs, which would only be allowed in the file content process.
So, this looks good.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: file1.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easy at all. As you can see in the description, the way this issue is used is via a custom version of the content child, where a internal only File CTOR is exposed to content.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Here make that IPC method available only for testing and for file-content-process.

Which older supported branches are affected by this flaw?

Only aurora.

If not all supported branches, which bug introduced the flaw?

bug 1335536

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Easy to port.

How likely is this patch to cause regressions; how much testing does it need?

low. We basically introduce a new pref and set it to true in a couple of tests. The rest, if the IPC method is called and the pref is set to false, we kill the process. It's green on try.

Comment 15

[Haik Aftandilian [:haik]]

(In reply to Andrea Marchesini [:baku] from comment #12)
> The reason why we need this feature for file content process is because of
> XHR. When an XHR wants a blob as result and it opens a file: URL, instead
> reading the content and creating a Blob from that data, we create a Blob
> pointing directly to the file on the filesystem.
> 
> This optimization happens in the content process:
> https://dxr.mozilla.org/mozilla-central/source/dom/xhr/
> XMLHttpRequestMainThread.cpp#2214

Thanks for the explanation.

Limiting this to the file process reduces the exposure of the API, but we should still consider if the benefit of the optimization is worth the risk. Just checked with Bob and there is a case where the file content process will be used to render web content--when a local page has an iframe that uses web content.

Comment 16

[Haik Aftandilian [:haik]]

(In reply to Haik Aftandilian [:haik] from comment #15)
> Limiting this to the file process reduces the exposure of the API, but we
> should still consider if the benefit of the optimization is worth the risk.
> Just checked with Bob and there is a case where the file content process
> will be used to render web content--when a local page has an iframe that
> uses web content.

And you can ignore that comment. As Bob pointed out to me on IRC, the file content process already has read access to the filesystem so whether or not the API allowed is a moot point (as long as the API is read only.)

Comment 17

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk but only for the fix, not the test.
Please nominate a patch for Aurora too so we can avoid shipping the vuln.

*After* the fix is checked into *both* Aurora and Trunk, *then* you can check in the test. I don't want a test to be checked in, have the fix not make it to Aurora for some reason, and then we self 0day ourselves on Aurora and ship the vuln.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8849019 [details] [diff] [review]
file1.patch

Generally speaking, fixes for security bugs should never include the test in the patch. You should open another bug to check in the patch after we ship the fix (or get into into all affected places if we haven't shipped the problem yet).

Comment 19

[Andrea Marchesini [:baku]]

There are no tests, I had to set a pref in a couple of existing tests.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fba648d8582e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 22

[Andrea Marchesini [:baku]]

Comment on attachment 8849019 [details] [diff] [review]
file1.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1335536
[User impact if declined]: If the child process has been hacked, it can send a IPC message in order to retrieve readonly access to any file on the filesystem.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: impossible to do
[Needs manual test from QE? If yes, steps to reproduce]:  impossible to do, except having a custom version of the content process.
[List of other uplifts needed for the feature/fix]: 1349276
[Is the change risky?]: low
[Why is the change risky/not risky?]: 
low. We basically introduce a new pref and set it to true in a couple of tests. The rest, if the IPC method is called and the pref is set to false, we kill the process. It's green on try.
[String changes made/needed]: none

I would like to port this to aurora together with bug 1349276 when reviewed.

Comment 23

[(no longer active)]

What is bug 1349276?  I'd like to take a look at that one also before this lands.  Can you please CC me on that one too?

Comment 24

[(no longer active)]

(In reply to :Ehsan Akhgari (busy) from comment #23)
> What is bug 1349276?  I'd like to take a look at that one also before this
> lands.  Can you please CC me on that one too?

Thanks, this is good to go. :-)

Comment 25

[Gerry Chang [:gchang]]

Comment on attachment 8849019 [details] [diff] [review]
file1.patch

Fix a sec-high. Aurora54+.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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