Stop frames from allowing themselves to navigate top without gesture [chromium/src : main]
Liam Brady (Gerrit)
Attention is currently required from: Antonio Sartori, Arthur Sonzogni, Josh Karlin, Liam Brady.
Liam Brady uploaded patch set #47 to this change.
Stop frames from allowing themselves to navigate top without gesture
A previous CL stopped a sandboxed iframe from navigating top if its
ancestor was not allowed to navigate top[1]. However, there is still a
corner case that allows a sandboxed frame to give itself permission to
navigate top if the top-level page doesn't have any sandbox flags set.
For this attack, a cross origin iframe is embedded in a page. At
creation time, both the main page and the iframe are unsandboxed, which
means that the iframe requires sticky user activation to be able to
navigate the top frame. Then, this iframe loads a page whose response
header's CSP includes `sandbox allow-top-navigation`. The cross-origin
page just gave itself permission to navigate the top-level frame
without sticky user activation.
This is problematic because it allows a cross-origin iframe to
circumvent our framebust intervention and redirect the top page to a
potentially malicious page. Currently, there is no way for the renderer
to detect this. It can't distinguish between sandbox flags set on the
frame itself by the embedding page and sandbox flags delivered in the
response headers. There are 2 checks in LocalFrame::CanNavigate that are
looking at the 2 top navigation sandbox flags, but by that point the
frame sandbox flags and delivered sandbox flags have been squashed into
one place, so there's no way to know if the `allow-top-navigation` flag
came from the frame or the response header.
The original fix in this CL involved downgrading the
`allow-top-navigation` sandbox flag to
`allow-top-navigation-by-user-activation` if it detected that the page's
response headers were attempting to give it that extra permission. We
ultimately decided against it so that we wouldn't be setting a new
precedent of changing sandbox flags.
The fix we decided on:
Add a bit to the PolicyContainerPolicies struct to track if a document
can navigate the top-level frame without sticky user activation. If any
of the following conditions hold:
- The document is cross-origin to the root document, and the document's parent document is not allowed to navigate top without
sticky user activation
- The document is cross-origin to the root document, and the frame
hosting the document is either unsandboxed or sandboxed without the
allow-top-navigation flag
The bit will be set to false and the document will require sticky user
activation to navigate top.
This will end up having the same behavior as the downgrading approach,
without needing to have the browser change sandbox flags.
[1] https://chromium-review.googlesource.com/c/chromium/src/+/2688360
Bug: 1251790
Change-Id: I7406d631d6b9c4bdbfc71c433db50b2fca2f0c21
---
M content/browser/renderer_host/navigation_policy_container_builder.cc
M content/browser/renderer_host/navigation_policy_container_builder.h
M content/browser/renderer_host/navigation_policy_container_builder_browsertest.cc
M content/browser/renderer_host/navigation_policy_container_builder_unittest.cc
M content/browser/renderer_host/navigation_request.cc
M content/browser/renderer_host/policy_container_host.cc
M content/browser/renderer_host/policy_container_host.h
M content/browser/renderer_host/policy_container_host_browsertest.cc
M content/browser/renderer_host/policy_container_host_unittest.cc
M content/browser/renderer_host/render_frame_host_impl.cc
M content/renderer/policy_container_util.cc
M third_party/blink/public/mojom/frame/policy_container.mojom
M third_party/blink/public/platform/web_policy_container.h
M third_party/blink/renderer/core/frame/local_frame.cc
M third_party/blink/renderer/core/frame/policy_container.cc
M third_party/blink/renderer/core/frame/policy_container_test.cc
M third_party/blink/renderer/core/loader/frame_loader_test.cc
A third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-iframe-element/resources/sandbox-top-navigation-helper.js
A third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-iframe-element/sandbox-top-navigation-child-special-cases.tentative.sub.window.js
A third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-iframe-element/sandbox-top-navigation-child.tentative.sub.window.js
A third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-iframe-element/sandbox-top-navigation-escalate-privileges.tentative.sub.window.js
A third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-iframe-element/sandbox-top-navigation-grandchild.tentative.sub.window.js
D third_party/blink/web_tests/flag-specific/disable-site-isolation-trials/http/tests/security/frameNavigation/sandbox-DENIED-cross-origin-top-navigation-expected.txt
D third_party/blink/web_tests/flag-specific/disable-site-isolation-trials/http/tests/security/frameNavigation/sandbox-DENIED-cross-origin-top-navigation-nested-sandbox-expected.txt
D third_party/blink/web_tests/http/tests/security/frameNavigation/resources/cross-iframe-that-performs-top-navigation-in-nested-sandboxed-frame.html
D third_party/blink/web_tests/http/tests/security/frameNavigation/sandbox-ALLOWED-top-navigation-with-two-flags-expected.txt
D third_party/blink/web_tests/http/tests/security/frameNavigation/sandbox-ALLOWED-top-navigation-with-two-flags.html
D third_party/blink/web_tests/http/tests/security/frameNavigation/sandbox-ALLOWED-top-navigation-with-user-gesture-expected.txt
D third_party/blink/web_tests/http/tests/security/frameNavigation/sandbox-ALLOWED-top-navigation-with-user-gesture.html
D third_party/blink/web_tests/http/tests/security/frameNavigation/sandbox-DENIED-cross-origin-top-navigation-expected.txt
D third_party/blink/web_tests/http/tests/security/frameNavigation/sandbox-DENIED-cross-origin-top-navigation-nested-sandbox-expected.txt
D third_party/blink/web_tests/http/tests/security/frameNavigation/sandbox-DENIED-cross-origin-top-navigation-nested-sandbox.html
D third_party/blink/web_tests/http/tests/security/frameNavigation/sandbox-DENIED-cross-origin-top-navigation.html
33 files changed, 541 insertions(+), 337 deletions(-)
To view, visit change 3842458. To unsubscribe, or for help writing mail filters, visit settings.
Liam Brady (Gerrit)
Attention is currently required from: Antonio Sartori, Arthur Sonzogni, Josh Karlin, Liam Brady.
Liam Brady uploaded patch set #48 to this change.
Stop frames from allowing themselves to navigate top without gesture
A previous CL stopped a sandboxed iframe from navigating the top document if its
ancestor was not allowed to navigate top[1]. However, there is still a
corner case that allows a sandboxed frame to give itself permission to
navigate top if the top-level page doesn't have any sandbox flags set.
For this attack, a cross origin iframe is embedded in a page. At
creation time, both the main page and the iframe are unsandboxed, which
means that the iframe requires sticky user activation to be able to
navigate the top frame. Then, this iframe loads a page whose response
header's CSP includes `sandbox allow-top-navigation`. The cross-origin
page just gave itself permission to navigate the top-level frame
without sticky user activation.
This is problematic because it allows a cross-origin iframe to
circumvent our framebust intervention and navigate the top page to a
potentially malicious page. Before this CL, there was no way for the
renderer to detect this. It can't distinguish between sandbox flags set on the frame itself by the embedding page and sandbox flags delivered in the response headers. There are 2 checks in LocalFrame::CanNavigate that are looking at the 2 top navigation sandbox flags, but by that point the frame sandbox flags and delivered sandbox flags have been squashed into one place, so there's no way to know if the `allow-top-navigation` flag came from the frame or the response header.
This CL's original approach involved downgrading the allow-top-navigation sandbox flag to allow-top-navigation-by-user-activation specifically when the browser process detected that the document's response headers were attempting to give it that extra ability when the embedder-supplied sandbox flags did not. We ultimately decided against it so that we wouldn't be setting a new precedent of dynamically re-interpreting sandbox flags.
33 files changed, 530 insertions(+), 337 deletions(-)
To view, visit change 3842458. To unsubscribe, or for help writing mail filters, visit settings.
Liam Brady (Gerrit)
Attention is currently required from: Antonio Sartori, Arthur Sonzogni, Dominic Farolino, Josh Karlin.
10 comments:
Commit Message:
Patch Set #46, Line 7:
frames from allowing itselfbe consistent with singular vs plural
Done
the top document
Done
Patch Set #46, Line 23:
redirectnavigate
Done
Patch Set #46, Line 24:
CurrentlyShould this be "Before this CL"?
Done
The original fix in this CL involved downgrading the
`allow-top-navigation` sandbox flag to
`allow-top-navigation-by-user-activation` if it detected that the page's
response headers were attempting to give it that extra permission. We
ultimately decided against it so that we wouldn't be setting a new
precedent of changing sandbox flags.
nit: Slight rewording recommendation: […]
Done
I'm wondering if we can make these conditions clearer. […]
I think there are cases where #1 would be allowed to navigate the top frame. Using the example we discussed offline:
Top-level A
<iframe src=B sandbox="allow-top-navigation">
<iframe src=C> // Unsandboxed
</iframe>
</iframe>
The top level page (A) explicitly gave permission to a cross-origin frame (B) to navigate top and delegate permissions inside of its frame. Because (C) is unsandboxed, it will inherit its permissions from its parent and become a sandboxed frame. So I think this still works with the comment I wrote (with inheritance the frame becomes sandboxed with `allow-top-navigation` which means the intervention won't kick in).
File content/browser/renderer_host/navigation_policy_container_builder.cc:
Patch Set #46, Line 262:
laterLet's specifically mention `NavigationRequest::CommitNavigation()` here.
Done
File content/browser/renderer_host/navigation_request.cc:
Patch Set #37, Line 5004:
frame_tree_node()->current_frame_host()->GetLastCommittedOrigin()Just to be clear: this means that sandbox="allow-top-navigation" will give the iframe strictly more capabilities than cross-origin iframes, right?
That is correct. This means that we can still have our intervention in place but at the same time not break `allow-top-navigation`.
File content/browser/renderer_host/navigation_request.cc:
Patch Set #46, Line 5048:
// Same-origin navigations are always allowed to navigate top.Technically same-origin-to-top navigations are always allowed to navigate to top. […]
Done
File third_party/blink/renderer/core/frame/local_frame.cc:
Patch Set #46, Line 1771:
PolicyContainerPolicies::can_navigate_top_without_user_gesturewrap in backticks
Done
To view, visit change 3842458. To unsubscribe, or for help writing mail filters, visit settings.
Dominic Farolino (Gerrit)
Attention is currently required from: Antonio Sartori, Arthur Sonzogni, Josh Karlin, Liam Brady.
2 comments:
Commit Message:
The original fix in this CL involved downgrading the
`allow-top-navigation` sandbox flag to
`allow-top-navigation-by-user-activation` if it detected that the page's
response headers were attempting to give it that extra permission. We
ultimately decided against it so that we wouldn't be setting a new
precedent of changing sandbox flags.
Done
Looks like this is not done?
Patchset:
Please check the other comments marked as "Done" to see if they are really done
To view, visit change 3842458. To unsubscribe, or for help writing mail filters, visit settings.
Liam Brady (Gerrit)
Attention is currently required from: Antonio Sartori, Arthur Sonzogni, Josh Karlin, Liam Brady.
Liam Brady uploaded patch set #50 to this change.
Stop frames from allowing themselves to navigate top without gesture
A previous CL stopped a sandboxed iframe from navigating the top document if its ancestor was not allowed to navigate top[1]. However, there is still a corner case that allows a sandboxed frame to give itself permission to navigate top if the top-level page doesn't have any sandbox flags set.
For this attack, a cross origin iframe is embedded in a page. At
creation time, both the main page and the iframe are unsandboxed, which
means that the iframe requires sticky user activation to be able to
navigate the top frame. Then, this iframe loads a page whose response
header's CSP includes `sandbox allow-top-navigation`. The cross-origin
page just gave itself permission to navigate the top-level frame
without sticky user activation.
This is problematic because it allows a cross-origin iframe to
circumvent our framebust intervention and navigate the top page to a
potentially malicious page. Before this CL, there is no way for the renderer to detect this. It can't distinguish between sandbox flags set on the frame itself by the embedding page and sandbox flags delivered in the response headers. There are 2 checks in LocalFrame::CanNavigate that are looking at the 2 top navigation sandbox flags, but by that point the frame sandbox flags and delivered sandbox flags have been squashed into one place, so there's no way to know if the `allow-top-navigation` flag came from the frame or the response header.
33 files changed, 539 insertions(+), 337 deletions(-)
To view, visit change 3842458. To unsubscribe, or for help writing mail filters, visit settings.
Liam Brady (Gerrit)
Attention is currently required from: Antonio Sartori, Arthur Sonzogni, Dominic Farolino, Josh Karlin.
Patch set 50:Commit-Queue +1
1 comment:
Commit Message:
The original fix in this CL involved downgrading the
`allow-top-navigation` sandbox flag to
`allow-top-navigation-by-user-activation` if it detected that the page's
response headers were attempting to give it that extra permission. We
ultimately decided against it so that we wouldn't be setting a new
precedent of changing sandbox flags.
Looks like this is not done?
Oops I think I forgot to actually save the commit message. I went and re-added the ones from the other 4 comments back in.
To view, visit change 3842458. To unsubscribe, or for help writing mail filters, visit settings.
Liam Brady (Gerrit)
Attention is currently required from: Antonio Sartori, Arthur Sonzogni, Dominic Farolino, Josh Karlin.
1 comment:
File third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-iframe-element/cross-origin-top-nav-sandbox.tentative.sub.html:
Patch Set #24, Line 1:
<!DOCTYPE html>Since we're no longer trying to land this + the spec at the same time, should these tests be moved b […]
I'm going to keep the tests where they are, but keep them as `.tentative`.
To view, visit change 3842458. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
Attention is currently required from: Antonio Sartori, Dominic Farolino, Josh Karlin, Liam Brady.
Patch set 50:Code-Review +1
7 comments:
Commit Message:
Rewrap to fit 72 char:
```
Stop frames from allowing themselves to navigate top without gesture
A previous CL stopped a sandboxed iframe from navigating the top
document if its ancestor was not allowed to navigate top[1]. However,
there is still a corner case that allows a sandboxed frame to give
itself permission to navigate top if the top-level page doesn't have any
sandbox flags set.For this attack, a cross origin iframe is embedded in a page. At
creation time, both the main page and the iframe are unsandboxed, which
means that the iframe requires sticky user activation to be able to
navigate the top frame. Then, this iframe loads a page whose response
header's CSP includes `sandbox allow-top-navigation`. The cross-origin
page just gave itself permission to navigate the top-level frame without
sticky user activation.This is problematic because it allows a cross-origin iframe to
circumvent our framebust intervention and navigate the top page to a
potentially malicious page. Before this CL, there is no way for the
renderer to detect this. It can't distinguish between sandbox flags set
on the frame itself by the embedding page and sandbox flags delivered in
the response headers. There are 2 checks in LocalFrame::CanNavigate that
are looking at the 2 top navigation sandbox flags, but by that point the
frame sandbox flags and delivered sandbox flags have been squashed into
one place, so there's no way to know if the `allow-top-navigation` flag
came from the frame or the response header.
This CL's original approach involved downgrading the
allow-top-navigation sandbox flag to
allow-top-navigation-by-user-activation specifically when the browser
process detected that the document's response headers were attempting to
give it that extra ability when the embedder-supplied sandbox flags did
not. We ultimately decided against it so that we wouldn't be setting a
new precedent of dynamically re-interpreting sandbox flags.
The fix we decided on: Add a bit to the PolicyContainerPolicies struct
to track if a document can navigate the top-level frame without sticky
user activation. If any of the following conditions hold:
- The document is cross-origin to the root document, and the document's
parent document is not allowed to navigate top without sticky user
activation
- The document is cross-origin to the root document, and the frame
hosting the document is either unsandboxed or sandboxed without the
allow-top-navigation flag The bit will be set to false and the
document will require sticky user activation to navigate top.This will end up having the same behavior as the downgrading approach,
without needing to have the browser change sandbox flags.[1] https://chromium-review.googlesource.com/c/chromium/src/+/2688360
```
Patchset:
Still LGTM % some minor nits below
File content/browser/renderer_host/navigation_request.cc:
Patch Set #37, Line 5004:
frame_tree_node()->current_frame_host()->GetLastCommittedOrigin()
> b. […]
Yes, this comment can be resolved.
File content/browser/renderer_host/navigation_request.cc:
// Determine if top-level navigation is allowed without sticky user
// activation. This is used to fix the exploit in (crbug.com/1251790).
// If a child document is cross-origin with its parent, it loses its
// ability to navigate top without user gesture. One notable exception is
// made if its parent embeds it using sandbox="allow-top-navigation".
// Please note this is quite unusual, because it means using sandbox
// brings new capabilities, as opposed to new restrictions.nit: wrap to fit 80.
I also added https:// to the link to make it clickable.
```
// Determine if top-level navigation is allowed without sticky user
// activation. This is used to fix the exploit in https://crbug.com/1251790.
// If a child document is cross-origin with its parent, it loses its ability
// to navigate top without user gesture. One notable exception is made if its
// parent embeds it using sandbox="allow-top-navigation". Please note this is
// quite unusual, because it means using sandbox brings new capabilities, as
// opposed to new restrictions.
```
File third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-iframe-element/resources/sandbox-top-navigation-helper.js:
nit: rm empty line
async function createNestedIframe(parent, origin, frame_sandbox, header_sandbox) {
return parent.addIframe(
/*extraConfig=*/ {
origin: origin,
scripts: ['/resources/testdriver.js', '/resources/testdriver-driver.js', '/resources/testdriver-vendor.js'],
headers: header_sandbox ? [["Content-Security-Policy", "sandbox allow-scripts " + header_sandbox]] : [],
},
/*attributes=*/ frame_sandbox ? {sandbox: "allow-scripts " + frame_sandbox} : {},
)
}nit: I had difficulty reading it. Maybe we can wrap it below 80 char wide and use variables.
Suggestion:
```
async function createNestedIframe(parent, origin, frame_sandbox, header_sandbox)
{
let headers = [];
if (header_sandbox) {
headers.push([
"Content-Security-Policy",
"sandbox allow-scripts " + header_sandbox
]);
}
let iframe_attributes = {};
if (frame_sandbox) {
iframe_attributes.sandbox = "allow-scripts " + frame_sandbox;
}return parent.addIframe({
origin: origin,
scripts: [
'/resources/testdriver.js',
'/resources/testdriver-driver.js',
'/resources/testdriver-vendor.js'
],
headers: headers,
}, iframe_attributes);
}
```Patch Set #50, Line 30:
await iframe.executeScript(() => window.top.location.href = "https://google.com");nit: wrap to 80
```
await iframe.executeScript(() => {
window.top.location.href = "https://google.com";
});
```
To view, visit change 3842458. To unsubscribe, or for help writing mail filters, visit settings.
Dominic Farolino (Gerrit)
Attention is currently required from: Antonio Sartori, Josh Karlin, Liam Brady.
2 comments:
Commit Message:
I think there are cases where #1 would be allowed to navigate the top frame. […]
Is that the behavior we want though? Our original intervention is specifically targeted at cross-origin iframes, so it feels weird that C automatically gets to navigate `top` without user activation, and the only way for B to prevent this is to explicitly sandbox C without the `allow-top-navigation` flag.
I could be convinced this is OK, but let's check with Arthur first.
Commit Message:
Patch Set #50, Line 9:
ancestorDo you mean "ancestors were not allowed", or "parent was not allowed". Again let's be consistent with singular/plural. Only reason I'm being picky about this is because I think the previous version of CanNavigateHelper was recursive, whereas this CL is making the change to only look at the parent frame (IIUC).
To view, visit change 3842458. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
Attention is currently required from: Antonio Sartori, Dominic Farolino, Josh Karlin, Liam Brady.
1 comment:
Commit Message:
Is that the behavior we want though? Our original intervention is specifically targeted at cross-ori […]
I agree there is no need to prevent same-origin-to-top child from navigating
top, because if we did, it would be trivially bypassable, because it has an
handle to top and can execute script on its behalf. For instance:
```
window.top.location = "XXX";
```
```
window.top.eval("location.href = XXX;")
```
```
window.top.document.write("<script>location.href = XXX;</scr"+"ipt>");
```
To view, visit change 3842458. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
Attention is currently required from: Antonio Sartori, Arthur Sonzogni, Dominic Farolino, Josh Karlin, Liam Brady.
1 comment:
Commit Message:
I agree there is no need to prevent same-origin-to-top child from navigating […]
(Sorry, I was off topic).
I think this is okay. It matches the current behavior.
`A` lets its cross-origin iframe to trigger navigations to top. The `<iframe>` appears to `A` as a black box. It doesn't really matter if `B` was initiating navigation directly or it did via `C`.
Also, `B` has a way to restrict `C` by removing the "allow-top-navigation".
To view, visit change 3842458. To unsubscribe, or for help writing mail filters, visit settings.
Liam Brady (Gerrit)
Attention is currently required from: Antonio Sartori, Dominic Farolino, Josh Karlin.
6 comments:
Commit Message:
Patch Set #50, Line 9:
ancestorDo you mean "ancestors were not allowed", or "parent was not allowed". […]
Done
Rewrap to fit 72 char: […]
Done
File content/browser/renderer_host/navigation_request.cc:
// Determine if top-level navigation is allowed without sticky user
// activation. This is used to fix the exploit in (crbug.com/1251790).
// If a child document is cross-origin with its parent, it loses its
// ability to navigate top without user gesture. One notable exception is
// made if its parent embeds it using sandbox="allow-top-navigation".
// Please note this is quite unusual, because it means using sandbox
// brings new capabilities, as opposed to new restrictions.
nit: wrap to fit 80. […]
Done
File third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-iframe-element/resources/sandbox-top-navigation-helper.js:
nit: rm empty line
Done
async function createNestedIframe(parent, origin, frame_sandbox, header_sandbox) {
return parent.addIframe(
/*extraConfig=*/ {
origin: origin,
scripts: ['/resources/testdriver.js', '/resources/testdriver-driver.js', '/resources/testdriver-vendor.js'],
headers: header_sandbox ? [["Content-Security-Policy", "sandbox allow-scripts " + header_sandbox]] : [],
},
/*attributes=*/ frame_sandbox ? {sandbox: "allow-scripts " + frame_sandbox} : {},
)
}
nit: I had difficulty reading it. Maybe we can wrap it below 80 char wide and use variables. […]
Done
Patch Set #50, Line 30:
await iframe.executeScript(() => window.top.location.href = "https://google.com");
nit: wrap to 80 […]
Done
To view, visit change 3842458. To unsubscribe, or for help writing mail filters, visit settings.
Dominic Farolino (Gerrit)
Attention is currently required from: Antonio Sartori, Josh Karlin, Liam Brady.
Patch set 51:Code-Review +1
Liam Brady (Gerrit)
Attention is currently required from: Antonio Sartori, Josh Karlin, Liam Brady.
Patch set 51:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Change-Id: I7406d631d6b9c4bdbfc71c433db50b2fca2f0c21
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3842458
Reviewed-by: Arthur Sonzogni <arthurs...@chromium.org>
Reviewed-by: Dominic Farolino <d...@chromium.org>
Commit-Queue: Liam Brady <lbr...@google.com>
Reviewed-by: Mike West <mk...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1057526}
33 files changed, 559 insertions(+), 339 deletions(-)
Blink W3C Test Autoroller (Gerrit)
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/35697