Able to reproduce on M59 stable.

Tentatively handing this to alexmos@, +creis, who also might have opinions.

We've had similar-sounding issues when pushing origins out into distinct processes (which I'm pretty sure we do for `google.com`, because it's your search engine). Do you see this happening only when transitioning from `arturjanc.com` to `docs.google.com`? Or does it also happen from `google.com` -> `google.com`?

This might be something specific to *docs*.google.com -- redirecting through www.google.com doesn't remove the sandbox restrictions which is the expected behavior (I added a link for this case to the https://arturjanc.com/cgi-bin/sandbox-bug.py PoC).

Another interesting behavior is the following: 
1. Navigate to docs.google.com from a document in a CSP sandbox (click on the link in the second frame in the PoC)
2. In the JS console on docs.google.com type "location='https://example.org'"
3. In the JS console on example.org type "alert(1)" -- the browser chrome showing the alert will display "Google Drive" instead of "example.org says:"

Mike, I think I can't easily test the google.com -> google.com transition because I can't host the PoC in that origin.

If I try it in an incognito window, the script is blocked. I suspect that one of our corp-installed extensions/apps is doing something strange with the request to `docs.google.com`.

While I'm totally a jerk who would report bugs without disabling extensions, I think it might not be the case here ;-)

I tried it in non-corp mobile Chrome (Android; stable + canary) and I'm seeing the same thing: the window opened from docs.google.com executes scripts but redirecting through other origins doesn't. Perhaps we have some built-in, non-app/extension integration with Docs/Drive?

Do you see it happening in incognito mode for those same Chromes? Chrome itself bundles some extensions/apps, so I wouldn't put it past one or more of those causing weirdness.

You're right, this is probably the case (Incognito works as expected for me and blocks the scripts on docs.g.c).

Actually, that's still weird: we don't have extensions on Android. Or apps? I have no idea why you'd be seeing this behavior on Android outside of incognito. :(

Alex, Charlie: Any ideas where we should be looking for weirdnesses?

Thanks for the report!  From my initial investigation, it appears that something is wrong with tracking sandbox flags on the browser side when they are set from a CSP header.  At the time the new tab is created, the opener's sandbox flags in FrameTreeNode are empty.

This will cause issues whenever the new tab navigates cross-process from the original one.  I'm guessing this is happening for docs.google.com because of the Drive hosted app, which I believe covers docs.google.com in its web extent.  Do you have that app, and do you see a separate process prefixed by "App: " when clicking on the docs.g.c link?  I couldn't repro after I removed the Drive app.  But FWIW, I can also repro by simply doing a window.open("https://google.com") from https://arturjanc.com/cgi-bin/sandbox-bug.py, which also swaps processes as Mike mentioned, and which also appears to have script access.

Thanks for looking into this!

Yes, I see the "App: " process when clicking on the docs.g.c link, and also can reproduce that www.google.com opened in a new window will be able to execute scripts (whereas e.g. facebook.com will not).

So it appears that there's no easy fix for this, and I'm busy with a few other things right now and might not have time to work on this right away.  Ian/Luna -- would you have any interest in helping out with this, given that this is similar to what you needed to do for feature policy?  Does my proposal below sound reasonable?

Basically, I think that to fix this we need to separately track the sandbox flags from the container policy (i.e., defined in the frame owner) and the sandbox flags defined by the page itself via the CSP header in the browser process.  Currently, we only track the former in FrameReplicationState + FrameTreeNode, and when replicating to proxies, we kind of incorrectly assume that it corresponds to both FrameOwner flags and the flags in effect for the frame's SecurityContext:

void RenderFrameProxy::OnDidUpdateFramePolicy(
    blink::WebSandboxFlags flags,
    const ParsedFeaturePolicyHeader& container_policy) {
  web_frame_->SetReplicatedSandboxFlags(flags);
  web_frame_->SetFrameOwnerPolicy(flags,
                                  FeaturePolicyHeaderToWeb(container_policy));
}

That's not true with sandbox flags applied via CSP. :(  I've tried playing with adding in the CSP sandbox flags to existing sandbox_flags in the FRS, but I think that breaks down when A has <iframe sandbox="s1" src=B>, B sets sandbox flags "s2" via CSP, and then B navigates to C: we need to apply "s1" to the new page/renderer and not the combination of s1|s2.

I think feature policy faced a very similar situation because it can be set via both a header and a (snapshotted) container policy.  The replication state currently tracks feature policies for both of those separately (using |feature_policy_header| and |container_policy| in FrameReplicationState), and we probably need to do the same for sandbox flags, and ideally we'd be able to refactor/unify things along the way.

Here's one rough plan to fix this:

- Fold in the current |sandbox_flags| from FrameReplicationState into |container_policy|, and make it a struct that houses both sandbox flags and feature policy from the frame owner.  Use it to initialize SetFrameOwnerPolicy only for RemoteFrames.  I think this is something that iclelland@ was planning on doing at some point, after adding the container_policy plumbing in r465563.

- Add sandbox_flags_ to RenderFrameHostImpl to track sandbox flags that are currently in effect in the corresponding frame's SecurityContext.  This can combine the (effective) container flags and the CSP header flags.  Replication to proxies should then plumb this through and use it when calling SetReplicatedSandboxFlags() on RemoteFrames.  Tracking this on RenderFrameHostImpl will allow us to accurately know sandbox flags for pending/pending deletion RenderFrameHosts, similarly to last committed URL/origin/feature_policy_, which might be useful for doing security checks using those sandbox flags in the browser process.  

- Actually send CSP sandbox flags to the browser process, and use this to initialize RFHI::sandbox_flags_ above.  For example, this can be sent on commit, in DidCommitProvisionalLoad_Params, along with the new page's origin, etc.  We'll need to make sure that the CSP sandbox flags get cleared properly when navigating to a new page.  Ideally we'd get the sandbox directive from the header directly in the browser process, but in the short term we might still need Blink to parse it for us.  Browser process should validate this by ensuring that this is always a more restrictive set than the container sandbox flags.

- Audit existing code that uses FrameTreeNode::effective_sandbox_flags() - it should probably use the above instead.

Note that CSP in <meta> doesn't support the sandbox directive, so we only need to worry about the header.

I'm happy to help out with this, Alex. It definitely has a lot of overlap with the FP work in any case, but I'm glad to help.

Does CSP-embedded complicate this at all? Is the sandbox directive supported in an iframe csp attribute?

Thanks, Ian!  

Good question about <iframe csp>.  Looking at example 4 in https://w3c.github.io/webappsec-csp/embedded/#csp-attribute, "sandbox" seems to be a valid directive, so we probably need to support it.  I'd imagine if a page uses <iframe sandbox=s1 csp="sandbox 's2'">, then we apply s1|s2 to the subframe - Mike, is that right?

We already support replication for the csp attribute (as |required_csp| in FrameOwnerProperties).  I think if we send over a document's final sandbox frags at commit time, they should already include the flags from its frame owner's csp attribute.  So I guess the final sandbox flags in effect for a document can actually come from four sources:
- <iframe sandbox> (container policy)
- <iframe csp> 
- the CSP header for the document itself
- flags inherited from parent/opener document

iclelland: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Current status: WIP

Friendly ping from security sheriff. iclelland: have you been able to make anymore progress on this?

Yes, thanks for the ping -- I've been slowed by some other work, but I have a couple of CLs in progress; should hopefully submit for review next week (post BlinkOn)

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

commit 542ed06b5383710e26dd6027f7c1d404eaa296d5
Author: Ian Clelland <iclelland@google.com>
Date: Fri Oct 13 16:57:02 2017

Introduce FramePolicy struct

This change encapsulates both sandbox flags and the container feature
policy into a single struct called FramePolicy. The two components
should nearly always be set and replicated together, and this makes it
easier to pass them between methods and over IPC, and will help to make
clear when they are being manipulated separately.

Bug:  740556 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Iaaef51fb7d0e189e16ad5516118a1b37f58a582c
Reviewed-on: https://chromium-review.googlesource.com/712704
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508730}
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/browser/frame_host/frame_tree.cc
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/browser/frame_host/frame_tree.h
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/browser/frame_host/frame_tree_node_blame_context_unittest.cc
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/browser/frame_host/frame_tree_unittest.cc
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/browser/frame_host/navigation_controller_impl_unittest.cc
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/browser/frame_host/render_frame_host_feature_policy_unittest.cc
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/browser/frame_host/render_frame_host_manager_unittest.cc
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/browser/frame_host/render_frame_message_filter.cc
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/browser/renderer_host/render_process_host_unittest.cc
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/common/BUILD.gn
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/common/frame_messages.h
[add] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/common/frame_policy.cc
[add] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/common/frame_policy.h
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/renderer/render_frame_impl.h
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/renderer/render_frame_proxy.cc
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/renderer/render_frame_proxy.h
[modify] https://crrev.com/542ed06b5383710e26dd6027f7c1d404eaa296d5/content/test/test_render_frame_host.cc

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

commit cdc4f31606eb3d1b1546df94fe32897e62cfd69d
Author: Ian Clelland <iclelland@google.com>
Date: Fri Oct 13 22:24:12 2017

Use FramePolicy in FrameReplicationState and FrameTreeNode

This merges the sandbox_flags and container_policy members of both
FrameReplicationState and FrameTreeNode into FramePolicy structs.

Before:
  FrameReplicationState.sandbox_flags
  FrameReplicationState.container_policy
  FrameTreeNode.pending_sandbox_flags_
  FrameTreeNode.pending_container_policy_
  FrameTreeNode.SetPendingSandboxFlags
  FrameTreeNode.SetPendingContainerPolicy
  FrameTreeNode.pending_sandbox_flags()
  FrameTreeNode.pending_container_policy()
  FrameTreeNode.effective_sandbox_flags()
  FrameTreeNode.effective_container_policy()

After:
  FrameReplicationState.frame_policy
  FrameTreeNode.pending_frame_policy_
  FrameTreeNode.SetPendingFramePolicy()
  FrameTreeNode.pending_frame_policy()
  FrameTreeNode.effective_frame_policy()


Bug:  740556 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Ida1aedb486689cfb31d50be418c0cb039850145c
Reviewed-on: https://chromium-review.googlesource.com/713236
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508845}
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/browser/frame_host/frame_tree.cc
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/browser/frame_host/frame_tree_browsertest.cc
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/browser/frame_host/frame_tree_node.cc
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/browser/frame_host/frame_tree_node.h
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/common/frame_messages.h
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/common/frame_replication_state.cc
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/common/frame_replication_state.h
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/renderer/render_frame_proxy.cc
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/renderer/render_view_browsertest.cc
[modify] https://crrev.com/cdc4f31606eb3d1b1546df94fe32897e62cfd69d/content/renderer/render_view_impl.cc

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

commit 5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d
Author: Ian Clelland <iclelland@google.com>
Date: Mon Nov 27 22:00:03 2017

Track CSP-set sandbox flags separately from frame owner flags

Bug:  740556 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: If696428160446e636eaa1a9fa3ef7e067b0c8922
Reviewed-on: https://chromium-review.googlesource.com/712711
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519420}
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/browser/frame_host/frame_tree_browsertest.cc
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/browser/frame_host/frame_tree_node.cc
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/browser/frame_host/frame_tree_node.h
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/browser/frame_host/render_frame_host_feature_policy_unittest.cc
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/browser/frame_host/render_frame_host_manager.h
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/browser/frame_host/render_frame_host_manager_unittest.cc
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/common/frame_messages.h
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/common/frame_replication_state.cc
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/common/frame_replication_state.h
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/renderer/render_frame_impl.h
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/renderer/render_frame_proxy.cc
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/renderer/render_frame_proxy.h
[add] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/test/data/csp_sandboxed_frame.html
[add] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/test/data/csp_sandboxed_frame.html.mock-http-headers
[add] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/test/data/sandbox_main_frame_csp.html
[add] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/test/data/sandbox_main_frame_csp.html.mock-http-headers
[add] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/test/data/sandboxed_child_frame.html
[add] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/test/data/sandboxed_child_frame.html.mock-http-headers
[add] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/test/data/sandboxed_frames_csp.html
[add] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/test/data/sandboxed_main_frame_script.html
[add] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/test/data/sandboxed_main_frame_script.html.mock-http-headers
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/test/test_render_frame_host.cc
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/content/test/test_render_frame_host.h
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/third_party/WebKit/Source/core/dom/PolicyTest.cpp
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/third_party/WebKit/Source/core/dom/RemoteSecurityContext.h
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/third_party/WebKit/Source/core/dom/SecurityContext.h
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.cpp
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/third_party/WebKit/Source/core/exported/LocalFrameClientImpl.h
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/third_party/WebKit/Source/core/exported/WebRemoteFrameImpl.cpp
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/third_party/WebKit/Source/core/frame/LocalFrameClient.h
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
[modify] https://crrev.com/5cbaaf8a0fbb6f2ef61fb12f7b461cf8009d985d/third_party/WebKit/public/web/WebFrameClient.h

This should be fixed now; the original PoC no longer appears to work; all frames keep sandbox restrictions. (As of 64.0.3279.0)

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot