Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2920353002:
Add the V2 sandbox rules for renderer processes. (Closed)
DescriptionAdd the V2 sandbox rules for renderer processes.
Add the V2 sandbox rules, which eliminate the unsandboxed warmup
phase in favor of explicitly enumerating resource access, to the
tree.
BUG=689306
Review-Url: https://codereview.chromium.org/2920353002
Cr-Commit-Position: refs/heads/master@{#478443}
Committed: https://chromium.googlesource.com/chromium/src/+/147d8ebbba78a52af03a6ab1570af96536b4e1fa
Patch Set 1 #Patch Set 2 : Add the rules file #
Total comments: 11
Patch Set 3 : Cleanup indentation and comments #Patch Set 4 : Remove unused mach services #
Total comments: 2
Patch Set 5 : Fix last nit #
Messages
Total messages: 20 (10 generated)
kerrnel@chromium.org changed reviewers: + rsesek@chromium.org
rsesek@, PTAL. Are you OK with landing the V2 sandbox rules before they can actually be used? The alternative is to land the 150 line profile with the CL enabling the V2 sandbox itself, and so far this work has been much easier to land piece by piece. I will audit the TODOs once we actually have the full V2 plumbing in place.
The CQ bit was checked by kerrnel@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
I'm okay with landing the v2 rules now, but I think I'd like the TODOs for Mach services resolved. https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... File content/renderer/renderer_v2.sb (right): https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... content/renderer/renderer_v2.sb:6: ; The top of this will be the V2 common profile. Why not have common_v2.sb now? https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... content/renderer/renderer_v2.sb:31: (deny default (with no-log)) Why 4-space indents instead of the usual 2? https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... content/renderer/renderer_v2.sb:43: ; Allow logging for all processes nit, and throughout this file: comments need proper punctuation. https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... content/renderer/renderer_v2.sb:67: ; This is the renderer specific stuff. Better put as "; End of common.sb" ? https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... content/renderer/renderer_v2.sb:76: (path "/dev/autofs_nowait") Now 8-space indents? https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... content/renderer/renderer_v2.sb:114: ; TODO(kerrnel): nix if possible. Can these be resolved before landing?
On 2017/06/06 14:56:01, Robert Sesek wrote: > I'm okay with landing the v2 rules now, but I think I'd like the TODOs for Mach > services resolved. > > https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... > File content/renderer/renderer_v2.sb (right): > > https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... > content/renderer/renderer_v2.sb:6: ; The top of this will be the V2 common > profile. > Why not have common_v2.sb now? > > https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... > content/renderer/renderer_v2.sb:31: (deny default (with no-log)) > Why 4-space indents instead of the usual 2? > > https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... > content/renderer/renderer_v2.sb:43: ; Allow logging for all processes > nit, and throughout this file: comments need proper punctuation. > > https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... > content/renderer/renderer_v2.sb:67: ; This is the renderer specific stuff. > Better put as "; End of common.sb" ? > > https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... > content/renderer/renderer_v2.sb:76: (path "/dev/autofs_nowait") > Now 8-space indents? > > https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... > content/renderer/renderer_v2.sb:114: ; TODO(kerrnel): nix if possible. > Can these be resolved before landing? I can't resolve the questions about mach-services until I write the rest of the V2 sandbox. What we can do is hold this off until the main CL is ready and make this a dependent CL. SG? - Greg
On 2017/06/06 17:10:56, Greg K wrote: > I can't resolve the questions about mach-services until I write the rest of the > V2 sandbox. What we can do is hold this off until the main CL is ready and make > this a dependent CL. SG? SGTM
https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... File content/renderer/renderer_v2.sb (right): https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... content/renderer/renderer_v2.sb:6: ; The top of this will be the V2 common profile. On 2017/06/06 14:56:00, Robert Sesek wrote: > Why not have common_v2.sb now? It's not clear exactly what will be abstracted out, so I thought it made more sense to do the abstraction as soon as another process type uses the sandbox and I know what the actual diff is. https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... content/renderer/renderer_v2.sb:31: (deny default (with no-log)) On 2017/06/06 14:56:00, Robert Sesek wrote: > Why 4-space indents instead of the usual 2? Done. https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... content/renderer/renderer_v2.sb:43: ; Allow logging for all processes On 2017/06/06 14:56:00, Robert Sesek wrote: > nit, and throughout this file: comments need proper punctuation. Done. https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... content/renderer/renderer_v2.sb:67: ; This is the renderer specific stuff. On 2017/06/06 14:56:00, Robert Sesek wrote: > Better put as "; End of common.sb" ? Done. https://codereview.chromium.org/2920353002/diff/20001/content/renderer/render... content/renderer/renderer_v2.sb:76: (path "/dev/autofs_nowait") On 2017/06/06 14:56:00, Robert Sesek wrote: > Now 8-space indents? Done.
avi@chromium.org changed reviewers: + avi@chromium.org
FYI drive-by content lgtm cool stuff!
LGTM w/ a nit https://codereview.chromium.org/2920353002/diff/60001/content/renderer/render... File content/renderer/renderer_v2.sb (right): https://codereview.chromium.org/2920353002/diff/60001/content/renderer/render... content/renderer/renderer_v2.sb:106: (allow iokit-open (iokit-registry-entry-class "RootDomainUserClient")) Structure this like the other multi-target allow rules?
The CQ bit was checked by kerrnel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/2920353002/#ps80001 (title: "Fix last nit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2920353002/diff/60001/content/renderer/render... File content/renderer/renderer_v2.sb (right): https://codereview.chromium.org/2920353002/diff/60001/content/renderer/render... content/renderer/renderer_v2.sb:106: (allow iokit-open (iokit-registry-entry-class "RootDomainUserClient")) On 2017/06/09 21:44:48, Robert Sesek wrote: > Structure this like the other multi-target allow rules? Done.
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497045411515330,
"parent_rev": "4a15f818c5fdbd1e2c44bf102047ecd9b2c166e2", "commit_rev":
"147d8ebbba78a52af03a6ab1570af96536b4e1fa"}
Message was sent while issue was closed.
Description was changed from ========== Add the V2 sandbox rules for renderer processes. Add the V2 sandbox rules, which eliminate the unsandboxed warmup phase in favor of explicitly enumerating resource access, to the tree. BUG=689306 ========== to ========== Add the V2 sandbox rules for renderer processes. Add the V2 sandbox rules, which eliminate the unsandboxed warmup phase in favor of explicitly enumerating resource access, to the tree. BUG=689306 Review-Url: https://codereview.chromium.org/2920353002 Cr-Commit-Position: refs/heads/master@{#478443} Committed: https://chromium.googlesource.com/chromium/src/+/147d8ebbba78a52af03a6ab1570a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/147d8ebbba78a52af03a6ab1570a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
