From 5677a42db99766c5f43fb0a81a4b869a8c4581ec Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Wed, 28 Nov 2018 13:28:29 -0800 Subject: [PATCH 1/5] feat: support mixed-sandbox mode on linux --- atom/browser/web_contents_preferences.cc | 6 +- patches/common/chromium/.patches | 1 + .../support_mixed_sandbox_with_zygote.patch | 71 +++++++++++++++++++ 3 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 patches/common/chromium/support_mixed_sandbox_with_zygote.patch diff --git a/atom/browser/web_contents_preferences.cc b/atom/browser/web_contents_preferences.cc index 405cdabba9b14..93dd2be03eac7 100644 --- a/atom/browser/web_contents_preferences.cc +++ b/atom/browser/web_contents_preferences.cc @@ -245,10 +245,12 @@ void WebContentsPreferences::AppendCommandLineSwitches( // If the `sandbox` option was passed to the BrowserWindow's webPreferences, // pass `--enable-sandbox` to the renderer so it won't have any node.js // integration. - if (IsEnabled(options::kSandbox)) + if (IsEnabled(options::kSandbox)) { command_line->AppendSwitch(switches::kEnableSandbox); - else if (!command_line->HasSwitch(switches::kEnableSandbox)) + } else if (!command_line->HasSwitch(switches::kEnableSandbox)) { command_line->AppendSwitch(service_manager::switches::kNoSandbox); + command_line->AppendSwitch(::switches::kNoZygote); + } // Check if nativeWindowOpen is enabled. if (IsEnabled(options::kNativeWindowOpen)) diff --git a/patches/common/chromium/.patches b/patches/common/chromium/.patches index c22c1636a6823..e26d1b27a1628 100644 --- a/patches/common/chromium/.patches +++ b/patches/common/chromium/.patches @@ -73,3 +73,4 @@ cross_site_document_resource_handler.patch content_allow_embedder_to_prevent_locking_scheme_registry.patch fix_trackpad_scrolling.patch mac_fix_form_control_rendering_on_10_14_mojave.patch +support_mixed_sandbox_with_zygote.patch diff --git a/patches/common/chromium/support_mixed_sandbox_with_zygote.patch b/patches/common/chromium/support_mixed_sandbox_with_zygote.patch new file mode 100644 index 0000000000000..f643ecebfb455 --- /dev/null +++ b/patches/common/chromium/support_mixed_sandbox_with_zygote.patch @@ -0,0 +1,71 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Jeremy Apthorp +Date: Wed, 28 Nov 2018 13:20:27 -0800 +Subject: support mixed-sandbox with zygote + +On Linux, Chromium launches all new renderer processes via a "zygote" +process which has the sandbox pre-initialized (see +//docs/linux_zygote.md). In order to support mixed-sandbox mode, in +which some renderers are launched with the sandbox engaged and others +without it, we need the option to launch non-sandboxed renderers without +going through the zygote. + +Chromium already supports a `--no-zygote` flag, but it turns off the +zygote completely, and thus also disables sandboxing. This patch allows +the `--no-zygote` flag to affect renderer processes on a case-by-case +basis, checking immediately prior to launch whether to go through the +zygote or not based on the command-line of the to-be-launched renderer. + +This patch could conceivably be upstreamed, as it does not affect +production Chromium (which does not use the `--no-zygote` flag). +However, the patch would need to be reviewed by the security team, as it +does touch a security-sensitive class. + +diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc +index 05e0ee79e5ad..80512d14c3dc 100644 +--- a/content/browser/renderer_host/render_process_host_impl.cc ++++ b/content/browser/renderer_host/render_process_host_impl.cc +@@ -465,7 +465,7 @@ SiteProcessMap* GetSiteProcessMapForBrowserContext(BrowserContext* context) { + class RendererSandboxedProcessLauncherDelegate + : public SandboxedProcessLauncherDelegate { + public: +- RendererSandboxedProcessLauncherDelegate() {} ++ RendererSandboxedProcessLauncherDelegate(bool use_zygote): use_zygote_(use_zygote) {} + + ~RendererSandboxedProcessLauncherDelegate() override {} + +@@ -485,6 +485,9 @@ class RendererSandboxedProcessLauncherDelegate + + #if BUILDFLAG(USE_ZYGOTE_HANDLE) + service_manager::ZygoteHandle GetZygote() override { ++ if (!use_zygote_) { ++ return nullptr; ++ } + const base::CommandLine& browser_command_line = + *base::CommandLine::ForCurrentProcess(); + base::CommandLine::StringType renderer_prefix = +@@ -498,6 +501,9 @@ class RendererSandboxedProcessLauncherDelegate + service_manager::SandboxType GetSandboxType() override { + return service_manager::SANDBOX_TYPE_RENDERER; + } ++ ++ private: ++ bool use_zygote_; + }; + + const char kSessionStorageHolderKey[] = "kSessionStorageHolderKey"; +@@ -1731,11 +1737,13 @@ bool RenderProcessHostImpl::Init() { + cmd_line->PrependWrapper(renderer_prefix); + AppendRendererCommandLine(cmd_line.get()); + ++ bool use_zygote = !cmd_line->HasSwitch(switches::kNoZygote); ++ + // Spawn the child process asynchronously to avoid blocking the UI thread. + // As long as there's no renderer prefix, we can use the zygote process + // at this stage. + child_process_launcher_ = std::make_unique( +- std::make_unique(), ++ std::make_unique(use_zygote), + std::move(cmd_line), GetID(), this, std::move(mojo_invitation_), + base::BindRepeating(&RenderProcessHostImpl::OnMojoError, id_)); + channel_->Pause(); From 02f2e745aa052a1fc0d6a3949549ebb6b05b01f1 Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Thu, 29 Nov 2018 15:31:11 -0800 Subject: [PATCH 2/5] fix build on non-zygote platforms --- .../support_mixed_sandbox_with_zygote.patch | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/patches/common/chromium/support_mixed_sandbox_with_zygote.patch b/patches/common/chromium/support_mixed_sandbox_with_zygote.patch index f643ecebfb455..dd4c3d15a4fb0 100644 --- a/patches/common/chromium/support_mixed_sandbox_with_zygote.patch +++ b/patches/common/chromium/support_mixed_sandbox_with_zygote.patch @@ -22,19 +22,21 @@ However, the patch would need to be reviewed by the security team, as it does touch a security-sensitive class. diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc -index 05e0ee79e5ad..80512d14c3dc 100644 +index 05e0ee79e5ad..9a4522f59e6f 100644 --- a/content/browser/renderer_host/render_process_host_impl.cc +++ b/content/browser/renderer_host/render_process_host_impl.cc -@@ -465,7 +465,7 @@ SiteProcessMap* GetSiteProcessMapForBrowserContext(BrowserContext* context) { - class RendererSandboxedProcessLauncherDelegate +@@ -466,6 +466,10 @@ class RendererSandboxedProcessLauncherDelegate : public SandboxedProcessLauncherDelegate { public: -- RendererSandboxedProcessLauncherDelegate() {} -+ RendererSandboxedProcessLauncherDelegate(bool use_zygote): use_zygote_(use_zygote) {} + RendererSandboxedProcessLauncherDelegate() {} ++#if BUILDFLAG(USE_ZYGOTE_HANDLE) ++ RendererSandboxedProcessLauncherDelegate(bool use_zygote): ++ use_zygote_(use_zygote) {} ++#endif ~RendererSandboxedProcessLauncherDelegate() override {} -@@ -485,6 +485,9 @@ class RendererSandboxedProcessLauncherDelegate +@@ -485,6 +489,9 @@ class RendererSandboxedProcessLauncherDelegate #if BUILDFLAG(USE_ZYGOTE_HANDLE) service_manager::ZygoteHandle GetZygote() override { @@ -44,28 +46,35 @@ index 05e0ee79e5ad..80512d14c3dc 100644 const base::CommandLine& browser_command_line = *base::CommandLine::ForCurrentProcess(); base::CommandLine::StringType renderer_prefix = -@@ -498,6 +501,9 @@ class RendererSandboxedProcessLauncherDelegate +@@ -498,6 +505,11 @@ class RendererSandboxedProcessLauncherDelegate service_manager::SandboxType GetSandboxType() override { return service_manager::SANDBOX_TYPE_RENDERER; } + + private: -+ bool use_zygote_; ++#if BUILDFLAG(USE_ZYGOTE_HANDLE) ++ bool use_zygote_ = true; ++#endif }; const char kSessionStorageHolderKey[] = "kSessionStorageHolderKey"; -@@ -1731,11 +1737,13 @@ bool RenderProcessHostImpl::Init() { +@@ -1731,11 +1743,18 @@ bool RenderProcessHostImpl::Init() { cmd_line->PrependWrapper(renderer_prefix); AppendRendererCommandLine(cmd_line.get()); ++#if BUILDFLAG(USE_ZYGOTE_HANDLE) + bool use_zygote = !cmd_line->HasSwitch(switches::kNoZygote); ++ auto delegate = std::make_unique(use_zygote); ++#else ++ auto delegate = std::make_unique(); ++#endif + // Spawn the child process asynchronously to avoid blocking the UI thread. // As long as there's no renderer prefix, we can use the zygote process // at this stage. child_process_launcher_ = std::make_unique( - std::make_unique(), -+ std::make_unique(use_zygote), ++ std::move(delegate), std::move(cmd_line), GetID(), this, std::move(mojo_invitation_), base::BindRepeating(&RenderProcessHostImpl::OnMojoError, id_)); channel_->Pause(); From 398e9fc78b59a6ee15de16771bbb5403ccbf5217 Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Mon, 3 Dec 2018 15:54:29 -0800 Subject: [PATCH 3/5] enable app sandbox tests on linux --- spec/api-app-spec.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index c44cef392a97f..a7f0e323955e8 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -930,13 +930,6 @@ describe('app module', () => { const socketPath = process.platform === 'win32' ? '\\\\.\\pipe\\electron-mixed-sandbox' : '/tmp/electron-mixed-sandbox' beforeEach(function (done) { - // XXX(alexeykuzmin): Calling `.skip()` inside a `before` hook - // doesn't affect nested `describe`s. - // FIXME Get these specs running on Linux - if (process.platform === 'linux') { - this.skip() - } - fs.unlink(socketPath, () => { server = net.createServer() server.listen(socketPath) From cac38c875b4ffd22b83169f3ae4e37658f8d4839 Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Tue, 4 Dec 2018 15:14:53 -0800 Subject: [PATCH 4/5] re-disable the tests on ARM because docker --- spec/api-app-spec.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index a7f0e323955e8..48be7ce94cd5c 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -930,6 +930,20 @@ describe('app module', () => { const socketPath = process.platform === 'win32' ? '\\\\.\\pipe\\electron-mixed-sandbox' : '/tmp/electron-mixed-sandbox' beforeEach(function (done) { + if (process.platform === 'linux' && (process.arch === 'arm64' || process.arch === 'arm')) { + // Our ARM tests are run on VSTS rather than CircleCI, and the Docker + // setup on VSTS disallows syscalls that Chrome requires for setting up + // sandboxing. + // See: + // - https://docs.docker.com/engine/security/seccomp/#significant-syscalls-blocked-by-the-default-profile + // - https://chromium.googlesource.com/chromium/src/+/70.0.3538.124/sandbox/linux/services/credentials.cc#292 + // - https://github.com/docker/docker-ce/blob/ba7dfc59ccfe97c79ee0d1379894b35417b40bca/components/engine/profiles/seccomp/seccomp_default.go#L497 + // + // Adding `--cap-add SYS_ADMIN` or `--security-opt seccomp=unconfined` + // to the Docker invocation allows the syscalls that Chrome needs, but + // are probably more permissive than we'd like. + this.skip() + } fs.unlink(socketPath, () => { server = net.createServer() server.listen(socketPath) From 037cc87fd7f4af28d142beae290f07b6c5865141 Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Tue, 4 Dec 2018 15:25:30 -0800 Subject: [PATCH 5/5] link to jessfraz's blog post about seccomp and chrome --- spec/api-app-spec.js | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index 48be7ce94cd5c..3477d686368c5 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -938,6 +938,7 @@ describe('app module', () => { // - https://docs.docker.com/engine/security/seccomp/#significant-syscalls-blocked-by-the-default-profile // - https://chromium.googlesource.com/chromium/src/+/70.0.3538.124/sandbox/linux/services/credentials.cc#292 // - https://github.com/docker/docker-ce/blob/ba7dfc59ccfe97c79ee0d1379894b35417b40bca/components/engine/profiles/seccomp/seccomp_default.go#L497 + // - https://blog.jessfraz.com/post/how-to-use-new-docker-seccomp-profiles/ // // Adding `--cap-add SYS_ADMIN` or `--security-opt seccomp=unconfined` // to the Docker invocation allows the syscalls that Chrome needs, but