|
|
Created:
4 years, 3 months ago by sadrul Modified:
3 years, 2 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, kalyank, darin (slow to review), rjkroege, fwang, tonikitoo Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionservices/shell: Turn on sandboxing for mojo apps.
Turn on sandbox for mojo apps by default. Apps that need to run
without a sandbox need to include 'shell:unsandboxed' in their
capabilities in manifest for mojo:shell.
Update manifests for mojo:ui, mojo:font_service, mojo:file_system,
and mojo:leveldb services to include 'shell:unsandboxed'.
BUG=645602, 646376
Committed: https://crrev.com/43aa4f706d6020c93f11fa952d7f1d7a7337c570
Cr-Commit-Position: refs/heads/master@{#419891}
Patch Set 1 : . #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #Patch Set 7 : . #Patch Set 8 : . #Patch Set 9 : . #
Dependent Patchsets: Messages
Total messages: 60 (45 generated)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== wip: turn on sandbox for mojo apps. BUG= ========== to ========== services/shell: Turn on sandboxing for mojo apps. . mojo manifests can have a 'privileged' entry. Turning it on means the shell wouldn't start sandbox for that service. The service itself, however, can initiate a sandbox if it wants to. The shell starts the sandbox before launching the app if 'privileged' is not turned on in the manifest. . 'privileged' is turned on for mojo:ui, mojo:font_service, mojo:file_system, and mojo:leveldb services. BUG=645602, 646376 ==========
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sadrul@chromium.org changed reviewers: + ben@chromium.org, rockot@chromium.org, tsepez@chromium.org
Hello! Does this look like a reasonable approach for dealing with sandboxing issues for now? I think we would want to share more sandbox code from content (e.g. content/common/sandbox_linux) in shell. I also think the public shell client API should have functions a client can use to get into a sandbox (e.g. mus:gpu would start unsandboxed, but would start the sandbox after doing some pre-sandbox initialization).
On 2016/09/14 at 15:18:42, sadrul wrote: > Hello! Does this look like a reasonable approach for dealing with sandboxing issues for now? I think we would want to share more sandbox code from content (e.g. content/common/sandbox_linux) in shell. I also think the public shell client API should have functions a client can use to get into a sandbox (e.g. mus:gpu would start unsandboxed, but would start the sandbox after doing some pre-sandbox initialization). I don't think we should expect or require apps like mus to link against sandbox configuration code. We also already have capabilities as a unified way of managing, well...capabilities :) And having the ability to run any code outside of a sandbox feels like a capability to be managed. Rather than a "privileged" key, what about adding a builtin shell capability class similar to shell:all_users etc (like "shell:pre_sandbox_init"). This can be a signal for the shell to pass a flag to the NativeRunner, and that flag can tell the runner to call some DSO entry point like "PreSandboxServiceInit()" before initializing the sandbox. WDYT?
Can we use a more specific term than Privileged since it really means "no-sandbox", e.g. "sandbox": false, or some such?
On 2016/09/14 16:04:14, Tom Sepez wrote: > Can we use a more specific term than Privileged since it really means > "no-sandbox", e.g. "sandbox": false, or some such? Also, this defaults to "unprivileged", right? We want this to be safe by default.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/09/14 15:40:12, Ken Rockot wrote: > On 2016/09/14 at 15:18:42, sadrul wrote: > > Hello! Does this look like a reasonable approach for dealing with sandboxing > issues for now? I think we would want to share more sandbox code from content > (e.g. content/common/sandbox_linux) in shell. I also think the public shell > client API should have functions a client can use to get into a sandbox (e.g. > mus:gpu would start unsandboxed, but would start the sandbox after doing some > pre-sandbox initialization). > > I don't think we should expect or require apps like mus to link against sandbox > configuration code. > > We also already have capabilities as a unified way of managing, > well...capabilities :) And having the ability to run any code outside of a > sandbox feels like a capability to be managed. > > Rather than a "privileged" key, what about adding a builtin shell capability > class similar to shell:all_users etc (like "shell:pre_sandbox_init"). This can > be a signal for the shell to pass a flag to the NativeRunner, and that flag can > tell the runner to call some DSO entry point like "PreSandboxServiceInit()" > before initializing the sandbox. WDYT? I like this idea in general. I think we are going to need three types of privileges: (1) fully sandboxed, (2) sandbox with pre-init, and (3) completely unsandboxed. mojo:ui, font_services, filesystem and leveldb: all these services need to be of type (3). mojo:gpu would be of type (2). However, for the gpu, the sandbox initialization is pretty tied up with the gpu initialization, and it'd be better if the client could explicitly initiate the sandbox (relevant GpuSandboxHelper: https://cs.chromium.org/chromium/src/gpu/ipc/service/gpu_init.h?sq=package:ch...). For example, the pre-sandbox code could start a thread, and before it enters the sandbox, the client needs to stop the thread, and restart the thread again after entering the sandbox (e.g. https://cs.chromium.org/chromium/src/content/gpu/gpu_main.cc?type=cs&sq=packa...). So the client would need to be fairly involved in the sandbox initialization process. So for (2), I still feel like it would be better if there's some API the client could use to get into the sandbox, instead of the shell doing this. How about for now, I add support for (1) and (3). I can look into farther untangling the gpu initilization stages, so that (2) could work (i.e. some DSO entry point). Otherwise, for mus:gpu, we can probably just do (3), and then explicitly enter the sandbox from the client. WDYT? On 2016/09/14 16:06:29, Tom Sepez wrote: > On 2016/09/14 16:04:14, Tom Sepez wrote: > > Can we use a more specific term than Privileged since it really means > > "no-sandbox", e.g. "sandbox": false, or some such? > > Also, this defaults to "unprivileged", right? We want this to be safe by > default. As of now in tip of tree, the default is privileged (i.e. we never start the sandbox). I switch that to default to unprivileged (i.e. fully sandboxed) in this CL (see https://codereview.chromium.org/2338793003/diff/100001/services/shell/service...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/15 at 04:26:54, sadrul wrote: > On 2016/09/14 15:40:12, Ken Rockot wrote: > > On 2016/09/14 at 15:18:42, sadrul wrote: > > > Hello! Does this look like a reasonable approach for dealing with sandboxing > > issues for now? I think we would want to share more sandbox code from content > > (e.g. content/common/sandbox_linux) in shell. I also think the public shell > > client API should have functions a client can use to get into a sandbox (e.g. > > mus:gpu would start unsandboxed, but would start the sandbox after doing some > > pre-sandbox initialization). > > > > I don't think we should expect or require apps like mus to link against sandbox > > configuration code. > > > > We also already have capabilities as a unified way of managing, > > well...capabilities :) And having the ability to run any code outside of a > > sandbox feels like a capability to be managed. > > > > Rather than a "privileged" key, what about adding a builtin shell capability > > class similar to shell:all_users etc (like "shell:pre_sandbox_init"). This can > > be a signal for the shell to pass a flag to the NativeRunner, and that flag can > > tell the runner to call some DSO entry point like "PreSandboxServiceInit()" > > before initializing the sandbox. WDYT? > > I like this idea in general. > > I think we are going to need three types of privileges: (1) fully sandboxed, (2) sandbox with pre-init, and (3) completely unsandboxed. mojo:ui, font_services, filesystem and leveldb: all these services need to be of type (3). mojo:gpu would be of type (2). However, for the gpu, the sandbox initialization is pretty tied up with the gpu initialization, and it'd be better if the client could explicitly initiate the sandbox (relevant GpuSandboxHelper: https://cs.chromium.org/chromium/src/gpu/ipc/service/gpu_init.h?sq=package:ch...). For example, the pre-sandbox code could start a thread, and before it enters the sandbox, the client needs to stop the thread, and restart the thread again after entering the sandbox (e.g. https://cs.chromium.org/chromium/src/content/gpu/gpu_main.cc?type=cs&sq=packa...). So the client would need to be fairly involved in the sandbox initialization process. So for (2), I still feel like it would be better if there's some API the client could use to get into the sandbox, instead of the shell doing this. > > How about for now, I add support for (1) and (3). I can look into farther untangling the gpu initilization stages, so that (2) could work (i.e. some DSO entry point). Otherwise, for mus:gpu, we can probably just do (3), and then explicitly enter the sandbox from the client. WDYT? Yep, all sounds reasonable to me, and ServiceManager LGTM > > On 2016/09/14 16:06:29, Tom Sepez wrote: > > On 2016/09/14 16:04:14, Tom Sepez wrote: > > > Can we use a more specific term than Privileged since it really means > > > "no-sandbox", e.g. "sandbox": false, or some such? > > > > Also, this defaults to "unprivileged", right? We want this to be safe by > > default. > > As of now in tip of tree, the default is privileged (i.e. we never start the sandbox). I switch that to default to unprivileged (i.e. fully sandboxed) in this CL (see https://codereview.chromium.org/2338793003/diff/100001/services/shell/service...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ben@: ping for the manifest changes.
Description was changed from ========== services/shell: Turn on sandboxing for mojo apps. . mojo manifests can have a 'privileged' entry. Turning it on means the shell wouldn't start sandbox for that service. The service itself, however, can initiate a sandbox if it wants to. The shell starts the sandbox before launching the app if 'privileged' is not turned on in the manifest. . 'privileged' is turned on for mojo:ui, mojo:font_service, mojo:file_system, and mojo:leveldb services. BUG=645602, 646376 ========== to ========== services/shell: Turn on sandboxing for mojo apps. Turn on sandbox for mojo apps by default. Apps that need to run without a sandbox need to include 'shell:unsandboxed' in their capabilities in manifest for mojo:shell. Update manifests for mojo:ui, mojo:font_service, mojo:file_system, and mojo:leveldb services to include 'shell:unsandboxed'. BUG=645602, 646376 ==========
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by sadrul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2338793003/#ps180001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== services/shell: Turn on sandboxing for mojo apps. Turn on sandbox for mojo apps by default. Apps that need to run without a sandbox need to include 'shell:unsandboxed' in their capabilities in manifest for mojo:shell. Update manifests for mojo:ui, mojo:font_service, mojo:file_system, and mojo:leveldb services to include 'shell:unsandboxed'. BUG=645602, 646376 ========== to ========== services/shell: Turn on sandboxing for mojo apps. Turn on sandbox for mojo apps by default. Apps that need to run without a sandbox need to include 'shell:unsandboxed' in their capabilities in manifest for mojo:shell. Update manifests for mojo:ui, mojo:font_service, mojo:file_system, and mojo:leveldb services to include 'shell:unsandboxed'. BUG=645602, 646376 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== services/shell: Turn on sandboxing for mojo apps. Turn on sandbox for mojo apps by default. Apps that need to run without a sandbox need to include 'shell:unsandboxed' in their capabilities in manifest for mojo:shell. Update manifests for mojo:ui, mojo:font_service, mojo:file_system, and mojo:leveldb services to include 'shell:unsandboxed'. BUG=645602, 646376 ========== to ========== services/shell: Turn on sandboxing for mojo apps. Turn on sandbox for mojo apps by default. Apps that need to run without a sandbox need to include 'shell:unsandboxed' in their capabilities in manifest for mojo:shell. Update manifests for mojo:ui, mojo:font_service, mojo:file_system, and mojo:leveldb services to include 'shell:unsandboxed'. BUG=645602, 646376 Committed: https://crrev.com/43aa4f706d6020c93f11fa952d7f1d7a7337c570 Cr-Commit-Position: refs/heads/master@{#419891} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/43aa4f706d6020c93f11fa952d7f1d7a7337c570 Cr-Commit-Position: refs/heads/master@{#419891}
Message was sent while issue was closed.
I suspect that this CL causes reliable ChromeOS failures in mash_browser_tests: https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%... https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%... https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%... Speculatively reverting to see if it solves the problem.
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:180001) has been created in https://codereview.chromium.org/2353783004/ by loyso@chromium.org. The reason for reverting is: Causes a timeout on mash_browser_tests..
Message was sent while issue was closed.
Description was changed from ========== services/shell: Turn on sandboxing for mojo apps. Turn on sandbox for mojo apps by default. Apps that need to run without a sandbox need to include 'shell:unsandboxed' in their capabilities in manifest for mojo:shell. Update manifests for mojo:ui, mojo:font_service, mojo:file_system, and mojo:leveldb services to include 'shell:unsandboxed'. BUG=645602, 646376 Committed: https://crrev.com/43aa4f706d6020c93f11fa952d7f1d7a7337c570 Cr-Commit-Position: refs/heads/master@{#419891} ========== to ========== services/shell: Turn on sandboxing for mojo apps. Turn on sandbox for mojo apps by default. Apps that need to run without a sandbox need to include 'shell:unsandboxed' in their capabilities in manifest for mojo:shell. Update manifests for mojo:ui, mojo:font_service, mojo:file_system, and mojo:leveldb services to include 'shell:unsandboxed'. BUG=645602, 646376 Committed: https://crrev.com/43aa4f706d6020c93f11fa952d7f1d7a7337c570 Cr-Commit-Position: refs/heads/master@{#419891} ==========
On 2016/09/21 01:06:45, loyso (OOO) wrote: > A revert of this CL (patchset #9 id:180001) has been created in > https://codereview.chromium.org/2353783004/ by mailto:loyso@chromium.org. > > The reason for reverting is: Causes a timeout on mash_browser_tests.. Ě |