Closed
Bug 1272772
Opened 9 years ago
Closed 9 years ago
Inline system.sb and remove unneeded rules
Categories
(Core :: Security: Process Sandboxing, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: haik, Assigned: haik)
Details
(Whiteboard: sbmc1)
Attachments
(3 files, 3 obsolete files)
6.34 KB,
patch
|
Details | Diff | Splinter Review | |
3.57 KB,
patch
|
Details | Diff | Splinter Review | |
3.71 KB,
patch
|
Details | Diff | Splinter Review |
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 | ||
Updated•9 years ago
|
Assignee: nobody → haftandilian
Whiteboard: sbmc1
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Attachment #8755174 -
Flags: review?(bobowen.code)
Assignee | ||
Updated•9 years ago
|
Attachment #8755175 -
Flags: review?(bobowen.code)
Assignee | ||
Updated•9 years ago
|
Attachment #8755176 -
Flags: review?(bobowen.code)
Comment 4•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8755175 -
Flags: review?(bobowen.code)
Updated•9 years ago
|
Attachment #8755176 -
Flags: review?(bobowen.code)
Assignee | ||
Updated•9 years ago
|
Attachment #8755174 -
Flags: review?(gpascutto)
Assignee | ||
Updated•9 years ago
|
Attachment #8755175 -
Flags: review?(gpascutto)
Assignee | ||
Updated•9 years ago
|
Attachment #8755176 -
Flags: review?(gpascutto)
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
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 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
(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.
Assignee | ||
Comment 9•9 years ago
|
||
(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
...
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8755174 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8755175 -
Attachment is obsolete: true
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8755176 -
Attachment is obsolete: true
Assignee | ||
Comment 13•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 14•9 years ago
|
||
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
Comment 15•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b01ae5885b0b
https://hg.mozilla.org/mozilla-central/rev/fe9c2571c130
https://hg.mozilla.org/mozilla-central/rev/4a08f1841ee0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•