1110911 - Move Mac sandboxing code into plugin-container

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

This has already been done on Windows (bug 1088488) and Linux (bug 1101170).  We also need it on the Mac.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Move process sandboxing bugs to their new, separate component.

(Sorry for the bugspam; filter on 3c21328c-8cfb-4819-9d88-f6e965067350.)

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Here's a patch for this bug.

I removed all XUL dependencies from the Mac sandbox and turned it into a "Library" -- actually just a bare object file.  This way it can be (and is) linked both into plugin-container (where it's used to start the GMP plugin sandbox) and into XUL (where it's used to start the content process sandbox).  I can't think of any reasons not to do this -- but let me know if there are any objections.

I made some changes to the GMPLoader and SandboxStarter objects to accommodate how the Mac sandbox works (to set the rules appropriately we need information that's only available just before the sandbox is started).  I also no longer crash (with MOZ_CRASH) when the sandbox fails to get initialized.  The reason is that it's now possible for the failing sandbox initialization code to return an error value.  (I still crash when the content sandbox fails to initialize, because there we don't seem to be able to indicate failure using a return value.)

Later I'll ask André Reinald to review my changes specifically to the Mac sandbox code.  But first I want to make sure I'm on the right track.

I've started a set of tryserver builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=536121e80a4b

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8573506 [details] [diff] [review]
Fix

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

Overall pretty good. I don't really feel qualified to review the MacOSX specific stuff in Sandbox.mm, maybe you should find someone else who knows MacOSX to look over that?

::: dom/media/gmp/GMPLoader.cpp
@@ +219,5 @@
>    // Start the sandbox now that we've generated the device bound node id.
>    // This must happen after the node id is bound to the device id, as
>    // generating the device id requires privileges.
>    if (mSandboxStarter) {
> +    if (!mSandboxStarter->Start(aLibPath)) {

Could compress this to:

if (mSandboxStarter && !mSandboxStarter->Start(aLibPath)) {
  return false;
}

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1

This patch follows Chris's suggestion and adds a comment to the top of Sandbox.mm (explaining that it must not have external dependencies, and why).

André, please feel free to look at any of it.  But I particularly want you to look at my changes to Sandbox.mm and Sandbox.h.

   "(define sandbox-level %d)\n"
   "(define macosMajorVersion %d)\n"
-  "(define macosMinorVersion %d)\n"
   "(define appPath \"%s\")\n"
   "(define appBinaryPath \"%s\")\n"

...

   "(if \n"
   "  (or\n"
-  "    (< macosMinorVersion 9)\n"
+  "    (< macosMajorVersion 9)\n"
   "    (= sandbox-level 0))\n"
   "  (allow default)\n"

Strictly speaking I didn't need to make this change (and related changes).  But nobody really considers the first part of all the OS X version numbers (for example the "10" of "10.9.5") to really be a component of the version number.  Neither does it make sense to call the "9" of "10.9.5" the "minor version" -- it's the "major version number", and "5" is the "minor version number".

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8574833 [details] [diff] [review]
Fix rev1

I've started another set of tryserver builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=71ce44f0c4e5

Comment 6

[André Reinald]

Comment on attachment 8574833 [details] [diff] [review]
Fix rev1

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

I believe we must keep our naming consistent with nsCocoaFeatures in order to avoid future confusion.
nsCocoaFeatures has versionMajor, versionMinor, and versionBugFix.
That's why I decided to choose those names.

But all this is only about the small part of your patch which changes my own code.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

OK then.  I'll open another bug to fix nsCocoaFeatures.

Comment 8

[André Reinald]

(In reply to Steven Michaud from comment #7)
> OK then.  I'll open another bug to fix nsCocoaFeatures.

Not sure it's a good idea, since nsCocoaFeatures naming is in turn based Apple's naming.
So changing it would move the inconsistency to another level.

Comment 9

[André Reinald]

I personally feel comfortable with 10.9.5 being:
10 -> major
9  -> minor
5  -> bugFix

But that's maybe because I know Mac OS since version 1.0

Comment 10

[André Reinald]

I thought you didn't like drive-by changes, but if you want to make some in this part of the code, I suggest you pass the system version in the "MacSandboxType_Plugin" part sandbox rules, and test it the rules themselves, the way I did in bug 1083344, that would factor out the code and avoid dirty tricks with lisp comments.

Plus I volunteer to make this patch myself (and post it in this bug).

Comment 11

[Steven Michaud [:smichaud] (Retired)]

I think the major/minor/bugfix terminology is horribly misleading for OS X.

I agree it's old (it's used by Gestalt).  And I grant it did make sense back when dinosaurs roamed the earth and OS X hadn't yet evolved :-)

But before I go changing nsCocoaFeatures, let me see if I can find some precedent for my terminology (or something similar).

Comment 12

[Steven Michaud [:smichaud] (Retired)]

> I thought you didn't like drive-by changes

I don't.  But I think the major/minor/bugfix terminology makes me feel so ill that I'm sorely tempted to make an exception.

> I suggest you pass the system version in the "MacSandboxType_Plugin" part sandbox rules

That's actually not a bad idea.  We could drop the entire OSXVersion class from Sandbox.mm.

I'll revise my patch to do that.  I'll just pass what I call the major version and Gestalt calls the minor version (the "9" of "10.9.5").

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8574833 [details] [diff] [review]
Fix rev1

New patch coming up.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

>> I suggest you pass the system version in the
>>  "MacSandboxType_Plugin" part sandbox rules
>
> That's actually not a bad idea.  We could drop the entire OSXVersion
> class from Sandbox.mm.

Oops, this won't work.  We could do it from XUL, but not from
plugin-container.  So we're going to need to keep the OSXVersion class
in Sandbox.mm.

Let me think about the version naming terminology.  Apple's
NSOperatingSystemVersion structure (new with OS X 10.10) uses
major/minor/patch -- which I think is just as bad as as
major/minor/bugfix.

https://developer.apple.com/library/ios/documentation/Cocoa/Reference/Foundation/Classes/NSProcessI\
nfo_Class/index.html#//apple_ref/c/tdef/NSOperatingSystemVersion

Apple calls each new named version a "major release".  For
example here Apple calls Mountain Lion (OS X 10.8) the "ninth major
release of [OS X]":

http://www.apple.com/pr/library/2012/02/16Apple-Releases-OS-X-Mountain-Lion-Developer-Preview-with-\
Over-100-New-Features.html

Apple also uses the terms "minor release" and "minor update", but
perhaps somewhat less systemmatically, for example here:

http://store.apple.com/id/question/answers/readonly/if-i-purchase-ox-106-now-do-i-have-to-pay-again\
-to-upgrade-to-the-most-current-version/Q4K4D9JKK4TY447TU

Major/minor/bugfix and major/minor/patch are objectively confusing,
and even "wrong" -- otherwise why does Apple talk about "major
releases" and "minor releases"?  But they do have precedent, and
changing them opens a can of ugly worms.  By tomorrow I'll decide
which I think is uglier -- the major/minor/bugfix terminology
or the worms.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

>> I suggest you pass the system version in the
>>  "MacSandboxType_Plugin" part sandbox rules
>
> That's actually not a bad idea.  We could drop the entire OSXVersion
> class from Sandbox.mm.

Oops, this won't work.  We could do it from XUL, but not from
plugin-container.  So we're going to need to keep the OSXVersion class
in Sandbox.mm.

Let me think about the version naming terminology.  Apple's
NSOperatingSystemVersion structure (new with OS X 10.10) uses
major/minor/patch -- which I think is just as bad as as
major/minor/bugfix.

https://developer.apple.com/library/ios/documentation/Cocoa/Reference/Foundation/Classes/NSProcessInfo_Class/index.html#//apple_ref/c/tdef/NSOperatingSystemVersion

Apple calls each new named version a "major release".  For example
here Apple calls Mountain Lion (OS X 10.8) the "ninth major release of
[OS X]":

http://www.apple.com/pr/library/2012/02/16Apple-Releases-OS-X-Mountain-Lion-Developer-Preview-with-Over-100-New-Features.html

Apple also uses the terms "minor release" and "minor update", but
perhaps somewhat less systemmatically, for example here:

http://store.apple.com/id/question/answers/readonly/if-i-purchase-ox-106-now-do-i-have-to-pay-again-to-upgrade-to-the-most-current-version/Q4K4D9JKK4TY447TU

Major/minor/bugfix and major/minor/patch are objectively confusing,
and even "wrong" -- otherwise why does Apple talk about "major
releases" and "minor releases"?  But they do have precedent, and
changing them opens a can of ugly worms.  By tomorrow I'll decide
which I think is uglier -- the major/minor/bugfix terminology or the
worms.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev2

Here's my next revision.

I decided we should live with the major/minor/bugfix nonsense, since Apple has already blessed it.  But I added a comment explaining how misleading it is.

I intend shortly to open another bug to add the comment to nsCocoaFeatures.h.

Comment 17

[André Reinald]

Comment on attachment 8576274 [details] [diff] [review]
Fix rev2

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

Changes brought to the parts I know are fine.
But as this patch touches other parts as well, which seem to target parity with Linux and Windows, I'll ask jld for review as well.

Comment 18

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8576274 [details] [diff] [review]
Fix rev2

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

A couple of minor nits, but looks good otherwise.

::: dom/media/gmp/GMPLoader.h
@@ +10,5 @@
>  #include <stdint.h>
>  #include "gmp-entrypoints.h"
>  
> +#if defined(XP_MACOSX)
> +#include "mozilla/Sandbox.h"

Nit: Would it make sense to just forward-declare MacSandboxInfo instead of including the header?

::: ipc/contentproc/plugin-container.cpp
@@ +123,5 @@
> +    virtual bool Start(const char *aLibPath) MOZ_OVERRIDE {
> +      std::string err;
> +      bool rv = mozilla::StartMacSandbox(mInfo, err);
> +      if (!rv) {
> +        printf("sandbox_init() failed! Error \"%s\"\n", err.c_str());

Nit: Does this need to write to stdout rather than stderr?

Comment 19

[Steven Michaud [:smichaud] (Retired)]

> Nit: Would it make sense to just forward-declare MacSandboxInfo
> instead of including the header?

Will do.

> Nit: Does this need to write to stdout rather than stderr?

Actually printf does already write to stdout.  I checked by looking at
the machine code for printf in /usr/lib/system/libsystem_c.dylib.

By the way, bug 1135424 seems to have broken the tree.  Or at least I
can't do a self build on current trunk, and that seems to be the
reason why.  I won't be able to land this patch until that problem is
resolved.

See bug 1135424 comment #20.

Comment 20

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Steven Michaud from comment #19)
> > Nit: Does this need to write to stdout rather than stderr?
> 
> Actually printf does already write to stdout.

Sorry; I could have phrased that more clearly.  I was wondering if there's some reason this code is using printf and stdout, or if it could call fprintf(stderr, ...) instead, because stderr seems to me to be the more reasonable place for this error message to be written.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev4

Carrying forward r+.

>> Nit: Would it make sense to just forward-declare MacSandboxInfo
>> instead of including the header?
>
> Will do.

This didn't work -- apparently you can't forward-declare an enum.

>>> Nit: Does this need to write to stdout rather than stderr?
>>
>> Actually printf does already write to stdout.
>
> Sorry; I could have phrased that more clearly.  I was wondering if
> there's some reason this code is using printf and stdout, or if it
> could call fprintf(stderr, ...) instead, because stderr seems to me
> to be the more reasonable place for this error message to be
> written.

I ended up changing printf to fprintf(stderr, ...) everywhere.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Is there a recent Try link handy for this?

Comment 23

[Steven Michaud [:smichaud] (Retired)]

> Is there a recent Try link handy for this?

No.  I'll start one and post it here.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

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

Comment 25

[Steven Michaud [:smichaud] (Retired)]

How long should I wait before specifying checkin-needed again? :-)

Comment 26

[Steven Michaud [:smichaud] (Retired)]

See comment #24 for a recent tryserver run.

Comment 27

[Carsten Book [:Tomcat]]

hi Steven, this failed to apply:

renamed 1110911 -> bugzilla1110911-patch-rev4.txt
applying bugzilla1110911-patch-rev4.txt
patching file ipc/contentproc/plugin-container.cpp
Hunk #1 FAILED at 82
1 out of 1 hunks FAILED -- saving rejects to file ipc/contentproc/plugin-container.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bugzilla1110911-patch-rev4.txt

could you take a look, thanks!

Comment 28

[Steven Michaud [:smichaud] (Retired)]

Hi Tomcat.

Sigh.  That was a pretty quick case of bitrot.  I'll try again later.

Comment 29

[Steven Michaud [:smichaud] (Retired)]

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/004038107742

Comment 30

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/004038107742