Categories

(Core :: DOM: Content Processes, defect)

Tracking

(bug RESOLVED as FIXED for Firefox 62 which is tracked for Firefox 61)

People

(Reporter: Alex_Gaynor, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-disclosure, sec-moderate, Whiteboard: [adv-main61+][adv-esr52.9+][adv-esr60.1+][post-critsmash-triage])

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.

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

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?

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

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

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.

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

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

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

Consider this patch again. Read the latest comments.

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 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.

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

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

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

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 on attachment 8973241 [details] [diff] [review]
security.patch

sec-high fix for 52.9esr and 60.1esr

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.