Implement dangling markup injection mitigation

[shhnjk]

As discussed in whatwg/fetch#546 and whatwg/url#284, we'd like to add the dangling markup injection mitigation to the HTML spec (which was implemented in Chromium for ~ 6 years).

This PR makes the following changes:

1. Add and implement the dangling markup injection mitigation in the check dangling markup algorithm.
2. Add HTML-parse a URL algorithm which calls the check dangling markup algorithm before parsing.
3. Add HTML-encoding-parsing-and-serialize a URL algorithm which has same logic as HTML-encoding-parsing-and-serializing a URL except it uses HTML-parse a URL algorithm above.
4. Re-route all dangling markup injection sinks to use check dangling markup-backed algorithms.

I have not modified the module specifier resolution, since it has a strict grammer and it's most likely to not result in a successful dangling markup injection.

I hope this change covers all of the scope we are interested in 😊 And going forward, any potential dangling markup injection sinks should call the check dangling markup-backed algorithms.

Fixes: #10021

• At least two implementers are interested (and none opposed):
  • Chromium
  • Gecko Implement dangling markup injection mitigation #10022 (comment)
• Tests are written and can be reviewed and commented upon at:
  • Add more tests for dangling markup mitigation web-platform-tests/wpt#44466
  • Add more dangling markup tests to WPT web-platform-tests/wpt#43808
• Implementation bugs are filed:
  • Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=680970
  • Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1899514
  • WebKit: https://bugs.webkit.org/show_bug.cgi?id=172748
• MDN issue is filed: Not necessary (< and whitespace are not valid in URLs).
• The top of this comment includes a clear commit message to use.

---

/common-dom-interfaces.html ( diff )
/form-control-infrastructure.html ( diff )
/iframe-embed-object.html ( diff )
/images.html ( diff )
/input.html ( diff )
/links.html ( diff )
/media.html ( diff )
/rendering.html ( diff )
/scripting.html ( diff )
/semantics.html ( diff )
/urls-and-fetching.html ( diff )

[shhnjk]

@annevk PTAL when you have time!

[evilpie]

Some first surface level comments:

• Reading the new spec text, it's not obvious that "Parse a URL" is meant to prevent dangling markup attacks. (Maybe a note could help here?) I am afraid it's going to be quite easy to "use" the wrong URL parser in future specifications.
• The linked WPT only seems to cover <img src=

[shhnjk]

• Reading the new spec text, it's not obvious that "Parse a URL" is meant to prevent dangling markup attacks. (Maybe a note could help here?) I am afraid it's going to be quite easy to "use" the wrong URL parser in future specifications.

Added a note for this.

• The linked WPT only seems to cover <img src=

I can work on adding more tests.

[shhnjk]

@annevk friendly ping :)

[annevk]

I'm sorry, but I don't think this works. "parse a URL" is an entry point for non-HTML callers that want an easy way to obtain a base URL, such as XMLHttpRequest and fetch().

We should probably have "HTML-parse a URL" that's "encoding-parse a URL", but limited to HTML elements and with the additional injection mitigations. And then we need good test coverage to ensure the injection mitigations are not cropping up in places they shouldn't.

[shhnjk]

So, what I found out is that I only care about parsing URL when the environment is a Document object, because that's when URL parsing is called from HTML Elements. I essentially added that branch to encoding-parse a URL, but I can add the same in parse a URL, so that the mitigation will only kicks in for the environment with Document object (which does not touch fetch or XHR).

Does that work?

[annevk]

That seems somewhat attractive and might be a good way to find all the endpoints, but I think a more responsible approach would be to start by identifying the endpoints that need this mitigation. Then you make those endpoints invoke a new parse a URL algorithm (e.g., "HTML-parse a URL" as I suggested above or "parse an element URL" as I suggested in whatwg/fetch#546 (comment) a couple years back).

Then we can worry about deduplicating some of the logic between that new URL parser wrapper and existing URL parser wrappers. That's mainly an editorial matter.

(It might well turn out that after this we can remove the document overload from "parse a URL" and friends which would be a nice clarification.)

And in parallel there needs to be test coverage for all these endpoint changes and also some test coverage for other URL endpoints to ensure they are not changing.

[shhnjk]

Added HTML-parse a URL (and HTML-encoding-parsing-and-serialize a URL) as suggested!

[annevk]

I suspect this probably has to change https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes as well.

Also, I had a brief look at the tests, but they don't look exhaustive. I had expected some kind of loop over all the features we want to test and whether they should have this behavior or not and then create a test for each of them.

[shhnjk]

The tests has been added.

[zcorpan]

Should CSS features that fetch things use HTML-parse also? Consider

<div style='background-image: url("?\
...secret stuff... '>

It's slightly harder to pull off since the style attribute needs to be closed on the next line for the url() to not be invalid.

[zcorpan]

Missing:

• media element <source src>: https://whatpr.org/html/10022/055a678...6e9e98e/media.html#loading-the-media-resource:encoding-parsing-a-url
• cite on blockquote; del; ins; q
• itemid (it shouldn't be fetched, so maybe this one is ok?) https://whatpr.org/html/10022/055a678...6e9e98e/microdata.html#attr-itemid
• microdata processing of src/ href/etc attributes: https://whatpr.org/html/10022/055a678...6e9e98e/microdata.html#values
• Hyperlink auditing (both the ping attribute and the href attribute): https://whatpr.org/html/10022/055a678...6e9e98e/links.html#hyperlink-auditing

[zcorpan]

I would still expect more test coverage though, in particular that XMLHttpRequest, EventSource, importScripts(), fetch(), new URL(), etc. are not impacted by this.

I'm not entirely sold that the mitigation shouldn't apply to such APIs. If pages use libraries or polyfills that take URLs from markup (e.g. data-src or even <img src>) and then fetch that with fetch() or XMLHttpRequest(), they are not covered by the mitigation.

https://github.com/search?q=fetch+dataset.src&type=code

[annevk]

Yeah, perhaps that's why the prior approach attempted patching the URL parser directly, but that didn't result in agreement. I personally think this is a reasonable middle ground.

[shhnjk]

Missing:

• media element <source src>: https://whatpr.org/html/10022/055a678...6e9e98e/media.html#loading-the-media-resource:encoding-parsing-a-url

Done!

• cite on blockquote; del; ins; q

This was removed per discussion that no browser would be rendering cite attribute as a link.

• itemid (it shouldn't be fetched, so maybe this one is ok?) https://whatpr.org/html/10022/055a678...6e9e98e/microdata.html#attr-itemid

Yes, this is okay to leave.

• microdata processing of src/ href/etc attributes: https://whatpr.org/html/10022/055a678...6e9e98e/microdata.html#values

Similar to above, the result will just generate an itemprop which does not initiate a request nor render a link. Therefore I don't think we need to cover that.

• Hyperlink auditing (both the ping attribute and the href attribute): https://whatpr.org/html/10022/055a678...6e9e98e/links.html#hyperlink-auditing

Done!

[annevk]

I was thinking that maybe we should treat cite et al in the same way in case they are implemented through script. To at least mitigate some of the concern @zcorpan has. But I realized that the fallback for not being able to parse a URL is to leave the attribute as-is, so if you just pass on element.cite for instance it's not going to help you and it's actually very hard to imagine a robust solution.

This isn't necessarily sufficient to make me think that we should inflict this weird variant of URL parsing on all APIs, but maybe we should do more to make the failure case discoverable. (element.citeCanParse and friends? Should be a separate issue, but wanted to share my thought process here.)

[shhnjk]

CC: @mikewest if he has an opinion about above and this comment.

[shhnjk]

Hi, is there any way of moving forward with this change?

[zcorpan]

@achristensen07's objection in whatwg/url#284 (comment) as I understand it was mainly:

Our URL object is used for many things in the operating system that have nothing to do with HTML parsing.

An HTML-specific mitigation isn't needed everywhere, true. But as I mentioned in #10022 (comment) , URLs found in HTML attributes can end up used in JS APIs, and so the mitigation as specified in this PR is not as strong as it could be. Having at least all browser-supported URL parsing entry points perform the check seems more robust.

Although I care more about what happens in browsers, I think having non-browser APIs have consistent URL parsing with browsers is reasonable, even if it includes quirks that are not needed in other contexts. We have a similar situation with https://url.spec.whatwg.org/#windows-drive-letter , and that was seemingly deemed acceptable.

[shhnjk]

Okay, so should we reopen whatwg/url#284 and move the discussion there?

[annevk]

I'm not persuaded that because you could take an arbitrary HTML attribute value and parse it as a URL we have to protect against this at the level of the URL parser and thereby impact every other URL parser consumer. (And create issues for them whereby they have to deal with these code points before handing them to the URL parser.)

There is a tradeoff here and I think that values the defense-in-depth argument too highly.

[mikewest]

Sorry for the delayed response @shhnjk; it's been a long few weeks...

I continue to prefer patching URL and dealing with the problem there. Given that opinions there apparently haven't changed in 7 years, I'm quite happy to land on something smaller that we can agree on. :)

To that end, I agree with @annevk that dealing with the core set of dangling markup attacks is probably the right place to start, and doing so via a new wrapper in HTML is probably fine.

This patch ends up scanning the URL a few times, however, which certainly doesn't match Chromium's implementation, and would probably be pretty expensive for other vendors as well. I'd suggest that we could be more efficient by embedding the dangling-markup check in URL as whatwg/url#284 suggests, but still leaving HTML responsible for any action taken.

If the flag in whatwg/url#284 is too much, a smaller change would be to introduce a more-specific validation error for step 2 of https://url.spec.whatwg.org/#concept-basic-url-parser (e.g. "ascii-tab-or-newline validation error" rather than "invalid-URL-unit validation error"), then consuming that error combined with rescanning the URL for > in the wrapper we introduce into HTML. That would have the same effect in the end, and doesn't seem like a meaningful hardship for URL parsers.

It seems unlikely to me that Chromium would change its implementation to scan a URL more than once in any event, given that my initial (very dumb) implementation had some pretty noticeable impacts on benchmarks. That implementation detail seems unlikely to me to be web visible, though, so if it's a spec-level representation we can agree upon, I'm happy with it.

[zcorpan]

I've discussed this with @mozfreddyb and @valenting. We're OK with this PR.

I still think the added fragmentation of different behavior for different URL entry points in the web platform is not great. Adding complexity that is sometimes possible to bypass (minimized markup or getAttribute() + fetch) is also not ideal. But I don't have a counter-proposal where I have confidence that it's web compatible and doesn't regress URL parsing performance, and having some mitigation is better than none.

[shhnjk]

@annevk can we merge this now?

[evilpie]

Implementation bug for Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1899514

[zcorpan]

I found an existing WebKit bug (https://bugs.webkit.org/show_bug.cgi?id=172748). I think this is ready to merge, but leaving to @annevk.

[annevk]

Sorry for missing this somehow. It seems like there needs to be a new Chromium bug as they would have to make changes, no? This also needs rebasing.