Closed Bug 1272764 Opened 3 years ago Closed 3 years ago

Remove OS X 10.6-10.8-Specific Sandboxing Code

Categories

(Core :: Security: Process Sandboxing, defect, trivial)

49 Branch
Unspecified
macOS
defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: haik, Assigned: haik)

References

Details

(Whiteboard: sbmc1)

Attachments

(3 files, 11 obsolete files)

874 bytes, text/plain
Details
5.36 KB, patch
Details | Diff | Splinter Review
25.58 KB, patch
Details | Diff | Splinter Review
We have some code in our content and plugin sandbox rule sets that are only needed to support OS X 10.8 and earlier OS versions. 

Two examples from Sandbox.mm[1]:

In the content Sandbox, we have some code that bypasses the sandbox for pre 10.9 versions:

  156 static const char contentSandboxRules[] =                                       
  ...
  169   "(if \n"
  170   "  (or\n"
  171   "    (< macosMinorVersion 9)\n"                                               
  172   "    (< sandbox-level 1))\n"                                                  
  173   "  (allow default)\n" 

In the plugin Sandbox, we have some rules specific to 10.6-10.9

  127 static const char pluginSandboxRules[] =
  ...
  132   // Illegal syntax on OS X 10.6, needed on 10.7 and up.
  133   "%s(allow iokit-open (iokit-user-client-class \"IOHIDParamUserClient\"))\n"
  134   // Needed only on OS X 10.6
  135   "%s(allow file-read-data (literal \"%s\"))\n"

1. https://dxr.mozilla.org/mozilla-central/rev/c3f5e6079284a7b7053c41f05d0fe06ff031db03/security/sandbox/mac/Sandbox.mm#127
Assignee: nobody → haftandilian
Depends on: 1255589
Summary: Remove OS X Sandbox Code for Unsupported OS X Releases → Remove OS X 10.6-10.8-Specific Sandboxing Code
Version: 48 Branch → 49 Branch
Whiteboard: sbmc1
Removes the code only needed for OS X 10.8 and earlier. This includes some version-dependent sandbox rules as well as some code for parsing out the OS X version.
Attached patch Fix indentation / white space (obsolete) — Splinter Review
Attachment #8755172 - Flags: review?(bobowen.code)
Attachment #8755173 - Flags: review?(bobowen.code)
Comment on attachment 8755172 [details] [diff] [review]
Removes code only used on OS X 10.8 and earlier

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

Couple of things need fixing I think.

These rules are bad for the brain and I still don't feel very qualified to review them. :-)

::: security/sandbox/mac/Sandbox.mm
@@ +28,5 @@
>    "(version 1)\n"
>    "(deny default)\n"
>    "(allow signal (target self))\n"
>    "(allow sysctl-read)\n"
> +  "(allow file-read-data (literal \"%s\"))\n"

Isn't it the other line we need?

If it is I assume we'd lose the |aInfo.pluginInfo.pluginPath.c_str(),| from the asprintf.

@@ +266,5 @@
>    "        (literal \"/private/var/run/cupsd\")\n"
>    "        (literal \"/private/var/run/mDNSResponder\"))\n"
>    "\n"
>    "; print preview\n"
> +  "    (if (allow lsopen))\n"

Should this still have the if?

@@ +324,1 @@
>    ")\n";

micronit: shouldn't it be this line that goes, although I guess you fix it in the next patch.

@@ +330,5 @@
> +    asprintf(&profile, pluginSandboxRules,
> +             aInfo.pluginInfo.pluginPath.c_str(),
> +             aInfo.pluginInfo.pluginBinaryPath.c_str(),
> +             aInfo.appPath.c_str(),
> +             aInfo.appBinaryPath.c_str());

Nothing to do with this change, but I wonder why we need to give read access to these last two.
Attachment #8755172 - Flags: review?(bobowen.code)
Comment on attachment 8755173 [details] [diff] [review]
Fix indentation / white space

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

One minor thing, once the first patch is fixed.

::: security/sandbox/mac/Sandbox.mm
@@ +111,1 @@
>    "            ))\n"

nit: This didn't get shifted.

In fact it looks like it was in the wrong position to start with and should have already have been two to the left, lined up with (begin.
Attachment #8755173 - Flags: review?(bobowen.code) → review+
(In reply to Bob Owen (:bobowen) from comment #4)
> Couple of things need fixing I think.
> 
> These rules are bad for the brain and I still don't feel very qualified to
> review them. :-)

OK. Thanks for taking a look.

> ::: security/sandbox/mac/Sandbox.mm
> @@ +28,5 @@
> >    "(version 1)\n"
> >    "(deny default)\n"
> >    "(allow signal (target self))\n"
> >    "(allow sysctl-read)\n"
> > +  "(allow file-read-data (literal \"%s\"))\n"
> 
> Isn't it the other line we need?

Yep, you're right. Will fix.

> If it is I assume we'd lose the |aInfo.pluginInfo.pluginPath.c_str(),| from
> the asprintf.
> 
> @@ +266,5 @@
> >    "        (literal \"/private/var/run/cupsd\")\n"
> >    "        (literal \"/private/var/run/mDNSResponder\"))\n"
> >    "\n"
> >    "; print preview\n"
> > +  "    (if (allow lsopen))\n"
> 
> Should this still have the if?

No, it shouldn't. Will fix. Apparently the sandbox_init call silently accepted that.

> @@ +324,1 @@
> >    ")\n";
> 
> micronit: shouldn't it be this line that goes, although I guess you fix it
> in the next patch.

Updated patch will fix this.

> 
> @@ +330,5 @@
> > +    asprintf(&profile, pluginSandboxRules,
> > +             aInfo.pluginInfo.pluginPath.c_str(),
> > +             aInfo.pluginInfo.pluginBinaryPath.c_str(),
> > +             aInfo.appPath.c_str(),
> > +             aInfo.appBinaryPath.c_str());
> 
> Nothing to do with this change, but I wonder why we need to give read access
> to these last two.

I agree it seems odd to require permission to read the app directory and the executable path (which resides in the app directory) when we're already running the executable. These use the "literal" sandbox rule so the rule only applies to the dir or file specified, not subdirs or contained files. I looked at 1056936 and 1012949, but didn't see an explanation. And some testing with the widevine plugin showed that aInfo.appBinaryPath.c_str() is ending up being the empty string. I'll look into that and file a bug if necessary.
Using test code, printed the sandbox policies before and after the fix. For content, used diff -w to ignore white space changes. Collected on OS X 10.11. This shows what the effect of these changes are on the actual policy passed to sandbox_init.
Addresses bobowen's comments and also removes the fprintf's to dump the policy I had accidentally left in.
Attachment #8755172 - Attachment is obsolete: true
Attachment #8756104 - Flags: review?(bobowen.code)
Attached patch Fix indentation / white space (obsolete) — Splinter Review
Made minor white space change due to changes in patch 1.
Attachment #8755173 - Attachment is obsolete: true
Attachment #8755173 - Attachment is obsolete: false
Attachment #8756106 - Attachment is obsolete: true
Attached patch Fix indentation / white space (obsolete) — Splinter Review
Made minor white space change due to changes in patch 1. (Re-attaching because I failed to obsolete earlier version. Sorry for the spam.)
Attachment #8755173 - Attachment is obsolete: true
Comment on attachment 8756104 [details] [diff] [review]
Removes code only used on OS X 10.8 and earlier

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

::: security/sandbox/mac/Sandbox.mm
@@ +350,5 @@
> +               aInfo.appTempDir.c_str(),
> +               getenv("HOME"));
> +    } else {
> +      fprintf(stderr,
> +        "Content sandbox disabled due to sandbox level setting\n");

Hadn't spotted that those fprintfs weren't already there.
I take it we can't use our normal logging mechanisms here.
Attachment #8756104 - Flags: review?(bobowen.code) → review+
(In reply to Bob Owen (:bobowen) from comment #11)
> Comment on attachment 8756104 [details] [diff] [review]
> Removes code only used on OS X 10.8 and earlier
> 
> Review of attachment 8756104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/sandbox/mac/Sandbox.mm
> @@ +350,5 @@
> > +               aInfo.appTempDir.c_str(),
> > +               getenv("HOME"));
> > +    } else {
> > +      fprintf(stderr,
> > +        "Content sandbox disabled due to sandbox level setting\n");
> 
> Hadn't spotted that those fprintfs weren't already there.

My fault for leaving it in. (It would be good to allow an environment variable to trigger turning on printing of the policy for debugging purposes. I always add those kind of printfs when I'm changing the policy. Maybe an about:sandbox could display the policy.)

> I take it we can't use our normal logging mechanisms here.

I think we'd have to pull in some more Mozilla headers and the comment at the top of the file talks about this code being part of a static library which shouldn't have Mozilla dependencies.

https://dxr.mozilla.org/mozilla-central/rev/d6d4e8417d2fd71fdf47c319b7a217f6ace9d5a5/security/sandbox/mac/Sandbox.mm#6

And code in this file is already using fprintf to stderr for errors that prevent us from enabling the policy so I thought stderr made sense for this.
Attachment #8756104 - Attachment is obsolete: true
Attached patch Fix indentation / white space (obsolete) — Splinter Review
Attachment #8756108 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9fd1d6aeed21
https://hg.mozilla.org/mozilla-central/rev/d3bde9a513bb
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8756731 [details] [diff] [review]
Removes code only used on OS X 10.8 and earlier

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

::: security/sandbox/mac/Sandbox.mm
@@ -376,5 @@
>    "        (literal \"/private/var/run/mDNSResponder\"))\n"
>    "\n"
>    "; print preview\n"
> -  "    (if (> macosMinorVersion 9)\n"
> -  "        (allow lsopen))\n"

Problem on Nightly for 10.9.
Just noticed this says > 10.9, so this could be the culprit.
We probably need to back this out and put the OSXVersionMinor() function back in.
This is causing lots of crashiness on 10.9 (bug 1276295). I think we should back this out.
Backed out.
https://hg.mozilla.org/mozilla-central/rev/a41a34f7d936
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla49 → ---
Thanks, Bob. Yeah, that is very likely to be the problem. The bug should not have removed the rules that only allowed lsopen on 10.10 and newer OS versions:

  -  "    (if (> macosMinorVersion 9)\n"
  -  "        (allow lsopen))\n"
  +  "    (allow lsopen)\n"

I misread that line as >= instead of >. So we need to keep the OS X version checks in the code and we need to continue to pass the OS minor version to the ruleset lisp.

New patches on the way. Haven't yet verified the fix on 10.9, but will do that before pushing.
Added back the version checking and the rule needed for 10.9.
Attachment #8756731 - Attachment is obsolete: true
Attachment #8758846 - Flags: review?(bobowen.code)
Attachment #8756732 - Attachment is obsolete: true
Attachment #8758847 - Flags: review?(bobowen.code)
Attachment #8758846 - Flags: review?(bobowen.code) → review+
Attachment #8758847 - Flags: review?(bobowen.code) → review+
Fixed patch commit message.
Attachment #8758846 - Attachment is obsolete: true
Verified the latest patch on 10.9 and confirmed the 10.9 issue was due to the missing version check on the lsopen rule.
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ffe615e05a8
Remove OS X 10.6-10.8-Specific Sandboxing Code; r=bobowen
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f1b49de2869
Remove OS X 10.6-10.8-Specific Sandboxing Code (fix indentation); r=bobowen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4ffe615e05a8
https://hg.mozilla.org/mozilla-central/rev/4f1b49de2869
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.