1379276 - (CVE-2018-5107) chrome process printing code reads path from content process without checking if it's a symlink

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement, P3)

Description

[Alex Gaynor [:Alex_Gaynor]]

https://dxr.mozilla.org/mozilla-central/source/layout/printing/ipc/RemotePrintJobParent.cpp?q=RemotePrintJobParent%3A%3APrintPage&redirect_type=single#100-127

Is the relevant code, it accepts a path from the content process and then opens it up. On macOS at least, the content process can write a symlink into that directory, causing it to open an arbitrary file on disk.

It looks like the expected format of the the file is a custom binary format, so this probably can't be used to leak the full contents of arbitrary files, but it needs closer inspection: https://dxr.mozilla.org/mozilla-central/source/layout/printing/PrintTranslator.cpp?q=%2Bfunction%3A%22mozilla%3A%3Alayout%3A%3APrintTranslator%3A%3ATranslateRecording%28std%3A%3Aistream+%26%29%22&redirect_type=single#28

Unfortunately I haven't found a way to use the macOS sandboxing policy language to disallow creating symlinks. I'm going to file a bug with apple requesting it. Therefore any place the chrome process opens up paths in the content process's temporary directory, it needs to validate that they aren't symlinks -- my understanding (correct if wrong!) is that printing is the only place we do this. (I wonder if we should instead generate a pipe-pair and send the content process the write end, to avoid needing to read from this path on disk in chrome).

(I put this bug in sandboxing, but maybe it belongs in printing?) I believe this should be sec-low or sec-moderate (it only really affects nightly because the sandbox doesn't prevent filesystem access directly on other release channels!)

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

It also looks like passing a non-existant file path will cause |PrintTranslator::TranslateRecording| to read uninitialized memory.

Comment 2

[Bob Owen (:bobowen)]

Yes opening the files in the parent and passing down the handle, was something I wanted to look into here.
I think on Windows symlinks can only be created by Administrator.

I guess using a pipe essentially does the same thing?
Would there be size limitations here though?

Comment 3

[Bob Owen (:bobowen)]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #1)
> It also looks like passing a non-existant file path will cause
> |PrintTranslator::TranslateRecording| to read uninitialized memory.

I agree that it would be good to check that the file exists (along with the symlink check for MacOS and linux, although doesn't hurt on Windows as well). Better still to pass down some Handle/FD from the parent, if not too difficult.

However, I think if the file doesn't exist the ifstream will just fail to open and subsequent reads will fail.
This does mean that magicInt will remain uninitialised, but it would then almost certainly fail the following check.
If not there in the next checks or the call to good().
We could initialize it anyway, to make sure, given ReadElement doesn't indicate failure (not quite sure why that is).

When you say "read uninitialised memory" do you mean this uint32_t (magicInt)?

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #0)
...
> It looks like the expected format of the the file is a custom binary format,
> so this probably can't be used to leak the full contents of arbitrary files,
> but it needs closer inspection:

How could this leak the contents of files, wouldn't that need the parent process to pass something back to the child for it to read?
The main risk here is an attack on the parent process I think, but the child can already write whatever it wants into this file.
This is all somewhat mitigated by the fact that you have to convince someone to print first before the attack.

> ... Therefore any place the chrome process opens up paths in the
> content process's temporary directory, it needs to validate that they aren't
> symlinks -- my understanding (correct if wrong!) is that printing is the
> only place we do this.

It's the only case that I know for sure, I think we might write to it for some crash reporting data.

I know we create temporary files for media (for the cache I think) and blobs, but they are created in the parent [1].
We might be able to use this for printing or at least some of the code for a parent driven version of it.
One complication is that I'm not sure how easy it is to create an ofstream from an FD.
Although it looks like there have been some changes to further abstract the stream required for the recording.

[1] http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/dom/ipc/ContentChild.cpp#3456

Comment 4

[Bob Owen (:bobowen)]

Thanks for looking at this code by the way. :-)

It's definitely an area for concern as a possible route for exploits, so it's great to get more eyes on it.

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

Yes, when I said "read initialized memory", I was talking about |magicInt|, I don't know how seriously we consider uninitialized-stack-reads.

As for how it could leak the contents of the files, I don't _think_ it can. If the results were put in some file readable by content, that'd be a problem (for files properly formatted in this graphs serialization file, heh). I suppose if you physically printed the symlinked-to-file that'd be an interesting attack :-)

Agreed that the best way to resolve is to switch to passing around a handle/fd instead a filename. (Do we have a document anywhere that lists all the places that use content temp?)

a) Is there someone from a printing/gfx team we should add to this bug?
b) Is there good prior art to look at for passing the handle/fd to the child?

Comment 6

[Bob Owen (:bobowen)]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #5)
...
> Agreed that the best way to resolve is to switch to passing around a
> handle/fd instead a filename. (Do we have a document anywhere that lists all
> the places that use content temp?)

Not that I am aware of.

> a) Is there someone from a printing/gfx team we should add to this bug?

Maybe jwatt (added) at least so he's aware, as he's been dealing with a fair amount of printing stuff.

> b) Is there good prior art to look at for passing the handle/fd to the child?

That code I mentioned does it, if you look at AnonymousTemporaryFileRequestor.
I think haik might also have done something similar for the webext change, but he might have used existing functionality.

Getting the handle might be the easy part, we probably need to think about how we use it in the child first before we go too far down that road.

Comment 7

[Haik Aftandilian [:haik]]

(In reply to Bob Owen (:bobowen) from comment #6)
> > b) Is there good prior art to look at for passing the handle/fd to the child?
> 
> That code I mentioned does it, if you look at
> AnonymousTemporaryFileRequestor.
> I think haik might also have done something similar for the webext change,
> but he might have used existing functionality.

For the moz-extension remoting, there are two ways the file access is remoted. One uses AutoIPCStream and the other sends a bare FD to the child.

When the child is requesting a single non-JAR file from the parent, we use an AutoIPCStream. ExtensionProtocolHandler::NewStream() returns a FileInputStream to NeckoParent::RecvGetExtensionStream() and the Necko code creates an AutoIPCStream to send to the child. The child can then read from the input stream it receives. The AutoIPCStream implementation actually passes the FD to the child so data is _not_ streamed over IPC. See the comments on AutoIPCStream: http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/ipc/glue/IPCStreamUtils.h#123 This seems like a good possibility.

The other way is when the child is requesting a JAR file FD from the parent, we use the FileDescriptor IPDL type in a new Necko IPDL message which handles IPC serialization and "just works". In ExtensionProtocolHandler::NewFD(), the parent opens an existing file and gets its FD to send to the child. The child then creates an nsIFile using the received FD (which is slightly ugly because the nsIFile implementation FileDescriptorFile doesn't support all the nsIFile methods.)

:baku has done a lot of work to make this streamlined and might have a better recommendation.

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

The symlink part of this is addressed by 1379803 - assuming that Windows and Linux are also able to lock out symlinks.

Comment 9

[Haik Aftandilian [:haik]]

Even with 1379803, I think it would be good for the parent to make sure it's not getting a symlink (assuming we don't change the code to use FD's only) providing some defense in depth.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

On Linux this might also apply to hard links, but I don't think we need to allow link() at all — the only thing we saw using it was an input method plugin, and we stopped loading those in content processes because they don't directly handle input events.

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

Patch in bug 1319423 fixes this issue.

Comment 12

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

For the record, comment #10 turned out to be not quite correct; see bug 1380701 comment #23.