Closed Bug 1110911 Opened 5 years ago Closed 5 years ago

Move Mac sandboxing code into plugin-container

Categories

(Core :: Security: Process Sandboxing, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: smichaud, Assigned: smichaud)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

This has already been done on Windows (bug 1088488) and Linux (bug 1101170).  We also need it on the Mac.
Move process sandboxing bugs to their new, separate component.

(Sorry for the bugspam; filter on 3c21328c-8cfb-4819-9d88-f6e965067350.)
Component: Security → Security: Process Sandboxing
Attached patch Fix (obsolete) — Splinter Review
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
Attachment #8573506 - Flags: review?(cpearce)
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;
}
Attachment #8573506 - Flags: review?(cpearce) → review+
Attached patch Fix rev1 (obsolete) — Splinter Review
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".
Attachment #8573506 - Attachment is obsolete: true
Attachment #8574833 - Flags: review?(areinald)
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.
OK then.  I'll open another bug to fix nsCocoaFeatures.
(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.
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
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).
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).
> 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 on attachment 8574833 [details] [diff] [review]
Fix rev1

New patch coming up.
Attachment #8574833 - Flags: review?(areinald)
Attachment #8574833 - Attachment is obsolete: true
>> 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.
Attached patch Fix rev2 (obsolete) — Splinter Review
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.
Attachment #8576274 - Flags: review?(areinald)
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.
Attachment #8576274 - Flags: review?(jld)
Attachment #8576274 - Flags: review?(areinald)
Attachment #8576274 - Flags: review+
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?
Attachment #8576274 - Flags: review?(jld) → review+
> 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.
(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.
Attached patch Fix rev4Splinter Review
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.
Attachment #8576274 - Attachment is obsolete: true
Attachment #8586318 - Flags: review+
Is there a recent Try link handy for this?
Keywords: checkin-needed
> Is there a recent Try link handy for this?

No.  I'll start one and post it here.
How long should I wait before specifying checkin-needed again? :-)
See comment #24 for a recent tryserver run.
Keywords: checkin-needed
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!
Flags: needinfo?(smichaud)
Keywords: checkin-needed
Hi Tomcat.

Sigh.  That was a pretty quick case of bitrot.  I'll try again later.
Flags: needinfo?(smichaud)
https://hg.mozilla.org/mozilla-central/rev/004038107742
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1167494
You need to log in before you can comment on or make changes to this bug.