1459206 - (CVE-2018-12365) Arbitrary file listing (content disclosure?) by compromised content process

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Alex Gaynor [:Alex_Gaynor]]

The GetFilesRequest IPC method on PContentParent appears to allow a compromised child to list arbitrary files: https://searchfox.org/mozilla-central/source/dom/ipc/ContentParent.cpp#5238

I haven't worked out whether this is just directory listings, or whether it can also get contents.

The threat model for the sandbox should not allow a child process to access the filesystem without affirmative user consent (e.g. the UI shown by <input type=file>, drag-and-drop, etc.).

I'm not super familiar with this code or how it's used, so I'd appreciate feeback.

Comment 1

[Andrew McCreight [:mccr8]]

I'd guess that Andrea is most familiar with this.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: security.patch

We have a FileSystemSecurity component that knows the list of the paths sent to content processes via D&D and FilePicker. We should use this component here as well.

Ehsan, you reviewed FileSystemSecurity. Do you mind to take a look?

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

Removing sec-audit since this is confirmed. Should either be a sec-high or sec-moderate, not positive which.

Comment 4

[Andrea Marchesini [:baku]]

It's very important to have this check because here we _could_ leak file contents via DOM Blob objects.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Too late for 60 at this point.

Comment 6

[(no longer active)]

Comment on attachment 8973241 [details] [diff] [review]
security.patch

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

::: dom/ipc/ContentParent.cpp
@@ +5240,5 @@
>                                     const bool& aRecursiveFlag)
>  {
>    MOZ_ASSERT(!mGetFilesPendingRequests.GetWeak(aUUID));
>  
> +  if (!mozilla::Preferences::GetBool("dom.filesystem.pathcheck.disabled", false)) {

Is there a good reason why you aren't killing the child process here?

I think not doing so is wrong.

Please refactor this existing code into a new method on ContentParent, like DoFileSystemSecurityChecks(), and just call that method here.

Comment 7

[Andrea Marchesini [:baku]]

We are killing the content process: returning IPC_FAIL_NO_REASON(this) means killing the content process as far as I know.

Comment 8

[Andrea Marchesini [:baku]]

Note that in FileSystemRequest actor, we kill the content process on the main-thread, but the IPC protocol runs on PBackground:
https://searchfox.org/mozilla-central/source/dom/filesystem/FileSystemRequestParent.cpp#111-113

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

Yes, returning IPC_FAIL_NO_REASON(this) will kill the content process (an is friendlier to the fuzzer than just caling KillHard directly :-))

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8973241 [details] [diff] [review]
security.patch

Consider this patch again. Read the latest comments.

Comment 11

[(no longer active)]

Comment on attachment 8973241 [details] [diff] [review]
security.patch

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

OK, this makes sense given your comments.  Thanks!

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8973241 [details] [diff] [review]
security.patch

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

None. But if the content process is hacked, it will be able to retrieve the list of files, from a particular directory, as DOM Blob objects.

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

No.

Which older supported branches are affected by this flaw?

All. This method code is needed for webkitdirectory attribute. Bug 1265767.

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

Bug 1265767.

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

None.

Comment 13

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

sec-approval+ for trunk.
We'll want Beta and ESR patches for both of those branches made and nominated too.

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 8973241 [details] [diff] [review]
security.patch

Approval Request Comment
[Feature/Bug causing the regression]: webkitdirectory attribute
[User impact if declined]: if the content process is hacked, it can retrieve the list of files of a directory using this IPC method.
[Is this code covered by automated tests?]: n/a
[Has the fix been verified in Nightly?]: n/a
[Needs manual test from QE? If yes, steps to reproduce]: n/a
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: Similar approach is used in Entries API
[String changes made/needed]: none

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This was pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05ad0f66aa8f

Comment 16

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> This was pushed to inbound:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/05ad0f66aa8f

Whoops, that one got backed out for mochitest issues. It then re-landed as:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3ebab26f0d9

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8973241 [details] [diff] [review]
security.patch

Fixes a sec-high, approved for 61.0b4. We'll revisit the ESR requests later in the cycle.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5779de6a7028

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8973241 [details] [diff] [review]
security.patch

sec-high fix for 52.9esr and 60.1esr

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/61e14e969d80

Comment 22

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/4ef79fe9b3b735be610e1a855740ace89c1c49a5

Comment 23

[Julien Cristau [:jcristau]]

Followup build fix: https://hg.mozilla.org/releases/mozilla-esr52/rev/9ad16112044a136a0100f32d3bba3e30a862a375

Comment 24

[Daniel Veditz [:dveditz]]

sec-moderate seems more appropriate: a list of files is not as severe as reading the actual content or a sandbox escape that we call sec-high.