1415508 - (CVE-2019-9802) FTPChannelParent::RecvDivertOnDataAvailable - Chrome Process Info Leak

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect, P3)

Description

[Stephen Fewer]

As part of a code review I am conducting for Paul Theriault against the sandbox, the following vulnerability has been discovered.

A compromised content process can initiate an FTP file download which will in turn generate a mozilla::net::PFTPChannelChild object to handle downloading the files contents for rendering in the content process. If the MIME type is not supported for rendering in the content process, the file will be passed over to the Chrome process so the user may elect to either open or download the file. 

When the file is to be passed over to the Chrome process, the child content process will call FTPChannelParent::RecvDivertOnDataAvailable in the parent chrome process via the PFTPChannel IPDL protocol. The child may supply a length value which is greater than the total size of the file to be downloaded via the 'count' parameter.

> mozilla::ipc::IPCResult
> FTPChannelParent::RecvDivertOnDataAvailable(const nsCString& data, // <-- a byte buffer holding the files contents.
>                                             const uint64_t& offset,
>                                             const uint32_t& count) // <-- attacker can supply a count value greater than the size of the above data buffer.
> {
>   if (NS_WARN_IF(!mDivertingFromChild)) {
>     MOZ_ASSERT(mDivertingFromChild,
>                "Cannot RecvDivertOnDataAvailable if diverting is not set!");
>     FailDiversion(NS_ERROR_UNEXPECTED);
>     return IPC_FAIL_NO_REASON(this);
>   }
> 
>   // Drop OnDataAvailables if the parent was canceled already.
>   if (NS_FAILED(mStatus)) {
>     return IPC_OK();
>   }
> 
>   mEventQ->RunOrEnqueue(new FTPDivertDataAvailableEvent(this, data, offset,
>                                                         count));
>   return IPC_OK();
> }

The FTPDivertDataAvailableEvent event will in turn call FTPChannelParent::DivertOnDataAvailable which will create an new nsIInputStream to represent the data buffer to process. This stream can have an arbitrary length value supplied by the attacker. If the length value is greater than the data buffer supplied by the attacker, then the adjacent contents of memory can be read from when OnDataAvailable processes the stream.

> void
> FTPChannelParent::DivertOnDataAvailable(const nsCString& data,
>                                         const uint64_t& offset,
>                                         const uint32_t& count)
> {
>   LOG(("FTPChannelParent::DivertOnDataAvailable [this=%p]\n", this));
> 
>   if (NS_WARN_IF(!mDivertingFromChild)) {
>     MOZ_ASSERT(mDivertingFromChild,
>                "Cannot DivertOnDataAvailable if diverting is not set!");
>     FailDiversion(NS_ERROR_UNEXPECTED);
>     return;
>   }
> 
>   // Drop OnDataAvailables if the parent was canceled already.
>   if (NS_FAILED(mStatus)) {
>     return;
>   }
> 
>   nsCOMPtr<nsIInputStream> stringStream;
>   nsresult rv = NS_NewByteInputStream(getter_AddRefs(stringStream), data.get(), // <-- a pointer to the start or the data bufffer
>                                       count, NS_ASSIGNMENT_DEPEND); // <-- the count value which may be > then the size of the data buffer
>   if (NS_FAILED(rv)) {
>     if (mChannel) {
>       mChannel->Cancel(rv);
>     }
>     mStatus = rv;
>     return;
>   }
> 
>   AutoEventEnqueuer ensureSerialDispatch(mEventQ);
> 
>   rv = OnDataAvailable(mChannel, nullptr, stringStream, offset, count); // <-- continue to process the data and download it to disk.
> 
>   stringStream->Close();
>   if (NS_FAILED(rv)) {
>     if (mChannel) {
>       mChannel->Cancel(rv);
>     }
>     mStatus = rv;
>   }
> }

Firefox's default action is to present the user with a dialog to either open or save the file. However in the background the contents of the file will be downloaded to a temp directory with no user interaction required. For example on a Win10 machine I get a file like "C:\Users\stephen\AppData\Local\Temp\qfamDcQE.zip.part". This location will obviously be different across OS platforms which may effect how easily an attacker can read it back depending on the sandbox implementation for that OS.

If an attacker can read this files contents - 

    * They would need read permission explicitly for this file and then either discover or brute force the 'qfamDcQE' part of the file name.
    
    * They would need read permissions on the temp directory. Then discovering the file name to read should be trivial as directory enumeration API access should be allowed (e.g FindFirstfile/FindNextFile on Windows).

The attacker can now leak out memory from the Chrome process. This could be leveraged by an attacker to either bypass ASLR for an additional memory corruption based sandbox escape exploit, or to try and leak out sensitive contents from the Chrome process (Perhaps session keys or similar).

To reproduce this issue:

    * Visit ftp://speedtest.tele2.net/ in a content process.
    * Optional: Run a tool like procmon.exe to monitor firefox.exe file access if you want to see the .zip.part file being written.
    * Attach WinDbg to this content process.
    * Break into WinDbg and set a breakpoint via the command "bp xul!mozilla::net::PFTPChannelChild::SendDivertOnDataAvailable". Now continue execution of the process.
    * Click a file to download, for example ftp://speedtest.tele2.net/1KB.zip which is 1024 (0x400) bytes in size.
    * WinDbg will break in SendDivertOnDataAvailable. We now simulate a compromised content process. Using the WinDbg Locals window, edit the count value from 0x400 to something larger like 0x1400. Now press F5 to continue execution.
    * Firefox will display a save/open dialog, you can ignore this.
    * The "XXXXXXXX.zip.part" file will automatically be written to disk in the background by the Chrome process. It will be written to your AppData\Local\Temp\ directory. Inspecting this file will show the file size is 0x1400 and at offset 0x400 (the original file size) are 0x1000 bytes of the Chrome processes memory which was adjacent to the original data buffer from the original FTPChannelParent::RecvDivertOnDataAvailable call.
    * Note: There is a chance the Chrome process will read into unallocated memory and access violate. The attacker would need to perform some form of heap massaging in order to influence the layout of the heap and ensure this doesn't happen and that they leak some thing interesting.

Comment 1

[Honza Bambas (:mayhemer)]

(In reply to Stephen Fewer from comment #0)
> >   nsresult rv = NS_NewByteInputStream(getter_AddRefs(stringStream), data.get(), // <-- a pointer to the start or the data bufffer
> >                                       count, NS_ASSIGNMENT_DEPEND); // <-- the count value which may be > then the size of the data buffer

cropping |count| as min(count, data.Length()) is the solution here.

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Should the |min| call be done there, or when we |SendDivertOnDataAvailable| (or maybe even before then)? I'm not familiar with this code enough to say for certain which makes sense.

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1415508 - use Span in constructing a byte input stream; r?mayhemer

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Paul, I think this patch also fixes bug 1505678. On that bug you suggest there are other unsafe callsites, can you verify this patch fixes all of them?

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

The query I ran was something like "find all the places we recieve a nsCString arg in the parent which has a name which contains the substring "data". That yeilds this list:

RecvSetCustomCursor aCursorData TabParent.cpp#1748 - original bug, fixed.
RecvOnDataAvailable data PSMContentListener.cpp#232 - just calls data.append() so i think its ok?
RecvDivertOnDataAvailable data FTPChannelParent.cpp#272 - this bug
RecvWriteData data AltDataOutputStreamParent.cpp#30 - mOutputStream->Write(... data.Length() so I think this is safe?
RecvDivertOnDataAvailable data HttpChannelParent.cpp#1118 - fixed by your patch
RecvOnDataAvailable data ExternalHelperAppParent.cpp#15 - fixed by your patch

So I think you got all the cases. I'm leaving the need-info for me to remind me to look at this issue more broadly (ie without the filter on "data" but you got all the specific cases I had).

Comment 6

[Paul Theriault [:pauljt] (no longer reading bugmail)]

PS Live query is here if you want to view/edit it yourself: https://mozilla.demo.semmle.com/query/437877786200575246/

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/7bffd747d54a

I'm not sure if we want this on ESR60 or not (I'm inclined towards no unless there's a strong reason for doing so), but this at least grafts cleanly to Beta if we want it there.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7bffd747d54a

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Is this something you would like to uplift? We are kind of late in beta but if you would like, we can give it a try.

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 9045680 [details]
Bug 1415508 - use Span in constructing a byte input stream; r?mayhemer

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: Family of security bugs
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Code is well covered, fix was a mostly straightforward refactor.
• String changes made/needed:

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9045680 [details]
Bug 1415508 - use Span in constructing a byte input stream; r?mayhemer

Fix for several sec issues, may be a little risky but seems worth it.
OK to uplift for beta 13.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/70a034af6e4b

Comment 13

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #10)

• Is this code covered by automated tests?: Yes
• Needs manual test from QE?: No

Setting qe-verify- per comment 10.