[OOPIF PDF Viewer] Allow PDF resource requests for PDF navigation [chromium/src : main]
Andy Phan (Gerrit)
Attention is currently required from: Devlin Cronin.
Andy Phan would like Devlin Cronin to review this change.
[OOPIF PDF Viewer] Allow PDF resource requests for PDF navigation
Allow PDF extension resources to be requested only for the frames in the
PDF Viewer architecture. The frames that should be able to access the
resources are the extension frame and the content frame.
In ResourceRequestPolicy::CanRequestResource(), if the top frame has the
same origin as the resource, then the resource can be requested. This is
the case for MimeHandlerViewGuest PDF Viewer, as the PDF extension is in
an inner WebContents.
For OOPIF PDF Viewer, there is only one WebContents, so the top frame's
origin is not the same. Loosen the conditions to allow PDF resource
requests for OOPIF PDF Viewer.
Bug: 1445746
Change-Id: I05a5273d5842d35a70a08233761fb1416d97effb
---
M chrome/renderer/BUILD.gn
M chrome/renderer/extensions/DEPS
M chrome/renderer/extensions/resource_request_policy.cc
3 files changed, 27 insertions(+), 0 deletions(-)
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Devlin Cronin.
1 comment:
Patchset:
Hi Devlin, this CL is part of the OOPIF PDF Viewer project. Our previous discussion was here: https://groups.google.com/a/google.com/g/chrome-extensions-team-core/c/311XeU7V1rE.
I'm not sure if the checks in the current patchset are narrow enough to allow OOPIF PDF Viewer only, or if it needs to be stricter, such as the suggestion from Lei.
PTAL, thanks!
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Lei Zhang (Gerrit)
Attention is currently required from: Andy Phan, Devlin Cronin.
2 comments:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {Can a regular renderer that's on about:blank abuse this logic to request PDF extension resources?
Patch Set #6, Line 144:
ParentDoes `frame` always have a parent?
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Devlin Cronin, Lei Zhang.
1 comment:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {Can a regular renderer that's on about:blank abuse this logic to request PDF extension resources?
I'm not sure of the threat model here; lines 109, 119, and 129 all check the frame_url too, so I'd imagine that those could be abused, too, in addition to about:blank?
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Lei Zhang (Gerrit)
Attention is currently required from: Andy Phan, Devlin Cronin.
1 comment:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {
I'm not sure of the threat model here; lines 109, 119, and 129 all check the frame_url too, so I'd i […]
I don't understand your definition of "abused".
I'm just looking at this particular check and wondering about the behavior on about:blank. Can about:blank currently load PDF extension resources without this CL? e.g. With Devtools. Can about:blank load PDF extension resources with this CL? It's a straightforward question and I'm not implying there is anything unusual happening aside from the use of Devtools.
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Devlin Cronin, Lei Zhang.
1 comment:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {
I don't understand your definition of "abused". […]
Sorry, I misunderstood the question.
I tested this by navigating to about:blank using the omnibox, opening DevTools, and running in the console: `fetch('chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/index.css')`.
Without this CL, CanRequestResource() returns false, so the resource request is denied.
With this CL, CanRequestResource() returns true, but there's still an error on the request: `net::ERR_BLOCKED_BY_CLIENT`.
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Lei Zhang (Gerrit)
Attention is currently required from: Andy Phan, Devlin Cronin.
1 comment:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {
Sorry, I misunderstood the question. […]
Is that because this code is out of sync with AllowExtensionResourceLoad()? See the comment on line 77.
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Devlin Cronin, Lei Zhang.
1 comment:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {
Is that because this code is out of sync with AllowExtensionResourceLoad()? See the comment on line […]
Yes, with the CL, CanRequestResource() returning true but still having an error is because of https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/extension_protocols.cc;l=304.
Interestingly, without the CL, CanRequestResource() returns false, but AllowExtensionResourceLoad() returns true.
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Lei Zhang (Gerrit)
Attention is currently required from: Andy Phan, Devlin Cronin.
1 comment:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {
Yes, with the CL, CanRequestResource() returning true but still having an error is because of https: […]
I'm surprised to hear AllowExtensionResourceLoad() can return false with this CL, but return true without this CL. Even though this CL does not touch AllowExtensionResourceLoad().
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: David Bertoni, Devlin Cronin.
Andy Phan would like David Bertoni to review this change.
[OOPIF PDF Viewer] Allow PDF resource requests for PDF navigation
Allow PDF extension resources to be requested only for the frames in the
PDF Viewer architecture. The frames that should be able to access the
resources are the extension frame and the content frame.
In ResourceRequestPolicy::CanRequestResource(), if the top frame has the
same origin as the resource, then the resource can be requested. This is
the case for MimeHandlerViewGuest PDF Viewer, as the PDF extension is in
an inner WebContents.
For OOPIF PDF Viewer, there is only one WebContents, so the top frame's
origin is not the same. Loosen the conditions to allow PDF resource
requests for OOPIF PDF Viewer.
Bug: 1445746
Change-Id: I05a5273d5842d35a70a08233761fb1416d97effb
---
M chrome/renderer/BUILD.gn
M chrome/renderer/extensions/DEPS
M chrome/renderer/extensions/resource_request_policy.cc
3 files changed, 27 insertions(+), 0 deletions(-)
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: David Bertoni, Devlin Cronin.
1 comment:
Patchset:
+dbertoni@, as Devlin is effectively OOO. Please see the first comment for context on this CL. Thanks!
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: David Bertoni, Devlin Cronin.
Andy Phan has uploaded this change for review.
[OOPIF PDF Viewer] Allow PDF resource requests for PDF navigation
Allow PDF extension resources to be requested only for the frames in the
PDF Viewer architecture. The frames that should be able to access the
resources are the extension frame and the content frame.
In ResourceRequestPolicy::CanRequestResource(), if the top frame has the
same origin as the resource, then the resource can be requested. This is
the case for MimeHandlerViewGuest PDF Viewer, as the PDF extension is in
an inner WebContents.
For OOPIF PDF Viewer, there is only one WebContents, so the top frame's
origin is not the same. Loosen the conditions to allow PDF resource
requests for OOPIF PDF Viewer.
Bug: 1445746
Change-Id: I05a5273d5842d35a70a08233761fb1416d97effb
---
M chrome/renderer/BUILD.gn
M chrome/renderer/extensions/DEPS
M chrome/renderer/extensions/resource_request_policy.cc
3 files changed, 27 insertions(+), 0 deletions(-)
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
David Bertoni (Gerrit)
Attention is currently required from: Andy Phan, Devlin Cronin, Lei Zhang.
1 comment:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {
I'm surprised to hear AllowExtensionResourceLoad() can return false with this CL, but return true wi […]
Has this thread been resolved? I don't want to +1 this until everyone's OK.
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: David Bertoni, Devlin Cronin.
1 comment:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {
Has this thread been resolved? I don't want to +1 this until everyone's OK.
Not yet. I'm awaiting an opinion from someone either from the extensions team or the security team about this.
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Lei Zhang (Gerrit)
Attention is currently required from: Andy Phan, David Bertoni, Devlin Cronin.
1 comment:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {
Not yet. […]
Have you made it clear to extensions/security folks that you want their opinion? e.g. Are you waiting for dbertoni@ to give their thoughts on whether about:blank should be able to access PDF extension resources?
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: David Bertoni, Devlin Cronin, Lei Zhang.
1 comment:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {
Have you made it clear to extensions/security folks that you want their opinion? e.g. […]
I'll make it clearer.
+dbertoni@, are you familiar with this ResourceRequestPolicy::CanRequestResource()? WDYT about this line allowing about:blank to access extension resources? It doesn't actually allow it, since there are browser-side checks that disallow it, but we're not sure if those have to be in complete sync. There's an existing case mentioned in the thread where it goes out of sync.
Empty URLs, chrome:// schemes and other schemes allow extension resources, but I'd assume those are locked down more heavily than about:blank.
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
David Bertoni (Gerrit)
Attention is currently required from: Andy Phan, Devlin Cronin, Lei Zhang.
1 comment:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {
I'll make it clearer. […]
I don't have much experience in this area, so I hesitate to offer an opinion. There are certainly lots of cases where there are renderer-side checks that are duplicated in the browser, given that renderers can be compromised.
I'm somewhat worried about the inconsistency here. This is something Devlin should probably weigh in on.
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Lei Zhang (Gerrit)
Attention is currently required from: Andy Phan, David Bertoni, Devlin Cronin.
1 comment:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {
I don't have much experience in this area, so I hesitate to offer an opinion. […]
For this particular navigation where `frame_url` is about:blank, I see `initiator_origin` is chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai and so is `frame_origin`. I wonder if it makes sense to take those into account when making this decision.
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Devlin Cronin, Łukasz Anforowicz.
Andy Phan has uploaded this change for review.
[OOPIF PDF Viewer] Allow PDF resource requests for PDF navigation
Allow PDF extension resources to be requested only for the frames in the
PDF Viewer architecture. The frames that should be able to access the
resources are the extension frame and the content frame.
In ResourceRequestPolicy::CanRequestResource(), if the top frame has the
same origin as the resource, then the resource can be requested. This is
the case for MimeHandlerViewGuest PDF Viewer, as the PDF extension is in
an inner WebContents.
For OOPIF PDF Viewer, there is only one WebContents, so the top frame's
origin is not the same. Loosen the conditions to allow PDF resource
requests for OOPIF PDF Viewer.
Bug: 1445746
Change-Id: I05a5273d5842d35a70a08233761fb1416d97effb
---
M chrome/renderer/BUILD.gn
M chrome/renderer/extensions/DEPS
M chrome/renderer/extensions/resource_request_policy.cc
3 files changed, 27 insertions(+), 0 deletions(-)
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Devlin Cronin, Łukasz Anforowicz.
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {
For this particular navigation where `frame_url` is about:blank, I see `initiator_origin` is chrome- […]
+lukasza@ from the Security Team, do you have any thoughts about the discussion above? Thanks!
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Attention is currently required from: Andy Phan, Devlin Cronin.
2 comments:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {
+lukasza@ from the Security Team, do you have any thoughts about the discussion above? Thanks!
This is //chrome/**renderer**/extensions/... - overall security ultimately depends on checks that should be happening in the browser process (e.g. in //extensions/browser/extension_protocols.cc or //extensions/browser/extension_navigation_throttle.cc for navigations). In other words, having inconsistency between the renderer and browser checks is a bit undesirable, but shouldn’t have a security impact. OTOH, having correct/matching renderer checks can 1) provide better user experience (e.g. more details about the failure) and/or 2) avoid renderer kills when the browser receives an IPC that it thinks the renderer should never send. Still, I would defer to the Extensions Team for whether a discrepancy between browser and renderer checks is okay here or not.
That said, I agree with the recommendation to compare the `initiator_origin` with `kPdfExtensionId` (which would directly attribute the request to the PDF extension) instead of comparing `frame_url` with “about:blank” (which is a broad heuristic that is likely to catch more than just the PDF extension).
Patch Set #6, Line 144:
ParentDoes `frame` always have a parent?
+1
Is it possible that the current CL may result in null dereference and a crash if `frame->Parent()` is null? If so, it seems that it might be desirable to add test coverage that would catch such a crash?
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Andy Phan.
Andy Phan uploaded patch set #7 to this change.
[OOPIF PDF Viewer] Allow PDF resource requests for PDF navigation
Allow PDF extension resources to be requested only for the frames in the
PDF Viewer architecture. The frames that should be able to access the
resources are the extension frame and the content frame.
In ResourceRequestPolicy::CanRequestResource(), if the top frame has the
same origin as the resource, then the resource can be requested. This is
the case for MimeHandlerViewGuest PDF Viewer, as the PDF extension is in
an inner WebContents.
For OOPIF PDF Viewer, there is only one WebContents, so the top frame's
origin is not the same. Loosen the conditions to allow PDF resource
requests for OOPIF PDF Viewer.
Bug: 1445746
Change-Id: I05a5273d5842d35a70a08233761fb1416d97effb
---
M chrome/renderer/BUILD.gn
M chrome/renderer/extensions/DEPS
M chrome/renderer/extensions/resource_request_policy.cc
3 files changed, 30 insertions(+), 0 deletions(-)
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Andy Phan.
Andy Phan uploaded patch set #8 to this change.
[OOPIF PDF] Allow PDF resource requests for PDF navigation
Allow PDF extension resources to be requested only for the frames in the
PDF Viewer architecture. The frames that should be able to access the
resources are the extension frame and the content frame.
In ResourceRequestPolicy::CanRequestResource(), if the top frame has the
same origin as the resource, then the resource can be requested. This is
the case for MimeHandlerViewGuest PDF Viewer, as the PDF extension is in
an inner WebContents.
For OOPIF PDF Viewer, there is only one WebContents, so the top frame's
origin is not the same. Loosen the conditions to allow PDF resource
requests for OOPIF PDF Viewer.
Bug: 1445746
Change-Id: I05a5273d5842d35a70a08233761fb1416d97effb
---
M chrome/renderer/BUILD.gn
M chrome/renderer/extensions/DEPS
M chrome/renderer/extensions/resource_request_policy.cc
3 files changed, 30 insertions(+), 0 deletions(-)
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Andy Phan.
Andy Phan uploaded patch set #9 to this change.
Lei Zhang (Gerrit)
Attention is currently required from: Andy Phan, David Bertoni, Łukasz Anforowicz.
1 comment:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {
This is //chrome/**renderer**/extensions/... […]
I would suggest checking for PAGE_TRANSITION_AUTO_SUBFRAME instead of for about:blank, since that's the only valid possibility. The check for about:blank shouldn't be needed in the conditional, but maybe keep it inside the conditional block in a CHECK() as a sanity check.
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Lei Zhang (Gerrit)
Attention is currently required from: Andy Phan, David Bertoni, Łukasz Anforowicz.
2 comments:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #9, Line 134:
extension_origin.host() == extension_misc::kPdfExtensionId) {Check the scheme too.
Patch Set #9, Line 142:
// The parent origin of a PDF content frame should match the extensionWhat's the scenario where the above check fails, and code execution reaches this point?
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Andy Phan.
Andy Phan uploaded patch set #10 to this change.
[OOPIF PDF] Allow PDF resource requests for PDF navigation
Allow PDF extension resources to be requested only for the frames in the
PDF Viewer architecture. The frames that should be able to access the
resources are the extension frame and the content frame.
In ResourceRequestPolicy::CanRequestResource(), if the top frame has the
same origin as the resource, then the resource can be requested. This is
the case for MimeHandlerViewGuest PDF Viewer, as the PDF extension is in
an inner WebContents.
For OOPIF PDF Viewer, there is only one WebContents, so the top frame's
origin is not the same. Loosen the conditions to allow PDF resource
requests for OOPIF PDF Viewer.
Bug: 1445746
Change-Id: I05a5273d5842d35a70a08233761fb1416d97effb
---
M chrome/renderer/BUILD.gn
M chrome/renderer/extensions/DEPS
M chrome/renderer/extensions/resource_request_policy.cc
3 files changed, 38 insertions(+), 0 deletions(-)
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Lei Zhang.
4 comments:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {I would suggest checking for PAGE_TRANSITION_AUTO_SUBFRAME instead of for about:blank, since that's […]
Checking for PAGE_TRANSITION_AUTO_SUBFRAME, with a CHECK for about:blank.
Patch Set #6, Line 144:
Parent
+1 […]
Added conditional to check that parent is non-null.
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #9, Line 134:
extension_origin.host() == extension_misc::kPdfExtensionId) {Check the scheme too.
Done
Patch Set #9, Line 142:
// The parent origin of a PDF content frame should match the extensionWhat's the scenario where the above check fails, and code execution reaches this point?
This scenario occurs for the PDF content frame, which needs to access the stream resource. Added comments to clarify each scenario.
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Andy Phan, Lei Zhang.
3 comments:
Patchset:
Thanks, Andy, and sorry for the delay! It looks like I was never properly added to the feature set in Gerrit, so I never even saw this one come in. A couple of basic questions here
File chrome/renderer/extensions/resource_request_policy.cc:
extension_origin.scheme() == kExtensionScheme &&
extension_origin.host() == extension_misc::kPdfExtensionId) {
// The PDF extension frame should be able to request resources from itself.
if (ui::PageTransitionCoreTypeIs(
transition_type,
ui::PageTransition::PAGE_TRANSITION_AUTO_SUBFRAME) &&
frame_origin.scheme() == kExtensionScheme &&
frame_origin.host() == extension_misc::kPdfExtensionId) {
// The URL will be about:blank when the PDF extension requests its own
// resources.
CHECK(frame_url.IsAboutBlank());
return true;
}I'm surprised this is necessary. If the requesting frame is the PDF extension itself, why would it (ever) be blocked from accessing it's resources?
For regular extensions, if you do something like:
- open about:blank from an extension page
- try to fetch a resource
The fetch succeeds, because the about:blank frame is still tied to the extension origin.
What is the PDF viewer doing differently?
// The PDF content frame should be able to request resources from the PDF
// extension. The parent origin of a PDF content frame should match the
// extension origin.
blink::WebFrame* parent = frame->Parent();
if (parent) {
GURL parent_origin = url::Origin(parent->GetSecurityOrigin()).GetURL();
if (parent_origin == extension_origin) {
return true;
}
}what is the frame origin in this case?
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
findit-for-me@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Andy Phan, Lei Zhang.
This change will be blocked from submission as there are files which do not meet the coverage criteria. Following files have incremental coverage(all tests) < 70%.
- //chrome/renderer/extensions/resource_request_policy.cc
Please add tests for uncovered lines, or add Low-Coverage-Reason:<reason> in the change description to bypass. See https://bit.ly/46jhjS9 to understand when it is okay to bypass. If you think coverage is underreported, file a bug at https://bit.ly/3ENM7Pe
Patch set 11:Code-Coverage -1
Andy Phan (Gerrit)
Attention is currently required from: Andy Phan, Lei Zhang.
Andy Phan uploaded patch set #12 to this change.
[OOPIF PDF] Allow PDF resource requests for PDF navigation
Allow PDF extension resources to be requested only for the frames in the
PDF Viewer architecture. The frames that should be able to access the
resources are the extension frame and the content frame.
In ResourceRequestPolicy::CanRequestResource(), if the top frame has the
same origin as the resource, then the resource can be requested. This is
the case for MimeHandlerViewGuest PDF Viewer, as the PDF extension is in
an inner WebContents.
For OOPIF PDF Viewer, there is only one WebContents, so the top frame's
origin is not the same. Loosen the conditions to allow PDF resource
requests for OOPIF PDF Viewer.
Bug: 1445746
Change-Id: I05a5273d5842d35a70a08233761fb1416d97effb
---
M chrome/renderer/BUILD.gn
M chrome/renderer/extensions/DEPS
M chrome/renderer/extensions/resource_request_policy.cc
3 files changed, 28 insertions(+), 0 deletions(-)
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Devlin Cronin.
Andy Phan would like Devlin Cronin to review this change.
[OOPIF PDF] Allow PDF resource requests for PDF navigation
Allow PDF extension resources to be requested only for the frames in the
PDF Viewer architecture. The frames that should be able to access the
resources are the extension frame and the content frame.
In ResourceRequestPolicy::CanRequestResource(), if the top frame has the
same origin as the resource, then the resource can be requested. This is
the case for MimeHandlerViewGuest PDF Viewer, as the PDF extension is in
an inner WebContents.
For OOPIF PDF Viewer, there is only one WebContents, so the top frame's
origin is not the same. Loosen the conditions to allow PDF resource
requests for OOPIF PDF Viewer.
Bug: 1445746
Change-Id: I05a5273d5842d35a70a08233761fb1416d97effb
---
M chrome/renderer/BUILD.gn
M chrome/renderer/extensions/DEPS
M chrome/renderer/extensions/resource_request_policy.cc
3 files changed, 28 insertions(+), 0 deletions(-)
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Devlin Cronin.
Patch Set #6, Line 137:
if (frame_url.IsAboutBlank()) {
Checking for PAGE_TRANSITION_AUTO_SUBFRAME, with a CHECK for about:blank.
Potentially obsolete, so marking resolved for now.
File chrome/renderer/extensions/resource_request_policy.cc:
extension_origin.scheme() == kExtensionScheme &&
extension_origin.host() == extension_misc::kPdfExtensionId) {
// The PDF extension frame should be able to request resources from itself.
if (ui::PageTransitionCoreTypeIs(
transition_type,
ui::PageTransition::PAGE_TRANSITION_AUTO_SUBFRAME) &&
frame_origin.scheme() == kExtensionScheme &&
frame_origin.host() == extension_misc::kPdfExtensionId) {
// The URL will be about:blank when the PDF extension requests its own
// resources.
CHECK(frame_url.IsAboutBlank());
return true;
}
I'm surprised this is necessary. […]
Normally, `page_origin == extension_origin`, so extensions can get their own resources [1].
For OOPIF PDF case, I've observed that `page_origin` actually matches the original PDF URL's origin and not the extension origin, which is why resources cannot be requested.
Added a comment for this.
(Additionally, the code highlighted is actually no longer needed. The condition below is sufficient for both the extension frame and the content frame cases.)
// The PDF content frame should be able to request resources from the PDF
// extension. The parent origin of a PDF content frame should match the
// extension origin.
blink::WebFrame* parent = frame->Parent();
if (parent) {
GURL parent_origin = url::Origin(parent->GetSecurityOrigin()).GetURL();
if (parent_origin == extension_origin) {
return true;
}
}what is the frame origin in this case?
The frame origin matches the original PDF content URL's.
For example, if a user navigates to http://www.pdf995.com/samples/pdf.pdf, then the origin would be origin for the PDF content frame would be that.
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Devlin Cronin.
2 comments:
File chrome/renderer/extensions/resource_request_policy.cc:
extension_origin.scheme() == kExtensionScheme &&
extension_origin.host() == extension_misc::kPdfExtensionId) {
// The PDF extension frame should be able to request resources from itself.
if (ui::PageTransitionCoreTypeIs(
transition_type,
ui::PageTransition::PAGE_TRANSITION_AUTO_SUBFRAME) &&
frame_origin.scheme() == kExtensionScheme &&
frame_origin.host() == extension_misc::kPdfExtensionId) {
// The URL will be about:blank when the PDF extension requests its own
// resources.
CHECK(frame_url.IsAboutBlank());
return true;
}
Normally, `page_origin == extension_origin`, so extensions can get their own resources [1]. […]
To add more details:
Most of the resources can be requested, since they meet the `frame_url.DeprecatedGetOriginAsURL() == extension_origin` condition. E.g. `index.css, main.js, ...`.
The two resource requests that need special permission is the PDF's stream resource URL. There's a resource with an ID unique for each PDF viewer instance, that is required for PDF loading.
The resource_url will look something like: `chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/b44d5875-76c0-4155-8267-c57ced0cccd9`.
When the PDF extension frame tries to request the stream resource, `frame_url` is empty and `page_origin` is the original PDF URL.
When the PDF content frame tries to request the stream resource, `frame_url` and `page_origin` are both the original PDF URL.
// The PDF content frame should be able to request resources from the PDF
// extension. The parent origin of a PDF content frame should match the
// extension origin.
blink::WebFrame* parent = frame->Parent();
if (parent) {
GURL parent_origin = url::Origin(parent->GetSecurityOrigin()).GetURL();
if (parent_origin == extension_origin) {
return true;
}
}
The frame origin matches the original PDF content URL's. […]
And the frame origin is the PDF content URL for the PDF extension frame's resource request, too.
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Andy Phan, Łukasz Anforowicz.
Devlin Cronin would like Łukasz Anforowicz to review this change authored by Andy Phan.
[OOPIF PDF] Allow PDF resource requests for PDF navigation
Allow PDF extension resources to be requested only for the frames in the
PDF Viewer architecture. The frames that should be able to access the
resources are the extension frame and the content frame.
In ResourceRequestPolicy::CanRequestResource(), if the top frame has the
same origin as the resource, then the resource can be requested. This is
the case for MimeHandlerViewGuest PDF Viewer, as the PDF extension is in
an inner WebContents.
For OOPIF PDF Viewer, there is only one WebContents, so the top frame's
origin is not the same. Loosen the conditions to allow PDF resource
requests for OOPIF PDF Viewer.
Bug: 1445746
Change-Id: I05a5273d5842d35a70a08233761fb1416d97effb
---
M chrome/renderer/BUILD.gn
M chrome/renderer/extensions/DEPS
M chrome/renderer/extensions/resource_request_policy.cc
3 files changed, 28 insertions(+), 0 deletions(-)
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Andy Phan, Łukasz Anforowicz.
2 comments:
Patchset:
Thanks, Andy!
Andy and I talked offline, and adding a test for this isn't really feasible at the moment -- our renderer unit tests don't allow us to set up the proper environment, and this will really need an E2E pdf test.
I'm fine holding off on the test for now (this is gated on a base::Feature), but I do have an open question here. I'm not *too* concerned about this because the real protections should be in the browser anyway, but I did want to flag it and bring it to @luk...@chromium.org's attention.
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #12, Line 142:
blink::WebFrame* parent = frame->Parent();I'm assuming that, in the case of an iframe embedding a PDF, we can't look at the top-level origin to match it against the PDF extension?
I'm slightly worried here about an attack where we have something like:
evil.com
- pdf_extension_frame
- pdf_frame
and then evil.com navigates pdf_frame (via `frames`) to evil.com/subframe, and then evil.com/subframe can access any PDF extension resources. Is that a concern?I don't think there's a _ton_ that could happen there (since the resources would still be properly permissioned, I'd think?), but it... seems undesirable. Maybe that's just a cost we have here, though, since I suppose that might also be achievable through a malicious pdf_frame?
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Łukasz Anforowicz (Gerrit)
Attention is currently required from: Andy Phan, Devlin Cronin.
2 comments:
Patchset:
Thanks, Andy! […]
Drive-by comment: There is an example of adding an end-to-end PDF test in https://chromium-review.googlesource.com/c/chromium/src/+/1954870
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #12, Line 142:
blink::WebFrame* parent = frame->Parent();
I'm assuming that, in the case of an iframe embedding a PDF, we can't look at the top-level origin t […]
I think that I still stand by my earlier position [1] that
1. When reviewing from a security perspective, I think this is all fine, because ultimately we rely on correctness of the browser-side checks (the checks in the renderer are secondary). FWIW, I haven't reviewed the related browser-side checks for this scenario (but my default position is to assume that somebody has reviewed them in the past and that they look okay).
2. When just giving an opinion / optional advice, I think that having some consistency between renderer and browser is desirable for maintainability, but I guess that I defer to the Chrome Extensions team on this. If this early return needs to be made, then so be it. (And FWIW in the current patchset the early return seems relatively well/narrow-scoped.)
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Devlin Cronin.
1 comment:
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #12, Line 142:
blink::WebFrame* parent = frame->Parent();
I think that I still stand by my earlier position [1] that […]
Re: the evil.com example,
I don't think pdf_frame will ever be created. A valid PDF load needs to occur in order for the PDF extension to properly load and create the pdf_frame. Otherwise, the PDF viewer fails to load.
The embed for the pdf_frame is created in PDF extension JS code [1].
I.e., if evil.com attempts to navigate directly to the pdf_extension_frame, there isn't a PDF stream to consume, so the PDF extension JS code errors out [2], and the embed for the pdf_frame won't be created.
[2] https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/resources/pdf/main.ts;l=62
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Andy Phan, Łukasz Anforowicz.
Patch set 13:Code-Review +1
2 comments:
Patchset:
Thanks, Andy!
LGTM.
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #12, Line 142:
blink::WebFrame* parent = frame->Parent();
Re: the evil.com example, […]
Right -- I'm thinking more along the lines of evil.com embedding a (real) pdf, which triggers the PDF extension and frame load, such that then we get the frame tree I mentioned above (is that right so far?). Then, once that loads, evil.com can navigate the pdf_frame (embedded by the parent extension_frame, but accessible through window.frames) to its own resource. I'm assuming the pdf extension wouldn't monitor this and remove itself from the frame tree (does it?), and then that evil frame could make requests to the pdf extension that would be approved on the renderer, because it's from a frame that the pdf extension embeds.
Does that make sense?
Regardless, as Lukasz mentioned, this doesn't _really_ matter because it's in the renderer? So most of this is just an academic discussion at this point.
In the interest of unblocking this CL, I'll stamp for now, but it does seem like something we'd ideally make better in the future. What do you think about filing a bug that, ideally, this carve out wouldn't be required?
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Łukasz Anforowicz.
4 comments:
Patchset:
Drive-by comment: There is an example of adding an end-to-end PDF test in https://chromium-review. […]
Marking as done.
File chrome/renderer/extensions/resource_request_policy.cc:
extension_origin.scheme() == kExtensionScheme &&
extension_origin.host() == extension_misc::kPdfExtensionId) {
// The PDF extension frame should be able to request resources from itself.
if (ui::PageTransitionCoreTypeIs(
transition_type,
ui::PageTransition::PAGE_TRANSITION_AUTO_SUBFRAME) &&
frame_origin.scheme() == kExtensionScheme &&
frame_origin.host() == extension_misc::kPdfExtensionId) {
// The URL will be about:blank when the PDF extension requests its own
// resources.
CHECK(frame_url.IsAboutBlank());
return true;
}
To add more details: […]
Marking as done.
// The PDF content frame should be able to request resources from the PDF
// extension. The parent origin of a PDF content frame should match the
// extension origin.
blink::WebFrame* parent = frame->Parent();
if (parent) {
GURL parent_origin = url::Origin(parent->GetSecurityOrigin()).GetURL();
if (parent_origin == extension_origin) {
return true;
}
}
And the frame origin is the PDF content URL for the PDF extension frame's resource request, too.
Marking as done.
File chrome/renderer/extensions/resource_request_policy.cc:
Patch Set #12, Line 142:
blink::WebFrame* parent = frame->Parent();
Right -- I'm thinking more along the lines of evil. […]
Ack, makes sense. creis@ and I have briefly discussed window.frames and if we should somehow disable access to the PDF extension frame (and therefore also disabling access to the pdf_frame). I'll have to escalate this conversation further.
I filed two bugs which may solve this. One for potentially disallowing access to the content frame (which means this would no longer be a concern, and may have to be done anyway) [1], and one for potentially just changing how we access the PDF extension resources so this wouldn't occur [2].
Thanks!
[1] https://bugs.chromium.org/p/chromium/issues/detail?id=1495989
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=1495995
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit-for-me@appspot.gserviceaccount.com (Gerrit)
Attention is currently required from: Andy Phan.
This change meets the code coverage requirements.
Patch set 17:Code-Coverage +1
Łukasz Anforowicz (Gerrit)
Attention is currently required from: Andy Phan.
Patch set 17:Code-Review +1
1 comment:
Patchset:
Moving myself to CC, I defer to Devlin for review
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Patch set 18:Commit-Queue +2
Andy Phan (Gerrit)
Attention is currently required from: Andy Phan.
Andy Phan uploaded patch set #19 to this change.
[OOPIF PDF] Allow PDF resource requests for PDF navigation
Allow PDF extension resources to be requested only for the frames in the
PDF Viewer architecture. The frames that should be able to access the
resources are the extension frame and the content frame.
In ResourceRequestPolicy::CanRequestResource(), if the top frame has the
same origin as the resource, then the resource can be requested. This is
the case for MimeHandlerViewGuest PDF Viewer, as the PDF extension is in
an inner WebContents.
For OOPIF PDF Viewer, there is only one WebContents, so the top frame's
origin is not the same. Loosen the conditions to allow PDF resource
requests for OOPIF PDF Viewer.
Low-Coverage-Reason: TESTS_IN_SEPARATE_CL Future end-to-end tests for
OOPIF PDF will provide code coverage for the changes made in this CL.
Bug: 1445746
Change-Id: I05a5273d5842d35a70a08233761fb1416d97effb
---
M chrome/renderer/BUILD.gn
M chrome/renderer/extensions/DEPS
M chrome/renderer/extensions/resource_request_policy.cc
3 files changed, 28 insertions(+), 0 deletions(-)
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Lei Zhang.
Andy Phan would like Lei Zhang to review this change.
[OOPIF PDF] Allow PDF resource requests for PDF navigation
Allow PDF extension resources to be requested only for the frames in the
PDF Viewer architecture. The frames that should be able to access the
resources are the extension frame and the content frame.
In ResourceRequestPolicy::CanRequestResource(), if the top frame has the
same origin as the resource, then the resource can be requested. This is
the case for MimeHandlerViewGuest PDF Viewer, as the PDF extension is in
an inner WebContents.
For OOPIF PDF Viewer, there is only one WebContents, so the top frame's
origin is not the same. Loosen the conditions to allow PDF resource
requests for OOPIF PDF Viewer.
Low-Coverage-Reason: TESTS_IN_SEPARATE_CL Future end-to-end tests for
OOPIF PDF will provide code coverage for the changes made in this CL.
Bug: 1445746
Change-Id: I05a5273d5842d35a70a08233761fb1416d97effb
---
M chrome/renderer/BUILD.gn
M chrome/renderer/extensions/DEPS
M chrome/renderer/extensions/resource_request_policy.cc
3 files changed, 28 insertions(+), 0 deletions(-)
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Lei Zhang.
1 comment:
Patchset:
+thestig@ to reviewer for ownership of pdf/pdf_features.h for DEPS.
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Andy Phan (Gerrit)
Attention is currently required from: Lei Zhang.
Andy Phan uploaded patch set #20 to this change.
[OOPIF PDF] Allow PDF resource requests for PDF navigation
Allow PDF extension resources to be requested only for the frames in the
PDF Viewer architecture. The frames that should be able to access the
resources are the extension frame and the content frame.
In ResourceRequestPolicy::CanRequestResource(), if the top frame has the
same origin as the resource, then the resource can be requested. This is
the case for MimeHandlerViewGuest PDF Viewer, as the PDF extension is in
an inner WebContents.
For OOPIF PDF Viewer, there is only one WebContents, so the top frame's
origin is not the same. Loosen the conditions to allow PDF resource
requests for OOPIF PDF Viewer.
Bug: 1445746
Change-Id: I05a5273d5842d35a70a08233761fb1416d97effb
---
M chrome/renderer/BUILD.gn
M chrome/renderer/extensions/DEPS
M chrome/renderer/extensions/resource_request_policy.cc
3 files changed, 28 insertions(+), 0 deletions(-)
To view, visit change 4782091. To unsubscribe, or for help writing mail filters, visit settings.
Lei Zhang (Gerrit)
Andy Phan (Gerrit)
Patch set 20:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[OOPIF PDF] Allow PDF resource requests for PDF navigation
Allow PDF extension resources to be requested only for the frames in the
PDF Viewer architecture. The frames that should be able to access the
resources are the extension frame and the content frame.
In ResourceRequestPolicy::CanRequestResource(), if the top frame has the
same origin as the resource, then the resource can be requested. This is
the case for MimeHandlerViewGuest PDF Viewer, as the PDF extension is in
an inner WebContents.
For OOPIF PDF Viewer, there is only one WebContents, so the top frame's
origin is not the same. Loosen the conditions to allow PDF resource
requests for OOPIF PDF Viewer.
Bug: 1445746
Change-Id: I05a5273d5842d35a70a08233761fb1416d97effb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4782091
Reviewed-by: Łukasz Anforowicz <luk...@chromium.org>
Code-Coverage: findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>
Reviewed-by: Lei Zhang <the...@chromium.org>
Reviewed-by: Devlin Cronin <rdevlin...@chromium.org>
Commit-Queue: Andy Phan <andy...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1221234}
---
M chrome/renderer/BUILD.gn
M chrome/renderer/extensions/DEPS
M chrome/renderer/extensions/resource_request_policy.cc
3 files changed, 28 insertions(+), 0 deletions(-)