Sandbox the content process on Mac

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: areinald.bug, Assigned: areinald.bug)

Tracking

unspecified
mozilla36
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

No description provided.
Blocks: 730956
Posted patch bugzilla-1076385-1.patch (obsolete) — Splinter Review
WIP: Generalized GMP process sandbox code so it could be reused to sandbox the content process. Compiles, but not yet tested.
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.
Posted patch bugzilla-1076385-2.patch (obsolete) — Splinter Review
"Plumbing" part of the content sandboxing for Mac.
Attachment #8498538 - Attachment is obsolete: true
Attachment #8505619 - Flags: review?(smichaud)
Status: NEW → ASSIGNED
Depends on: 1083344
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.)
(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 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 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.
(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 ?
Additionally this file (mozilla-central/security/sandbox/mac/Sandbox.mm) should not be compiled in B2G as far as I understand.
> 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.
> 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.
> 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.
>> 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.
(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.
Posted patch bugzilla-1076385-3.patch (obsolete) — Splinter Review
Tried to take into account comments to previous patch.
Attachment #8505619 - Attachment is obsolete: true
Attachment #8505619 - Flags: review?(smichaud)
Attachment #8509460 - Flags: review?(smichaud)
Posted patch Patch rev4Splinter Review
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.
Attachment #8509460 - Attachment is obsolete: true
Attachment #8509460 - Flags: review?(smichaud)
Attachment #8509663 - Flags: review?(areinald)
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 !
Attachment #8509663 - Flags: review?(areinald) → review+
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 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
https://hg.mozilla.org/mozilla-central/rev/da167ffa2060
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1094196
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.
Attachment #8520012 - Flags: review?(smichaud)
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.
Attachment #8520012 - Flags: review?(smichaud) → review+
Keywords: checkin-needed
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).
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
(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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f22af8dfea68
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 1123291
You need to log in before you can comment on or make changes to this bug.