1332522 - [Mac] remove ~/Library read restriction from file content process sandbox

Status: VERIFIED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Haik Aftandilian [:haik]]

On Nightly (because security.sandbox.content.level=2) the Mac content sandbox prevents read access to ~/Library excluding $PROFILE/{extensions,weave,chrome}. The file-content process uses the same sandbox rules, but doesn't need this restriction because it should be allowed to read anywhere on the filesystem the user has access to.

Comment 1

[Haik Aftandilian [:haik]]

Attachment: Passes "file" or "web" to the content process, depending on process type

Requesting some general feedback on passing a new command line argument to content processes ("file" or "web") to indicate which type of content process it is. This allows the content process to use different sandbox settings for file and web content processes.

For debugging, on Mac at least, I found it's really convenient to be able to determine what type of process you are looking at from /bin/ps output.

Comment 2

[Bob Owen (:bobowen)]

Comment on attachment 8829770 [details] [diff] [review]
Passes "file" or "web" to the content process, depending on process type

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

The content process already knows what remote type it is, although it seems that I didn't do that early enough for it to be used in sandboxing. I didn't realise that the Mac sandbox policy was all determined by the child. On Windows it's set up front by the parent.

I think we should move the call at [1] to [2] (with a comment saying this must happen before the SendSetProcessSandbox).

It may well be useful to have things on the command line for debugging.
I'm not sure what the general feeling/policy is over adding extra command line parameters.
If we decide that it's a good enough reason to have it, we should probably pass the remote type, not just file and web and remove the SendRemoteType call.
You can pass extra arguments to LaunchAndWaitForProcessHandle.


[1] https://hg.mozilla.org/mozilla-central/file/8ff550409e1d/dom/ipc/ContentParent.cpp#l1095
[2] https://hg.mozilla.org/mozilla-central/file/8ff550409e1d/dom/ipc/ContentParent.cpp#l1861

Comment 3

[Haik Aftandilian [:haik]]

Attachment: Bug 1332522 - Part 1 - Send the remote type to the child before enabling the content sandbox;

Call SendRemoteType() before calling SendSetProcessSandbox() so the
child can use the remote type value when setting up the sandbox.

Review commit: https://reviewboard.mozilla.org/r/106216/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106216/

Comment 4

[Haik Aftandilian [:haik]]

Attachment: Bug 1332522 - Part 2 - Remove read restrictions from level 2 policy for file content processes;

Update MacSandboxInfo struct to include file system read flag and remove
filesytem read restrictions from the file content process sandbox.

Review commit: https://reviewboard.mozilla.org/r/106224/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106224/

Comment 5

[Bob Owen (:bobowen)]

Comment on attachment 8830968 [details]
Bug 1332522 - Part 1 - Send the remote type to the child before enabling the content sandbox;

https://reviewboard.mozilla.org/r/106216/#review108986

Comment 6

[Haik Aftandilian [:haik]]

(In reply to Bob Owen (:bobowen) from comment #2)
> Comment on attachment 8829770 [details] [diff] [review]
> Passes "file" or "web" to the content process, depending on process type
> 
> Review of attachment 8829770 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The content process already knows what remote type it is, although it seems
> that I didn't do that early enough for it to be used in sandboxing. I didn't
> realise that the Mac sandbox policy was all determined by the child. On
> Windows it's set up front by the parent.
> 
> I think we should move the call at [1] to [2] (with a comment saying this
> must happen before the SendSetProcessSandbox).
> 
> It may well be useful to have things on the command line for debugging.
> I'm not sure what the general feeling/policy is over adding extra command
> line parameters.
> If we decide that it's a good enough reason to have it, we should probably
> pass the remote type, not just file and web and remove the SendRemoteType
> call.
> You can pass extra arguments to LaunchAndWaitForProcessHandle.
> 
> 
> [1]
> https://hg.mozilla.org/mozilla-central/file/8ff550409e1d/dom/ipc/
> ContentParent.cpp#l1095
> [2]
> https://hg.mozilla.org/mozilla-central/file/8ff550409e1d/dom/ipc/
> ContentParent.cpp#l1861

(Meant to send this before posting the code for review.) Thanks for the feedback. I've changed the code to send the remote type earlier in the content startup flow so Mac can use it to setup the sandbox.

I'll look into other ways of identifying the file content process for debugging and might file a follow up to add a flag identifying them by the command line arguments. Soon we'll also have extensions processes.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8830969 [details]
Bug 1332522 - Part 2 - Remove read restrictions from level 2 policy for file content processes;

https://reviewboard.mozilla.org/r/106224/#review109018

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Autoland couldn't rebase this for landing.

Comment 10

[Haik Aftandilian [:haik]]

Comment on attachment 8830968 [details]
Bug 1332522 - Part 1 - Send the remote type to the child before enabling the content sandbox;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106216/diff/1-2/

Comment 11

[Haik Aftandilian [:haik]]

Comment on attachment 8830969 [details]
Bug 1332522 - Part 2 - Remove read restrictions from level 2 policy for file content processes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106224/diff/1-2/

Comment 12

[Haik Aftandilian [:haik]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> Autoland couldn't rebase this for landing.

Synced with central. Problem should be fixed now. Sorry about that.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07b8ccb72585
Part 1 - Send the remote type to the child before enabling the content sandbox; r=bobowen
https://hg.mozilla.org/integration/autoland/rev/a4117383c886
Part 2 - Remove read restrictions from level 2 policy for file content processes; r=gcp

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/07b8ccb72585
https://hg.mozilla.org/mozilla-central/rev/a4117383c886

Comment 15

[Simona Badau, Desktop QA]

Reproduced the ~/Library read restriction using Nightly 53.0a1 (Build ID: 20170115030210) on Mac OS X 10.12.

Verified as fixed on the latest Nightly 55.0a1 (Build ID: 20170403030207) and Aurora 54.0a2 (Build ID: 20170403004002) on Mac OS X 10.12.