1076385 - Sandbox the content process on Mac

Status: RESOLVED FIXED

(Core :: Security, defect)

Comment 1

[André Reinald]

Attachment: bugzilla-1076385-1.patch

WIP: Generalized GMP process sandbox code so it could be reused to sandbox the content process. Compiles, but not yet tested.

Comment 2

[André Reinald]

After a discussion with Chris Peterson, because content sandboxing will not be an option when e10s is turned on, I will submit a first patch for the "plumbing" part of the bug, then another patch for "tightened" rules, likely in a follow-up bug.

Tightening rules for the content process is a relatively risky task as the resources may vary with the content, so I expect much testing will be needed.

Comment 3

[André Reinald]

Attachment: bugzilla-1076385-2.patch

"Plumbing" part of the content sandboxing for Mac.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8505619 [details] [diff] [review]
bugzilla-1076385-2.patch

This looks fine to me ... but have you tested it yet? :-)

(In comment #1 you say you still haven't.)

Comment 5

[André Reinald]

(In reply to Steven Michaud from comment #4)
> Comment on attachment 8505619 [details] [diff] [review]
> bugzilla-1076385-2.patch
> 
> This looks fine to me ... but have you tested it yet? :-)
> 
> (In comment #1 you say you still haven't.)

I tested it locally with a few pages, and everything behaves normally (I don't expect my current ruleset to break anything). But I launched no tryserver builds yet. If you think I should, I will do that tomorrow morning. Unless you unless you do it for me.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8505619 [details] [diff] [review]
bugzilla-1076385-2.patch

I've started a set of tryserver builds:
https://tbpl.mozilla.org/?tree=Try&rev=690122058319

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8505619 [details] [diff] [review]
bugzilla-1076385-2.patch

This basically looks fine to me, but I have some nits:

-    Open(aChannel, aParentHandle, aIOLoop);
+    if (!Open(aChannel, aParentHandle, aIOLoop)) {
+      return false;
+    }

Why did you do this?  It seems reasonable on the face of it.  But making changes in cross-platform code with which you're not very familiar is risky.  And if you do keep it you should change it as follows:

    rv = Open(aChannel, aParentHandle, aIOLoop);
    if (NS_WARN_IF(NS_FAILED(rv))) {
        return false;
    }

My tryserver run did very well.  But there were some persistent failures in B2G builds, which could conceivably be related to this change (as B2G uses the content process).

-  if (nsCocoaFeatures::OnLionOrLater()) {
-    profile.AppendPrintf(rules, ";",
+  if (aInfo.type == MacSandboxType_Plugin) {
+    const char *commentLine = nsCocoaFeatures::OnLionOrLater() ? ";" : "";
+    profile.AppendPrintf(pluginSandboxRules,
+                         commentLine,
                          aInfo.pluginInfo.pluginPath.get(),
                          aInfo.pluginInfo.pluginBinaryPath.get(),
                          aInfo.appPath.get(),
                          aInfo.appBinaryPath.get());
-  } else {
-    profile.AppendPrintf(rules, "",
-                         aInfo.pluginInfo.pluginPath.get(),
-                         aInfo.pluginInfo.pluginBinaryPath.get(),
-                         aInfo.appPath.get(),
-                         aInfo.appBinaryPath.get());

Please drop this change.  I'd need to back it out in any case for my patch for bug 1083284.

Comment 8

[André Reinald]

(In reply to Steven Michaud from comment #7)
> Comment on attachment 8505619 [details] [diff] [review]
> bugzilla-1076385-2.patch
> 
> This basically looks fine to me, but I have some nits:
> 
> -    Open(aChannel, aParentHandle, aIOLoop);
> +    if (!Open(aChannel, aParentHandle, aIOLoop)) {
> +      return false;
> +    }
> 
> Why did you do this?  It seems reasonable on the face of it.  But making
> changes in cross-platform code with which you're not very familiar is risky.
> And if you do keep it you should change it as follows:
> 
>     rv = Open(aChannel, aParentHandle, aIOLoop);
>     if (NS_WARN_IF(NS_FAILED(rv))) {
>         return false;
>     }

I basically copied what you did for openh264 sandboxing.
But I agree a warning in the console would be nice.
Should I do the same for openh264 while I'm at it ?

> My tryserver run did very well.  But there were some persistent failures in
> B2G builds, which could conceivably be related to this change (as B2G uses
> the content process).
> 
> -  if (nsCocoaFeatures::OnLionOrLater()) {
> -    profile.AppendPrintf(rules, ";",
> +  if (aInfo.type == MacSandboxType_Plugin) {
> +    const char *commentLine = nsCocoaFeatures::OnLionOrLater() ? ";" : "";
> +    profile.AppendPrintf(pluginSandboxRules,
> +                         commentLine,
>                           aInfo.pluginInfo.pluginPath.get(),
>                           aInfo.pluginInfo.pluginBinaryPath.get(),
>                           aInfo.appPath.get(),
>                           aInfo.appBinaryPath.get());
> -  } else {
> -    profile.AppendPrintf(rules, "",
> -                         aInfo.pluginInfo.pluginPath.get(),
> -                         aInfo.pluginInfo.pluginBinaryPath.get(),
> -                         aInfo.appPath.get(),
> -                         aInfo.appBinaryPath.get());
> 
> Please drop this change.  I'd need to back it out in any case for my patch
> for bug 1083284.

All this happens in function StartMacSandbox, which is only called when we run on Mac.
Why do you think it could be related to the B2G failures ?

Comment 9

[André Reinald]

Additionally this file (mozilla-central/security/sandbox/mac/Sandbox.mm) should not be compiled in B2G as far as I understand.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

> Why do you think it could be related to the B2G failures?

I don't.  It's just that I'll have to back out this change when I fix bug 1083234.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

> Additionally this file
> (mozilla-central/security/sandbox/mac/Sandbox.mm) should not be
> compiled in B2G as far as I understand.

There are some B2G emulators that use Mac code (that run on OS X).

In any case I'm happy to land your patch (with my suggested changes)
on the trunk.  But in the unlikely event that it triggers persistent
failures, we'll have to back it out again.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

> I basically copied what you did for openh264 sandboxing.
> But I agree a warning in the console would be nice.

I didn't even realize.  I was just concerned that you'd made a driveby change in unfamiliar code, and didn't conform to the coding style just a few lines above.

> Should I do the same for openh264 while I'm at it ?

I'd say don't bother.  I don't like driveby changes unless there's a good reason for them.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

>> Why do you think it could be related to the B2G failures?
>
> I don't.  It's just that I'll have to back out this change when I fix bug 1083234.

Bug 1083284.

Comment 14

[André Reinald]

(In reply to Steven Michaud from comment #10)
> > Why do you think it could be related to the B2G failures?
> 
> I don't.  It's just that I'll have to back out this change when I fix bug
> 1083284.

Does it make sense that I wait for the fix to bug 1083284 then rebase my patch and make sure I break nothing? I would feel more comfortable with this option.

Comment 15

[André Reinald]

Attachment: bugzilla-1076385-3.patch

Tried to take into account comments to previous patch.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Attachment: Patch rev4

This is what I was looking for.

Also I discovered I gave you wrong advice (in comment #7) about the changes you made to the Open() call in ContentChild.cpp.  I was looking at the declaration of the wrong call to Open() :-(

This patch also fixes that problem.  Sorry for the confusion.

Comment 17

[André Reinald]

Yes, Open returns a bool, and rv can't be assigned a bool. I made the change locally but forgot to refresh/export, so patch-3 would not compile.

Aside from that, I still dislike the code duplication in Sandbox.mm, but you're right: patching it would be a driveby change, which we should avoid.

if (...)
  profile.AppendPrintf(pluginSandboxRules, ";", ...
else
  profile.AppendPrintf(pluginSandboxRules, "", ...

So let's go with this last version !

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Sorry, André, to keep dragging this bug out.  But what I recently learned at bug 1055395 and bug 1087901 raises another question.

I learned there that the (successful) receipt of *any* IPC message is as good a guarantee that IPC setup has finished as the receipt of the "hello" message that results in a call to Listener::OnChannelConnected() -- see particularly bug 1087901 comment 1.

ContentChild::RecvSetProcessSandbox() is where the content process sandbox gets started on Windows and Linux:

https://hg.mozilla.org/mozilla-central/annotate/2c94c22f3a29/dom/ipc/ContentChild.cpp#l1043

And this call is made on receipt of an IPC message sent by SendSetProcessSandbox() in ContentParent::InitInternal():

https://hg.mozilla.org/mozilla-central/annotate/2c94c22f3a29/dom/ipc/ContentParent.cpp#l2145

Why don't you start the Mac content sandbox where the Windows and Linux content sandboxes get started?  Is it just that you wanted to emulate how I used to start the Mac GMP sandbox (before the patch for bug 1087901 landed)?

Comment 19

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8509663 [details] [diff] [review]
Patch rev4

I decided to go ahead and land this anyway, before I hear an answer to my questions in comment #18.

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

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[André Reinald]

Attachment: Parity with Win and Linux implementation

Addressing comment 18 from Steven: starting Mac sandbox the same way as Win and Linux (on reception of a specific message from the main process).

Steven, I made this patch so it applies over the previous one (Patch rev4), as it has already landed, and I'm not sure of the preferred way to handle this case.

Comment 22

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8520012 [details] [diff] [review]
Parity with Win and Linux implementation

Looks good to me.

> I made this patch so it applies over the previous one (Patch rev4),
> as it has already landed

Yes, that's right.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Has this gone through Try? Also, please reopen in the bug if the work isn't done yet (or set the leave-open keyword to avoid it being auto-resolved in the first place).

Comment 24

[André Reinald]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #23)
> Has this gone through Try? Also, please reopen in the bug if the work isn't
> done yet (or set the leave-open keyword to avoid it being auto-resolved in
> the first place).

Thanks for the tips. Gone through try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=17bd6e8d0ef4

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f22af8dfea68

Please make sure future patches include a commit message per the guidelines below when requesting checkin.
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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