search

WICG / sanitizer-api

https://wicg.github.io/sanitizer-api/
Other
226 stars 31 forks source link

Also sanitize javascript: href on MathML and SVG anchor? #168

Open evilpie opened 2 years ago

evilpie commented 2 years ago

The easiest way of implementing https://wicg.github.io/sanitizer-api/#handle-funky-elements in Gecko will also automatically sanitize the href attribute from svg:a and MathML elements. Maybe something to consider.

evilpie commented 2 years ago

See also #147

mozfreddyb commented 2 years ago

I think we should do that. @otherdaniel thoughts?

otherdaniel commented 2 years ago

Makes sense, generally. I think the idea was to drop javascript:-URLs for navigation links, since no other link types support the javascript:-URL handling anyways, and <svg:a href=...> would certainly quality. Off-hand, I'm not sure which MathML elements contain navigation links.

annevk commented 10 months ago

This should also happen for all <form>-related ways of navigation.

mozfreddyb commented 6 months ago

@otherdaniel Pls remember to double-check w/ your pull request (e.g., xlink href, ...)

bkardell commented 5 months ago

Makes sense, generally. I think the idea was to drop javascript:-URLs for navigation links, since no other link types support the javascript:-URL handling anyways, and <svg:a href=...> would certainly quality. Off-hand, I'm not sure which MathML elements contain navigation links.

Part of this might be because there isn't a clearly correct answer :). WebKit and Gecko allow any MathML element to have an href. Chromium allows none of them to have an href. The MathML-Core L1 remains moot on it specifically because it was getting hard to sort out and there were a bunch of issues to discuss. I guess the best entry issue there is https://github.com/w3c/mathml-core/issues/142 -- The Working group would relish the ability to sort it out though so we could get them worked out. L1 is a lot stronger with it, I think. The proposal was perhaps token element and mrow. There was even an argument made iirc that perhaps even just on mrow would be enough. The reason being that tokens can contain foreign content and could therefore at least contain a link pretty easily already, and the math element itself can be wrapped in one - it is really maybe primarily the generic grouping element (mrow) that

annevk commented 5 months ago

Chromium's model of not supporting href in MathML that way seems superior. It also doesn't seem like it works well in WebKit, it doesn't match :link for instance.

otherdaniel commented 5 months ago

PR #212 is somewhat preliminary,and is meant to address this issue:

That last bit is rather ad-hoc: It isn't super precide, and forbids some situations that aren't actually dangerous (like an href attribute on an element that doesn't actually support it). It's the simplest check I could think of that leaves the animation support basically intact, but still covers all situations where animtion can be used to create javascript:-URLs.

I'm not quite sure what to do with MathML. One option would be to check for any MathML-element with an href attribute.

zcorpan commented 5 months ago

Checking href and xlink:href on all MathML elements seems reasonable.

bkardell commented 5 months ago

Chromium's model of not supporting href in MathML that way seems superior. It also doesn't seem like it works well in WebKit, it doesn't match :link for instance.

It also doesn't (iirc) work with related attributes (target, for example) and has unusual default tabdex which threw us way back in https://github.com/whatwg/html/pull/5248#issuecomment-599271891

benbucksch commented 4 months ago

The primary reason why I need (and originally created, for Mozilla Mail/News and Thunderbird) the sanitizer is to remove all Javascript (for security) and all external pings to server (mostly for privacy, partially for security). I think that's what devs will generally expect from a sanitizer, and should be the default. If there's any way whatsoever to run Javascript, the sanitizer is no longer useful.

This is particularly important for cases that normal devs do not think about, like MathML and SVG, and they will likely not test that, unless they are security experts.

So, I'm in favor of removing href from SVG and MathML. Ditto for any other Javascript and external links.

annevk commented 4 months ago

We discussed and agreed we should do what @zcorpan said above for MathML.

212 covers what we should do for SVG.

The remainder of the concerns should be handled by the safelists of elements and their attributes (e.g., <div href> will end up as <div> which helps defend against some obscurer reparsing attacks).

We don't think href should generally be removed as it doesn't lead to script execution on its own. We only want to defend against javascript: URL injection. (It's very easy to add href yourself to blockAttributes though.)

benbucksch commented 4 months ago

Sounds great to me.

I agree it makes sense to preserve links that a) require a user interaction (click on link) and that b) do not run JavaScript, but are https: URLs and c) open in another browser, not in the same window where the sanitized page was.