Closed Bug 1332522 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Security: Process Sandboxing, defect)

53 Branch
Unspecified
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox53 --- wontfix
firefox54 --- verified
firefox55 --- verified

People

(Reporter: haik, Assigned: haik)

References

Details

(Whiteboard: sbmc2)

Attachments

(2 files, 1 obsolete file)

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.
Assignee: nobody → haftandilian
Blocks: 1147911
OS: Unspecified → Mac OS X
Whiteboard: sbmc2
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.
Attachment #8829770 - Flags: feedback?(gpascutto)
Attachment #8829770 - Flags: feedback?(bobowencode)
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
Attachment #8829770 - Flags: feedback?(bobowencode)
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
Attachment #8830968 - Flags: review?(bobowencode) → review+
Attachment #8829770 - Attachment is obsolete: true
Attachment #8829770 - Flags: feedback?(gpascutto)
(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.
Duplicate of this bug: 1334288
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
Attachment #8830969 - Flags: review?(gpascutto) → review+
Keywords: checkin-needed
Autoland couldn't rebase this for landing.
Flags: needinfo?(haftandilian)
Keywords: checkin-needed
(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.
Flags: needinfo?(haftandilian)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/07b8ccb72585
https://hg.mozilla.org/mozilla-central/rev/a4117383c886
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.