Generate origins from nonces for sandboxed frames in browser process [chromium/src : main]
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 4 comments![]( "Open in Gerrit")
if (new_frame_should_be_sandboxed) {Can you please protect this (and other paths if needed) behind a killswitch flag so that we can turn it off easily in case there are some bugs found in the wild?
} else {
// For non-sandboxed windows, derive a new opaque origin from creator.
sandbox_new_window_origin =
sandbox_new_window_origin.DeriveNewOpaqueOrigin();What if we don't send an origin for the non-sandboxed case? Also why is the non-sandboxed case also opaque?
mojo_base.mojom.UnguessableToken? sandbox_origin_token;Let's add some comments here since this one isn't needed for routing
Related details
- Liang Zhao
- Monica Chintala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Monica Chintala (Gerrit)
Monica Chintala voted and added 3 comments![]( "Open in Gerrit")
Votes added by Monica Chintala
| Commit-Queue | +1 |
3 comments
if (new_frame_should_be_sandboxed) {Can you please protect this (and other paths if needed) behind a killswitch flag so that we can turn it off easily in case there are some bugs found in the wild?
Added a feature flag kUseSandboxTokenForOriginDerivation in blink that to be used here and in DocumentLoader to handle any regressions.
} else {
// For non-sandboxed windows, derive a new opaque origin from creator.
sandbox_new_window_origin =
sandbox_new_window_origin.DeriveNewOpaqueOrigin();What if we don't send an origin for the non-sandboxed case? Also why is the non-sandboxed case also opaque?
Good question, added in the comment section on its usage at sandbox_origin_token in CreateNewWindow.
// Sandbox origin token handling for popups:
// Normally sandboxed popups token is pre-generated by
// SetOriginDependentStateOfNewFrame(). Popups that escape sandbox (CSP
// "sandbox" + "allow-popups-to-escape-sandbox"):
// - Browser sees no sandbox flags, so GetSandboxOriginToken() is empty
// - Renderer still inherits CSP sandbox flags and will create opaque origin
// - So generate a new token here as fallback
mojo_base.mojom.UnguessableToken? sandbox_origin_token;Let's add some comments here since this one isn't needed for routing
- Liang Zhao
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 2 comments![]( "Open in Gerrit")
if (new_frame_should_be_sandboxed) {Monica ChintalaCan you please protect this (and other paths if needed) behind a killswitch flag so that we can turn it off easily in case there are some bugs found in the wild?
Added a feature flag kUseSandboxTokenForOriginDerivation in blink that to be used here and in DocumentLoader to handle any regressions.
Acknowledged
} else {
// For non-sandboxed windows, derive a new opaque origin from creator.
sandbox_new_window_origin =
sandbox_new_window_origin.DeriveNewOpaqueOrigin();Monica ChintalaWhat if we don't send an origin for the non-sandboxed case? Also why is the non-sandboxed case also opaque?
Good question, added in the comment section on its usage at sandbox_origin_token in CreateNewWindow.
// Sandbox origin token handling for popups:
// Normally sandboxed popups token is pre-generated by
// SetOriginDependentStateOfNewFrame(). Popups that escape sandbox (CSP
// "sandbox" + "allow-popups-to-escape-sandbox"):
// - Browser sees no sandbox flags, so GetSandboxOriginToken() is empty
// - Renderer still inherits CSP sandbox flags and will create opaque origin
// - So generate a new token here as fallback
Hmm that is an interesting case of browser <> renderer discrepancy. Is there a test that covers this? Does the browser-side calculated origin differ than the renderer-side origin in that case then?
Related details
- Liang Zhao
- Monica Chintala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Monica Chintala (Gerrit)
Monica Chintala added 1 comment![]( "Open in Gerrit")
} else {
// For non-sandboxed windows, derive a new opaque origin from creator.
sandbox_new_window_origin =
sandbox_new_window_origin.DeriveNewOpaqueOrigin();Monica ChintalaWhat if we don't send an origin for the non-sandboxed case? Also why is the non-sandboxed case also opaque?
Rakina Zata AmniGood question, added in the comment section on its usage at sandbox_origin_token in CreateNewWindow.
// Sandbox origin token handling for popups:
// Normally sandboxed popups token is pre-generated by
// SetOriginDependentStateOfNewFrame(). Popups that escape sandbox (CSP
// "sandbox" + "allow-popups-to-escape-sandbox"):
// - Browser sees no sandbox flags, so GetSandboxOriginToken() is empty
// - Renderer still inherits CSP sandbox flags and will create opaque origin
// - So generate a new token here as fallback
Hmm that is an interesting case of browser <> renderer discrepancy. Is there a test that covers this? Does the browser-side calculated origin differ than the renderer-side origin in that case then?
OpenSandboxedDocumentInUnsandboxedPopupFromCSPSandboxedDocument is the test that covers this exact scenario, and yes, the browser-side origin will differ from the renderer-side origin in that case. The browser computes a non-opaque origin while the renderer creates an opaque one from the inherited CSP.
// Frame: not sandboxed
EXPECT_EQ(kNone, foo_root->effective_frame_policy().sandbox_flags);
// Document: sandboxed (from inherited CSP)
EXPECT_EQ(expected_flags, foo_root->current_frame_host()->active_sandbox_flags());
OpenUnsandboxedPopupFromSandboxedFrame and OpenSandboxedDocumentInUnsandboxedPopupFromCSPSandboxedDocument are the other related tests
Related details
- Liang Zhao
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Monica Chintala (Gerrit)
Monica Chintala added 1 comment![]( "Open in Gerrit")
chrome-ipc-reviews@ Can I get your review on mojom file changes when you get a chance, thanks!
kbr@ Can I get the review on feature files, thanks!
Related details
- Chromium IPC Reviews
- Kenneth Russell
- Liang Zhao
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: mpde...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): mpde...@chromium.org
Reviewer source(s):
mpde...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Kenneth Russell
- Liang Zhao
- Matthew Denton
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenneth Russell (Gerrit)
Kenneth Russell voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kenneth Russell
| Code-Review | +1 |
1 comment
`third_party/blink/public/common/features.{cc,h}` lgtm
Related details
- Liang Zhao
- Matthew Denton
- Monica Chintala
- Rakina Zata Amni
Matthew Denton (Gerrit)
Matthew Denton voted Code-Review+1![]( "Open in Gerrit")
- Kenneth Russell
- Liang Zhao
- Monica Chintala
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 5 comments![]( "Open in Gerrit")
Thanks! I think we're really close now, just some suggestions
const base::UnguessableToken* GetSandboxOriginToken() {
return sandbox_origin_token_.get();
}
void ResetSandboxOriginToken() { sandbox_origin_token_.reset(); }Is it possible to just take the `sandbox_origin_token_` / move the unique_ptr instead of doing it in two steps like this?
Oh I think there's a bug in `SetOriginDependentStateOfNewFrame()` actually! It should check the `active_sandbox_flags()` (which would be the document's actual sandbox state) on the RFH instead of the BrowsingContexState (which seems to only contain the "frame sandbox"). I made a quick fix for that and it seems to work when applied to your patch: crrev.com/c/7596676.
I think that needs to land separately though, and I don't want to block your CL on that if there turns out to be some discussions needed, so can you just file a bug for this and cc arthursonzogni@ and pmeuleman@ who were involved in some changes around that case (crrev.com/c/3643878)? And then please add a TODO to remove the special case.
if (local_frame_params->policy_container &&
(local_frame_params->policy_container->policies->sandbox_flags &
network::mojom::WebSandboxFlags::kOrigin) !=
network::mojom::WebSandboxFlags::kNone) {Do you need the check if you already nullcheck below?
std::unique_ptr<base::UnguessableToken> sandbox_origin_token = nullptr);This and the FrameTokens constructor only have one caller, so it seems like it doesn't need default values?
Related details
- Kenneth Russell
- Liang Zhao
- Monica Chintala