1272764 - Remove OS X 10.6-10.8-Specific Sandboxing Code

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Haik Aftandilian [:haik]]

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

Comment 1

[Haik Aftandilian [:haik]]

Attachment: Removes code only used on OS X 10.8 and earlier

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.

Comment 2

[Haik Aftandilian [:haik]]

Attachment: Fix indentation / white space

Comment 3

[Haik Aftandilian [:haik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b0df3553cc7

Comment 4

[Bob Owen (:bobowen)]

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.

Comment 5

[Bob Owen (:bobowen)]

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.

Comment 6

[Haik Aftandilian [:haik]]

(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.

Comment 7

[Haik Aftandilian [:haik]]

Attachment: Diff output comparing the sandbox policies before/after these changes are applied.

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.

Comment 8

[Haik Aftandilian [:haik]]

Attachment: Removes code only used on OS X 10.8 and earlier

Addresses bobowen's comments and also removes the fprintf's to dump the policy I had accidentally left in.

Comment 9

[Haik Aftandilian [:haik]]

Attachment: Fix indentation / white space

Made minor white space change due to changes in patch 1.

Comment 10

[Haik Aftandilian [:haik]]

Attachment: Fix indentation / white space

Made minor white space change due to changes in patch 1. (Re-attaching because I failed to obsolete earlier version. Sorry for the spam.)

Comment 11

[Bob Owen (:bobowen)]

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.

Comment 12

[Haik Aftandilian [:haik]]

(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.

Comment 13

[Haik Aftandilian [:haik]]

Attachment: Removes code only used on OS X 10.8 and earlier

Comment 14

[Haik Aftandilian [:haik]]

Attachment: Fix indentation / white space

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fd1d6aeed21
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3bde9a513bb

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9fd1d6aeed21
https://hg.mozilla.org/mozilla-central/rev/d3bde9a513bb

Comment 17

[Bob Owen (:bobowen)]

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.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

This is causing lots of crashiness on 10.9 (bug 1276295). I think we should back this out.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out.
https://hg.mozilla.org/mozilla-central/rev/a41a34f7d936

Comment 20

[Haik Aftandilian [:haik]]

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.

Comment 21

[Haik Aftandilian [:haik]]

Attachment: Diff output comparing the sandbox policies before/after these changes are applied.

Comment 22

[Haik Aftandilian [:haik]]

Attachment: Removes code only used on OS X 10.8 and earlier (fix 10.9 issue)

Added back the version checking and the rule needed for 10.9.

Comment 23

[Haik Aftandilian [:haik]]

Attachment: Fix indentation / white space (fix 10.9 issue)

Comment 24

[Haik Aftandilian [:haik]]

Attachment: Removes code only used on OS X 10.8 and earlier (fix 10.9 issue)

Fixed patch commit message.

Comment 25

[Haik Aftandilian [:haik]]

Verified the latest patch on 10.9 and confirmed the 10.9 issue was due to the missing version check on the lsopen rule.

Comment 26

[Haik Aftandilian [:haik]]

Attachment: Patch 1: Removes code only used on OS X 10.8 and earlier (fix 10.9 issue)

Comment 27

[Haik Aftandilian [:haik]]

Attachment: Patch 2: Fix indentation / white space (fix 10.9 issue)

Comment 28

[Pulsebot]

Pushed by [e-mail]:
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

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ffe615e05a8
https://hg.mozilla.org/mozilla-central/rev/4f1b49de2869