New issue
Advanced search Search tips

Issue 850735 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug

Blocking:
issue 749839



Sign in to add a comment

content_shell does not properly support the v2 sandbox

Project Member Reported by rsesek@chromium.org, Jun 7 2018

Issue description

content_shell (Content Shell.app on Mac) does not properly use the v2 sandbox. In Chrome, the SeatbeltExecServer is executed as part of chrome_exe_main_mac.cc, which is obviously in //chrome, to sandbox the executable before loading the Chromium Framework and all its dependent libraries.

But because there is no equivalent code in //content/shell/app/shell_main.cc to run the SeatbeltExecServer, the V2 sandbox isn't used. Instead, the renderer goes through the v1 sandboxing path and sandboxes itself after the warmup phase.

We need content_shell to support the v2 sandbox before removing v1, but it also confused me when trying to debug the sandbox with just content_shell.
 
Of course to make content_shell equivalent to chrome, the content_shell_helper_app will need to stop directly linking to the content_shell_framework target (causing it to be loaded via dyld rather than dlopen). That's basically this TODO: https://cs.chromium.org/chromium/src/content/shell/BUILD.gn?l=753&rcl=fee8b56aeb463d58fdbeaed698d5f1a1dc14351b
Cc: kerrnel@chromium.org
Owner: rsesek@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d4117546b483fe82c8a659348a1f7b4960b692db

commit d4117546b483fe82c8a659348a1f7b4960b692db
Author: Robert Sesek <rsesek@chromium.org>
Date: Wed Jun 13 00:31:00 2018

[Mac] Reconfigure Content Shell executables to mirror Chrome.

Rather than directly linking Content Shell and Content Shell Helper
agasint the Content Shell Framework, to be loaded by dyld, this change
has the executables dlopen the framework at runtime.

This is done to match how Chrome is loaded, which is a prequisite for
using the V2 sandbox in Content Shell.

No-Presubmit: True
Bug:  850735 
Test: Build content_shell and //content test suites. Both launch and pass.
Change-Id: Id843d99877166fbf91d0df55d9976f6019149da8
Reviewed-on: https://chromium-review.googlesource.com/1093569
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566651}
[modify] https://crrev.com/d4117546b483fe82c8a659348a1f7b4960b692db/chrome/app/chrome_exe_main_mac.cc
[modify] https://crrev.com/d4117546b483fe82c8a659348a1f7b4960b692db/content/shell/BUILD.gn
[modify] https://crrev.com/d4117546b483fe82c8a659348a1f7b4960b692db/content/shell/app/shell_main.cc
[add] https://crrev.com/d4117546b483fe82c8a659348a1f7b4960b692db/content/shell/app/shell_main_mac.cc
[modify] https://crrev.com/d4117546b483fe82c8a659348a1f7b4960b692db/content/shell/tools/breakpad_integration_test.py
[modify] https://crrev.com/d4117546b483fe82c8a659348a1f7b4960b692db/testing/scripts/content_shell_crash_test.py

Comment 4 Deleted

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37128e1b486709b4d27cfd5049ad8db404b8b3d0

commit 37128e1b486709b4d27cfd5049ad8db404b8b3d0
Author: Robert Sesek <rsesek@chromium.org>
Date: Wed Jun 13 19:09:12 2018

[Mac] Refactor sandbox initialization code in chrome_exe_main_mac.cc

This pulls the code out of the executable and moves it into the
//sandbox/mac:seatbelt target. This way it can be reused by
content_shell.

In addition, this also reduces the number of arguments used by the V2
sandbox. A prior design used a double-exec, requiring additional
switches to know the stage at which the sandboxed process was running.
However the double-exec is no longer done for performance reasons, so
some switches can be eliminated.

No-Presubmit: True
Bug:  850735 , 749839
Test: No functional change. Sandboxed processes are still sandboxed.
Change-Id: I4eedf1e7e057d0650c805430fa53763b3909f923
Reviewed-on: https://chromium-review.googlesource.com/1097842
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566939}
[modify] https://crrev.com/37128e1b486709b4d27cfd5049ad8db404b8b3d0/chrome/app/chrome_exe_main_mac.cc
[modify] https://crrev.com/37128e1b486709b4d27cfd5049ad8db404b8b3d0/content/app/content_main_runner_impl.cc
[modify] https://crrev.com/37128e1b486709b4d27cfd5049ad8db404b8b3d0/content/browser/child_process_launcher_helper_mac.cc
[modify] https://crrev.com/37128e1b486709b4d27cfd5049ad8db404b8b3d0/content/common/sandbox_init_mac.cc
[modify] https://crrev.com/37128e1b486709b4d27cfd5049ad8db404b8b3d0/content/ppapi_plugin/ppapi_thread.cc
[modify] https://crrev.com/37128e1b486709b4d27cfd5049ad8db404b8b3d0/content/public/common/content_switches.cc
[modify] https://crrev.com/37128e1b486709b4d27cfd5049ad8db404b8b3d0/content/public/common/content_switches.h
[modify] https://crrev.com/37128e1b486709b4d27cfd5049ad8db404b8b3d0/sandbox/mac/seatbelt_exec.cc
[modify] https://crrev.com/37128e1b486709b4d27cfd5049ad8db404b8b3d0/sandbox/mac/seatbelt_exec.h

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/746bc0576ccbe17920691cf92fbcfa55c674d079

commit 746bc0576ccbe17920691cf92fbcfa55c674d079
Author: Robert Sesek <rsesek@chromium.org>
Date: Wed Jun 13 21:04:55 2018

[Mac] Fix isolated content_shell_crash_test.

The script pointed to the un-bundled Content Shell Framework, which is
not in the isolated inputs, rather than the Framework bundled in
Content Shell.app.

Bug:  850735 
Change-Id: I873a94a0e1c6882704199d918d9f463d4a82a39b
Reviewed-on: https://chromium-review.googlesource.com/1099276
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566982}
[modify] https://crrev.com/746bc0576ccbe17920691cf92fbcfa55c674d079/testing/scripts/content_shell_crash_test.py

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/878c88fd30730c968789988c747f3045d12d837b

commit 878c88fd30730c968789988c747f3045d12d837b
Author: Robert Sesek <rsesek@chromium.org>
Date: Fri Jun 15 00:23:29 2018

[Mac] Enable the MacV2Sandbox in content_shell.

The main executable now invokes the SeatbeltExecServer when the feature
is enabled.

This also sets up the OuterBundle and BaseBundleID overrides, which are
required to properly find the rohitfork port.

Bug:  850735 , 749839
Change-Id: Ic02df574a209213d1ca560ab2d1e947be5ae5136
Reviewed-on: https://chromium-review.googlesource.com/1099643
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567496}
[modify] https://crrev.com/878c88fd30730c968789988c747f3045d12d837b/content/shell/BUILD.gn
[modify] https://crrev.com/878c88fd30730c968789988c747f3045d12d837b/content/shell/app/paths_mac.h
[modify] https://crrev.com/878c88fd30730c968789988c747f3045d12d837b/content/shell/app/paths_mac.mm
[modify] https://crrev.com/878c88fd30730c968789988c747f3045d12d837b/content/shell/app/shell_main_delegate.cc
[modify] https://crrev.com/878c88fd30730c968789988c747f3045d12d837b/content/shell/app/shell_main_delegate_mac.h
[modify] https://crrev.com/878c88fd30730c968789988c747f3045d12d837b/content/shell/app/shell_main_delegate_mac.mm
[modify] https://crrev.com/878c88fd30730c968789988c747f3045d12d837b/content/shell/app/shell_main_mac.cc

Comment 8 by rsesek@chromium.org, Jun 15 2018

Status: Fixed (was: Started)
Blocking: 749839

Sign in to add a comment