Closed Bug 1272772 Opened 3 years ago Closed 3 years ago

Inline system.sb and remove unneeded rules

Categories

(Core :: Security: Process Sandboxing, defect)

49 Branch
Unspecified
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: haik, Assigned: haik)

Details

(Whiteboard: sbmc1)

Attachments

(3 files, 3 obsolete files)

The OS X content process sandbox includes the /System/Library/Sandbox/Profiles/system.sb sandbox rules file which has some rules we probably don't need. We should move what we do need from system.sb to be inline with our rules and not depend on system.sb.

system.sb seems to be meant for system utilities (but it's intended use is not well documented) and it's probably better to not depend on the file in case OS X silently introduces new rules we don't want for the content process. OS X 10.11 system.sb includes file-read access to some files in /private/etc and it defines some macros that are not used by our rule set, namely "system-graphics" and "system-network".
Assignee: nobody → haftandilian
Whiteboard: sbmc1
This patch removes some rules from the content sandbox that were originally picked up from system.sb. My own browsing tests and a try run didn't turn up any problems. Some of the rules have very little documentation from Apple so there is a degree of trial-and-error here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61451b139430
Attachment #8755174 - Flags: review?(bobowen.code)
Attachment #8755175 - Flags: review?(bobowen.code)
Attachment #8755176 - Flags: review?(bobowen.code)
Comment on attachment 8755174 [details] [diff] [review]
Inlines rules from /System/Library/Sandbox/Profiles/system.sb

I'm going to have to pass on these reviews, sorry.

I don't know enough about OSX or our use of it to make any real judgements here.
Attachment #8755174 - Flags: review?(bobowen.code)
Attachment #8755175 - Flags: review?(bobowen.code)
Attachment #8755176 - Flags: review?(bobowen.code)
Attachment #8755174 - Flags: review?(gpascutto)
Attachment #8755175 - Flags: review?(gpascutto)
Attachment #8755176 - Flags: review?(gpascutto)
Comment on attachment 8755174 [details] [diff] [review]
Inlines rules from /System/Library/Sandbox/Profiles/system.sb

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

rs+ assuming no copypaste errors
Attachment #8755174 - Flags: review?(gpascutto) → review+
Comment on attachment 8755175 [details] [diff] [review]
Removes unused macros from the inlined system.sb rules

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

I'd assume somewhere in our ruleset we have rules that allow the same things as these. It might be good to figure out what they are, and if these macros can be used to get the functionality we need with less stuff whitelisted.
Attachment #8755175 - Flags: review?(gpascutto) → review+
Comment on attachment 8755176 [details] [diff] [review]
Removes some rules from the inlined system.sb rule set

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

Looks ok but see remarks.

::: security/sandbox/mac/Sandbox.mm
@@ +167,1 @@
>    ";;; Allow read access to standard system paths.\n"

You changed some of the commenting layout for consistency, but then I think you missed this one?

@@ -207,5 @@
> -  "(allow network-outbound\n"
> -  "  (literal \"/private/var/run/asl_input\")\n"
> -  "  (literal \"/private/var/run/syslog\"))\n"
> -  "\n"
> -  ";;; Allow creation of core dumps.\n"

This might mess with debugging?

@@ -214,5 @@
> -  "    (vnode-type REGULAR-FILE)))\n"
> -  "\n"
> -  ";;; Allow IPC to standard system agents.\n"
> -  "(allow ipc-posix-shm-read*\n"
> -  "  (ipc-posix-name #\"apple.shm.notification_center\")\n"

Do notifications still work?

https://bugzilla.mozilla.org/show_bug.cgi?id=852648
Attachment #8755176 - Flags: review?(gpascutto) → review+
(In reply to Gian-Carlo Pascutto [:gcp] from comment #6)
> Comment on attachment 8755175 [details] [diff] [review]
> Removes unused macros from the inlined system.sb rules
> 
> Review of attachment 8755175 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd assume somewhere in our ruleset we have rules that allow the same things
> as these. It might be good to figure out what they are, and if these macros
> can be used to get the functionality we need with less stuff whitelisted.

There's some overlap between the system.sb graphics macro "system-graphics" and what we already have for content. I agree it's worth going through the two carefully. Theoretically, system-graphics tells us what Apple's recommendation is for accessing graphics hardware. We might use that to see if we have extra things not in those rules that we don't need or if we're missing anything and what the effect is.

Just to reiterate for anyone reading the bug later, the system.sb file defines these two macros for graphics and networking, but we aren't using them anywhere so removing them shouldn't affect our sandbox.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #7)
> Looks ok but see remarks.
> 
> ::: security/sandbox/mac/Sandbox.mm
> @@ +167,1 @@
> >    ";;; Allow read access to standard system paths.\n"
> 
> You changed some of the commenting layout for consistency, but then I think
> you missed this one?

Yes, I did, thanks. Will fix before pushing.

> @@ -207,5 @@
> > -  "(allow network-outbound\n"
> > -  "  (literal \"/private/var/run/asl_input\")\n"
> > -  "  (literal \"/private/var/run/syslog\"))\n"
> > -  "\n"
> > -  ";;; Allow creation of core dumps.\n"
> 
> This might mess with debugging?

In my tests, it didn't seem to prevent our ability to dump cores in /cores.

/cores isn't used by default on OS X. The user has to enable core dumps. By default, the Crash Reporter handles storing crash logs (which show stacks and register values and other crash info).

I tested enabling cores with "ulimit -c unlimited" and was able to kill -QUIT the content process and get a core dump in /cores from the plugin-container binary.

> @@ -214,5 @@
> > -  "    (vnode-type REGULAR-FILE)))\n"
> > -  "\n"
> > -  ";;; Allow IPC to standard system agents.\n"
> > -  "(allow ipc-posix-shm-read*\n"
> > -  "  (ipc-posix-name #\"apple.shm.notification_center\")\n"
> 
> Do notifications still work?
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=852648

No problems were uncovered running mochitests and manual tests of some web notification demos.

I poked around a bit and those appear to be forwarded to the parent via

  frame #0 XUL`mozilla::OSXNotificationCenter::ShowAlert() at OSXNotificationCenter.mm:260
  frame #1 XUL`(anonymous namespace)::ShowWithBackend() at nsAlertsService.cpp:138
  frame #2 XUL`nsAlertsService::ShowAlert() at nsAlertsService.cpp:254
  frame #3 XUL`mozilla::dom::ContentParent::RecvShowAlert() at ContentParent.cpp:4446
  frame #4 XUL`mozilla::dom::PContentParent::OnMessageReceived() at PContentParent.cpp:5051
  ...
(In reply to Haik Aftandilian [:haik] from comment #9)
> (In reply to Gian-Carlo Pascutto [:gcp] from comment #7)
> > This might mess with debugging?
> 
> In my tests, it didn't seem to prevent our ability to dump cores in /cores.

I looked at this a bit more. I was able to kill the content process and generate a core because we have the following rules

  "; the following rules should be removed when printing and \n"
  "; opening a file from disk are brokered through the main process\n"                 
  "    (if\n"                                                                          
  "      (< sandbox-level 2)\n"
  "      (allow file*\n"
  "          (require-not\n"                                                           
  "              (home-subpath \"/Library\")))\n"

which allow writing to any directory that the OS permits the user to write to except for ~/Library. We want to remove this rule as soon as we can.

Once we remove it, saving core dumps to /cores won't work. But since generating cores in /cores is OFF by default on OS X and I only expect developers to turn it on, I think this is the right choice.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b01ae5885b0b
Inline system.sb and remove unneeded rules (inline system.sb rules); r=gcp
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe9c2571c130
Inline system.sb and remove unneeded rules (removes unused macros); r=gcp
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a08f1841ee0
Inline system.sb and remove unneeded rules (removes unneeded rules); r=gcp
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.