Fix draggable region validation bypass in GuestViewBase [chromium/src : main]
Zoraiz Naeem (Gerrit)
Zoraiz Naeem added 1 comment![]( "Open in Gerrit")
jamescook@ PTAL at guest_view_base.cc and app_window.cc
Related details
- James Cook
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook added 2 comments![]( "Open in Gerrit")
One question. Also, your trybots are red. :-)
if (contents != web_contents()) {Can you explain in more detail, either here in a comment or in the CL description, why this mismatch is allowed? Or rather, why it has to be logged as an error rather than CHECKed? It not being a CHECK makes it seem like it's expected, but LOG(ERROR) implies that it's not.
Related details
- Zoraiz Naeem
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. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem voted and added 2 comments![]( "Open in Gerrit")
Votes added by Zoraiz Naeem
| Commit-Queue | +1 |
2 comments
if (contents != web_contents()) {Can you explain in more detail, either here in a comment or in the CL description, why this mismatch is allowed? Or rather, why it has to be logged as an error rather than CHECKed? It not being a CHECK makes it seem like it's expected, but LOG(ERROR) implies that it's not.
Thanks for pointing it out. I just released that by default, we do not collect draggable regions. Chrome only enables collecting draggable regions for certain cases and even in those cases, it only does for the main web-contents. [1]
For other cases like Chrome Apps, we explicitly enable draggable regions via `contents::WebContents::SetSupportsDraggableRegions)`. So in cases of chrome apps etc, we can keep the check.
It only for GLiC that we explicitly enable draggable regions for its `webview`.
Related details
- James Cook
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. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem added 1 comment![]( "Open in Gerrit")
if (contents != web_contents()) {Zoraiz NaeemCan you explain in more detail, either here in a comment or in the CL description, why this mismatch is allowed? Or rather, why it has to be logged as an error rather than CHECKed? It not being a CHECK makes it seem like it's expected, but LOG(ERROR) implies that it's not.
Thanks for pointing it out. I just released that by default, we do not collect draggable regions. Chrome only enables collecting draggable regions for certain cases and even in those cases, it only does for the main web-contents. [1]
For other cases like Chrome Apps, we explicitly enable draggable regions via `contents::WebContents::SetSupportsDraggableRegions)`. So in cases of chrome apps etc, we can keep the check.
It only for GLiC that we explicitly enable draggable regions for its `webview`.
Anyways, the crux of the fix, passing the correct draggable region in GuestViewBase.
James Cook (Gerrit)
James Cook voted and added 1 comment![]( "Open in Gerrit")
Votes added by James Cook
| Code-Review | +1 |
1 comment
- Zoraiz Naeem
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. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem added 1 comment![]( "Open in Gerrit")
- Ian Wells
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. |
Ian Wells (Gerrit)
Ian Wells voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ian Wells
| Code-Review | +1 |
1 comment
const bool is_webview_contents = web_contents() != contents;Nit, please put this definition closer to the SetDraggableRegion call where it is used
Related details
- Zoraiz Naeem
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. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem voted and added 2 comments![]( "Open in Gerrit")
Votes added by Zoraiz Naeem
| Commit-Queue | +2 |
2 comments
const bool is_webview_contents = web_contents() != contents;Nit, please put this definition closer to the SetDraggableRegion call where it is used
Done
if (contents != web_contents()) {Zoraiz NaeemCan you explain in more detail, either here in a comment or in the CL description, why this mismatch is allowed? Or rather, why it has to be logged as an error rather than CHECKed? It not being a CHECK makes it seem like it's expected, but LOG(ERROR) implies that it's not.
Zoraiz NaeemThanks for pointing it out. I just released that by default, we do not collect draggable regions. Chrome only enables collecting draggable regions for certain cases and even in those cases, it only does for the main web-contents. [1]
For other cases like Chrome Apps, we explicitly enable draggable regions via `contents::WebContents::SetSupportsDraggableRegions)`. So in cases of chrome apps etc, we can keep the check.
It only for GLiC that we explicitly enable draggable regions for its `webview`.
Anyways, the crux of the fix, passing the correct draggable region in GuestViewBase.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem added 1 comment![]( "Open in Gerrit")
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
7 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/glic/widget/glic_view.cc
Insertions: 5, Deletions: 5.
@@ -101,11 +101,6 @@
void GlicView::DraggableRegionsChanged(
const std::vector<blink::mojom::DraggableRegionPtr>& regions,
content::WebContents* contents) {
- // `GlicView::DraggableRegionsChanged()` is called when draggable regions for
- // either the main-webcontents or guest-webcontents are changed.
- // guest-webcontents are the webcontents associated to `<webview>` hosting the
- // glic web app,
- const bool is_webview_contents = web_contents() != contents;
SkRegion sk_region;
for (const auto& region : regions) {
sk_region.op(
@@ -114,6 +109,11 @@
region->draggable ? SkRegion::kUnion_Op : SkRegion::kDifference_Op);
}
+ // `GlicView::DraggableRegionsChanged()` is called when draggable regions for
+ // either the main-webcontents or guest-webcontents are changed.
+ // guest-webcontents are the webcontents associated to `<webview>` hosting the
+ // glic web app,
+ const bool is_webview_contents = web_contents() != contents;
SetDraggableRegion(sk_region, /*for_webview=*/is_webview_contents);
}
```
Change information
Fix draggable region validation bypass in GuestViewBase
GuestViewBase::DraggableRegionsChanged was incorrectly substituting the
embedder's WebContents for the source WebContents when forwarding the
event to the embedder delegate. This laundering bypassed source
validation checks in downstream delegates like AppWindow.
This CL fixes the vulnerability by passing the original guest
WebContents to the embedder delegate.
- M chrome/browser/glic/widget/glic_view.cc
- M chrome/browser/glic/widget/glic_view.h
- M components/guest_view/browser/guest_view_base.cc
Code-Review: +1 by Ian Wells, +1 by James Cook
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |