Dangerous strings can reach browser builtins

[mikesamuel]

Do you want to request a feature or report a bug?

A bug, but a well known and worked-around one.

What is the current behavior?

var x = 'javascript:alert(1)';
ReactDOM.render(
  (<a href={x}>Link</a>),
  document.getElementById('container')
);

produces a link that alerts.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

• Load the code above in the codepen REPL
• After the REPL loads, click the "Run" button at the top left.
• You should see a blue "link" in the bottom-right pane.
• Click it. An alert will popup.

The alert should not pop up.

A simple string that reaches an href attribute should not cause arbitrary code execution even with user interaction.

What is the expected behavior?
A string that reaches a browser builtin like the HTMLAElement.prototype.href setter should not cause code execution.

Discussion

Polymer Resin uses hooks in another webcomponents framework to intercept value before they reach browser builtins where they can be vetted. A similar approach could work for React.

It allows values to reach browser builtins when they are innocuous or have a runtime type that indicates that the author intentionally marked them as safe for that kind of browser builtin.

For example, an instanceof SafeURL would be allowed to reach HTMLAElement.prototype.href as would any string that is a relative URL, or one with a whitelisted protocol in (http, https, mailto, tel) but not javascript:....

Many developers know that <a href={...}> is risky, but if the link is an implementation detail of a custom React element, then developers don't have the context to know which attributes they need to be careful with. They shouldn't have to either since it is an implementation detail.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

I believe this is widespread across versions.

An earlier REPL I tried showed that it worked on version 16.2.0 from https://unpkg.com/react-dom/umd/react-dom.development.js but I don't know what version the jsfiddle above uses.

[gaearon]

Some past discussion on this:

#3473 (comment)
#3473 (comment)

[milesj]

If you're looking for safe HTML parsing and XSS protection, I offer up https://github.com/milesj/interweave (it's been pen tested).

Ignore the failed build, I'm working on changing the environment.

[mikesamuel]

@gaearon Thanks for the pointers.

To summarize,
#3473 (comment) mentions a similar mechanism for explicit

// explicitly React.createElement the POJO
<div>{trust(stringOrElement)}</div> 

This is similar to Perl untaint. Like tainting, it's a binary decision, not a decision to trust in a specific set of contexts a la https://github.com/WICG/trusted-types

Other themes in that thread:

• Proposed new syntax to allow authors to specify sinks known to receive only trusted values.
  I dislike this approach because it pushes XSS safety onto the application developer.
• Recognition that script element bodies and style attributes are risky value sinks.

To respond in particular to the ones you mentioned:

#3473 (comment)

@brigand We've already evaluated and decided against protecting against invalid styles. It is not a simple fix. When we last evaluated it, we found that:

• It was prohibitively expensive to do this automatically for you for any given value.

Requiring values that are not simple numbers or keywords to be explicitly wrapped in a type that specifies them as such makes things easier.

There is an ambiguity between strings used e.g., as URLs and font-faces which is problematic,
so this is not backwards compatible for injection of URLs.

• A whitelist of safe patterns would be massively huge and we actively want to move away from whitelists in React and fallback to the underlying DOM.
• Any blacklist we could come up with would likely be incomplete anyway.

Agreed x 2.

#3473 (comment)

E.g. what are all the dangers of a user supplied href? javascript: is one, but it applies to 3rd-party protocols too

This is true, but Webcomponents frameworks complicate things. In <a href={x}>...</a> I can see that x ends up being used as a URL which, if clicked, will execute as a script.

The same is not true when the user is writing <CustomComponent x={x} />. If that is defined

class MyCustomComponent extends React.Component {
  render() { 
    return (<a href={this.props.x}>Link</a>)
  }
}

then the fact that x reaches HTMLAElement.prototype.href instead of window.open is an implementation detail that should not have security consequences.

Custom components give developers great new ways to decompose an application into simpler part but the implementation hiding (which is otherwise a strength) make it harder to use code review and other techniques to avoid XSS.

---

Would you rather I copy these responses there?

---

@miesj, IIUC, interweave treats interpolated values and tags specified in JSX with the same level of trust. Is that right? The pre-interpolation hooks that worked for Polymer Resin treat the template as trusted, and are suspicious of interpolated values.

[gaearon]

To be clear, the conclusion from #3473 was that we implemented brand-checking for React elements so <div>{userContent}</div> can’t cause an XSS. But that doesn’t solve the href scenario.

[mikesamuel]

@gaearon Thanks for explaining. IIUC, that change prevented crafted JSON and objects that are mass-assigned from getting the same level of trust as the output of a JSX expression that produces a component.

[milesj]

@mikesamuel Sorry, got a bit ahead of myself. You're right, as the markup would need to be a string, which is then safely parsed and filtered with Interweave, like so.

var x = 'javascript:alert(1)';
ReactDOM.render(
  <Interweave content={`<a href=${x}>Link</a>`} />,
  document.getElementById('container')
);

That being said, I could possibly bubble up the XSS filter functions for easy reuse.

[mikesamuel]

As an update, I floated reactjs/rfcs#55 (comment) but that is tabled. It sounds like React is interested in opt-in to XSS safety via integration with Trusted Types.

[aweary]

You might interested in this warning that is being added for javascript: links as well.