Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handling iframing via <embed> / <object> #37

Closed
arturjanc opened this issue Jul 31, 2019 · 11 comments
Closed

Handling iframing via <embed> / <object> #37

arturjanc opened this issue Jul 31, 2019 · 11 comments

Comments

@arturjanc
Copy link
Contributor

The value of Sec-Fetch-Mode should tell the server how its response will be used, specifically: if it is navigate or nested-navigate then the server should expect the response will render, and if it's something else (e.g. no-cors or cors) it will be used as a resource.

The problem is that the <embed>and <object> tags can load both resources (plugins, images) and renderable documents. At the time of sending the request the browser does not know the MIME type of the response so cannot guarantee to the server that it will not treat the response as HTML.

This can be a problem for developers who would like to restrict navigations (e.g. to mitigate the exploitation of XSS or as a server-side equivalent to X-Frame-Options) without restricting subresource loads; in that case, the server will see a no-cors load and will not have enough information to be able to reject such requests. This could potentiality be solved by Sec-Fetch-Dest but its status is not clear-cut. We should figure this out.

Examples:
https://arturjanc.com/fetch-metadata-hackit/iframe-embed-test.html
https://arturjanc.com/fetch-metadata-hackit/iframe-object-test.html

@mikewest
Copy link
Member

mikewest commented Aug 5, 2019

This could potentiality be solved by Sec-Fetch-Dest

Why "potentially"? It seems like Sec-Fetch-Dest would solve this (to the extent that we can "solve" it in the absence of a clear link between the response headers that we don't have yet and the browser's processing model). Does it leave a hole as currently defined?

@arturjanc
Copy link
Contributor Author

s/potentially/This could be solved by Sec-Fetch-Dest if we all agreed to ship it/ :)

I'm happy if this doesn't require any spec changes and all browsers will send the destination value in the future; but we'd probably need to convince @annevk that this is a reasonable thing to do. Perhaps the ambiguity of <embed> is a good (or at least, better) argument for doing so?

@annevk
Copy link
Member

annevk commented Aug 6, 2019

Is the initial mode for these no-cors then? Maybe we should fix that as that does seem like a footgun. E.g., maybe-nested-navigate or some such.

@mikewest
Copy link
Member

mikewest commented Aug 7, 2019

Is the initial mode for these no-cors then?

Yes. See step 4.4 of https://html.spec.whatwg.org/multipage/iframe-embed-object.html#the-object-element and step 3.4 of https://html.spec.whatwg.org/multipage/iframe-embed-object.html#the-embed-element-setup-steps. Neither specifies mode, so it's no-cors.

Maybe we should fix that as that does seem like a footgun. E.g., maybe-nested-navigate or some such.

I don't have a strong opinion about overloading mode vs shipping Sec-Fetch-Dest. I recall us punting on the latter because of Service Workers, and we would have needed to invent some kind of mechanism to allow a page to prevent a laundered response being delivered. Is that still something you're worried about @arturjanc?

@annevk
Copy link
Member

annevk commented Aug 7, 2019

A worry I have is that even if we expose "destination" many folks might not think of the embed/object subtlety and only look at "mode". So indicating it there as a significant enough different category might be better for ease-of-use.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 4, 2019
In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 4, 2019
In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
aarongable pushed a commit to chromium/chromium that referenced this issue Sep 5, 2019
In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Łukasz Anforowicz <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#693517}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 5, 2019
In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Łukasz Anforowicz <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#693517}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 5, 2019
In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Łukasz Anforowicz <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#693517}
@arturjanc
Copy link
Contributor Author

My slight concern with adding another mode is that it would complicate writing policy logic for developers. For every request, developers generally need to inspect both site and mode to make a security decision, and creating another mode just for this (uncommon) pattern will require developers to make an explicit decision about what to do in this case.

As it stands, iframing by using <embed> or <object> fails closed, in the sense that the load is treated as a subresource request (mode=no-cors) and will likely get blocked by sites that reject cross-site resource loads (which is a more common use case for Fetch Metadata than restricting navigations). By keeping the status quo we are making it more difficult for developers to use <embed> or <object> for cross-site framing, because such requests are likely to be blocked if they enable resource isolation logic, but otherwise we're not losing anything: it's just an obstacle to enabling Fetch Metadata restrictions in -- I believe -- a very uncommon scenario.

So, given the choice of adding another mode and keeping this as-is, I'd prefer the latter. (Maybe we can even kill framing renderable documents via these two elements?)

@mikewest, re:

I recall us punting on [Sec-Fetch-Dest] because of Service Workers, and we would have needed to invent some kind of mechanism to allow a page to prevent a laundered response being delivered. Is that still something you're worried about @arturjanc?

The outcome of that discussion was that currently it's not possible to launder destination via Service Workers (because all requests from SWs have to happen via fetch(), so the server that sees Sec-Fetch-Dest: empty on such requests). But this is supposedly not a strong guarantee and future SW changes could undermine this, so the advice from SW folks was to not build a security boundary on top of it.

Given all this, I'm fine with the current behavior and so I'm inclined to close this bug. @mikewest WDYT?

@annevk
Copy link
Member

annevk commented Sep 10, 2019

You're saying this would complicate policy, but your position would complicate policy (much more so, I think) for the scenario expressed in OP.

@arturjanc
Copy link
Contributor Author

The original scenario talks about deploying a policy for restricting cross-site navigations without also restricting subresource loads. This is a secondary use case for Fetch Metadata; I wouldn't want to complicate the necessary logic for the primary -- likely much more common -- use case of preventing cross-site subresource loads for this reason.

So, on balance, I think adding another mode value wouldn't be a great trade-off here.

@annevk
Copy link
Member

annevk commented Sep 11, 2019

Is there a document that lists anticipated policies and the logic required for them? That would help a lot in evaluating this. The explainer has some and from that it seems you would evaluate Sec-Fetch-Site and Sec-Fetch-Mode, which works except for this (provided that if you want to block subresource loads you don't also want to block navigate loads solely from embed and object).

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 12, 2019
…rvice., a=testonly

Automatic update from web-platform-tests
Set `Sec-Fetch-Mode` from the network service.

In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Łukasz Anforowicz <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#693517}

--
Mark appcache.tentative.https.sub.html as slow
--

wpt-commits: 4f6052ac7224c6bd0c859bea09d73f4a846920f8, d6b04f2d1e8a3268ab31b373ec0d14fa55829dc9
wpt-pr: 18838
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Sep 12, 2019
…rvice., a=testonly

Automatic update from web-platform-tests
Set `Sec-Fetch-Mode` from the network service.

In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Łukasz Anforowicz <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#693517}

--
Mark appcache.tentative.https.sub.html as slow
--

wpt-commits: 4f6052ac7224c6bd0c859bea09d73f4a846920f8, d6b04f2d1e8a3268ab31b373ec0d14fa55829dc9
wpt-pr: 18838
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
…rvice., a=testonly

Automatic update from web-platform-tests
Set `Sec-Fetch-Mode` from the network service.

In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <kinukochromium.org>
Reviewed-by: Łukasz Anforowicz <lukaszachromium.org>
Commit-Queue: Mike West <mkwstchromium.org>
Cr-Commit-Position: refs/heads/master{#693517}

--
Mark appcache.tentative.https.sub.html as slow
--

wpt-commits: 4f6052ac7224c6bd0c859bea09d73f4a846920f8, d6b04f2d1e8a3268ab31b373ec0d14fa55829dc9
wpt-pr: 18838

UltraBlame original commit: 7d3b0c03f936cc0fb5a532f740664b0517ec0cc9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
…rvice., a=testonly

Automatic update from web-platform-tests
Set `Sec-Fetch-Mode` from the network service.

In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <kinukochromium.org>
Reviewed-by: Łukasz Anforowicz <lukaszachromium.org>
Commit-Queue: Mike West <mkwstchromium.org>
Cr-Commit-Position: refs/heads/master{#693517}

--
Mark appcache.tentative.https.sub.html as slow
--

wpt-commits: 4f6052ac7224c6bd0c859bea09d73f4a846920f8, d6b04f2d1e8a3268ab31b373ec0d14fa55829dc9
wpt-pr: 18838

UltraBlame original commit: 7d3b0c03f936cc0fb5a532f740664b0517ec0cc9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
…rvice., a=testonly

Automatic update from web-platform-tests
Set `Sec-Fetch-Mode` from the network service.

In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <kinukochromium.org>
Reviewed-by: Łukasz Anforowicz <lukaszachromium.org>
Commit-Queue: Mike West <mkwstchromium.org>
Cr-Commit-Position: refs/heads/master{#693517}

--
Mark appcache.tentative.https.sub.html as slow
--

wpt-commits: 4f6052ac7224c6bd0c859bea09d73f4a846920f8, d6b04f2d1e8a3268ab31b373ec0d14fa55829dc9
wpt-pr: 18838

UltraBlame original commit: 7d3b0c03f936cc0fb5a532f740664b0517ec0cc9
@annevk
Copy link
Member

annevk commented Mar 10, 2021

Similar to #45 I believe this can be closed.

@mikewest
Copy link
Member

Similar to #45 I believe this can be closed.

I agree. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants