Consider What Happens to XSS Surface Area With No Whitelist

[sebmarkbage]

Consider the case in #10506 but let's say we've fix it by special casing dangerouslySetInnerHTML to use a symbol.

Now consider this URL:

?{"content":"Click%20Me","onclick":"alert(%27p0wned%27)"}

If onclick (or any other event name, current or future) gets passed through as a plain old string, then this is a XSS vulnerability.

The tricky part about this one is that it's plain old strings and it's a potentially infinite long list since it also includes future event names.

Now I think a safe strategy might be to hard blacklist (meaning they don't get added to the DOM) any attribute that starts with on. Does that cover everything?

cc @nhunzaker

[nhunzaker]

I think checking for attributes that start with on is the best approach. Preact and Inferno already do this:

1. Preact
2. Inferno

There's a related conversation to have about custom/future event support. Depending on the timeline, that might not be possible for 16.0.0. Would adding custom event support require a major release?

I'm going to investigate if the Event prototype gives us anything about if the handler is a string. I'm guessing no, but it looks like arguments is correct in button.setAttribute('onclick', 'console.log(arguments);debugger'), so maybe. It might be a nice guard, even if it's a long shot.

[nhunzaker]

Checking the spec... https://www.w3.org/TR/html51/webappapis.html#event-handlers-on-elements-document-objects-and-window-objects

I haven't found anything yet that, more or less, says "all events start with on", but I believe it is safe. Disallowing anything outside of that boundary and only allowing functions would be reasonable and address this issue.

[sebmarkbage]

Even if all events start with on there might be others. Here another fun XSS for example (src):

<script {...data}></script>

What else can be used?

[syranide]

We still do not provide any protection against <a href={userUrl} /> right? It's not really apples to apples, but that seems like a far more common mistake, and easily overlooked too.

There will always be edge-cases that are hugely problematic, spreading onto a/form/style/script/object/embed (object/embed could probably be huge). If values can be objects then anywhere you spread is problematic (style is vulnerable). Neither to forget that spreading user-supplied values onto a component can theoretically do anything. If there are third-party scripts on the site you could also imagine quite complex attacks that React simply cannot defend against.

I wonder if it wouldn't be preferable to not address this in a sense. If you're spreading props in this manner it's obviously exploitable, but it's more likely that someone will just lololtroll you just for fun quickly rather than someone intentionally malicious finding a complex edge-case in the future that would be catastrophic. If you're spreading user-supplied props you're in for a bad time, so IMHO it makes sense that if you do add a blacklist, that it would straight up throw a strongly worded error that hopefully shows up in their error tracking.

[sebmarkbage]

We addressed this specifically for the on prefix case for now.