Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn for javascript: URLs in DOM sinks #15047

Merged
merged 11 commits into from Mar 11, 2019

Conversation

@sebmarkbage
Copy link
Member

commented Mar 7, 2019

Motivation

URLs is a common attack surface. If an attack can get a user provided URL into one of the DOM sinks it is essentially game over. Even partials of URLs can be bad.

javascript: links execute arbitrary code in the context of the page. URLs in image srcs can be used to track or even exfiltrate some data. URLs in script tags can execute arbitrary URLs. Links to mobile apps can cause problems.

There is no way we can cover all cases because there are many intentional use cases that overlap with unintentional ones. We intend to support Trusted Types objects passed to sinks like <a href=...> etc. which allows safe URLs to be distinguished from unsafe ones. The right approach here is a whitelist instead of a blacklist. However, opting in to Trusted Types is kind of non-trivial so we'd expect it to take a while for most sites.

In the meantime there is one XSS hole that is particularly common to forget about. This could be a javascript: URL.

<a href={url}>...</a>

JavaScript URLs are particularly bad because it gains full control over the page where as other vulnerabilities relies on other things going wrong (such as native apps) or has limited access.

JavaScript URLs is also not really a legit use case once you're on the client in React since you can just attach event listeners and preventDefault instead.

Once you upgrade to Trusted Types or CSP, the first thing you'll do is probably add a filter that blocks JavaScript URLs anyway.

One thing React can do to help with this problem in the meantime is to forbid javascript: URLs when rendered by React - by default.

Breaking Change

Unfortunately, this would be a breaking change. This PR introduces a DEV mode deprecation warning which will last for the remainder of React 16.x. This doesn't actually provide any safety but it lets people clean up all their existing use cases.

Behind a flag, we also enable actually enforcing this block in production mode at runtime, which we'll turn on for a future React major.

Coverage

This PR adds a flag to the property info for a few property names. We only block javascript URLs in native DOM, not in Custom Elements. We block this on the property name basis but the intention is to cover these cases which are confirmed to execute JavaScript URLs:

HTML

<a href />
<form action />
<iframe src />
<area href />
<button formaction />
<input formaction />
<frame src />

SVG

<a xlinkHref />
<a href />

The <script src /> case is inherently broken since it executes script anyway.

We don't validate <script text />, ref.innerHTML and <iframe srcdoc /> but they're more known to be dangerous. The future solution for those is to let pass-through Trusted Types to these values and use the policy to control them.

My understanding is that we don't need to cover these because they already don't execute javascript: URLs:

<link href />
<img src />
<input type="image" src />
<source src />
<track src />
<media src />
<embed src />
<img srcset />
<object data />
<object codebase />
<meta http-equiv="refresh" content="0;URL='...'" />

They can be other vulnerabilities though so in the future we'll also allow them to accept Trusted Types.

EDIT: <base href /> is vulnerable to:

<base href="javascript:alert(1)//">
<a href="#">click</a>

PR

For code reviewers: The first two commits just enables us to write tests against frameset.

@sizebot

This comment has been minimized.

Copy link

commented Mar 7, 2019

ReactDOM: size: 🔺+0.2%, gzip: 🔺+0.2%

Details of bundled changes.

Comparing: 5d0c3c6...c8979c0

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.4% +0.4% 776.31 KB 779.14 KB 176.84 KB 177.56 KB UMD_DEV
react-dom.production.min.js 🔺+0.2% 🔺+0.2% 104.88 KB 105.11 KB 34.05 KB 34.11 KB UMD_PROD
react-dom.profiling.min.js +0.2% +0.2% 107.88 KB 108.11 KB 34.71 KB 34.77 KB UMD_PROFILING
react-dom.development.js +0.4% +0.4% 770.7 KB 773.53 KB 175.33 KB 176.06 KB NODE_DEV
react-dom.production.min.js 🔺+0.2% 🔺+0.2% 104.86 KB 105.09 KB 33.54 KB 33.6 KB NODE_PROD
react-dom.profiling.min.js +0.2% +0.2% 107.97 KB 108.2 KB 34.16 KB 34.21 KB NODE_PROFILING
ReactDOM-dev.js +0.4% +0.4% 793.5 KB 796.53 KB 176.45 KB 177.21 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.3% 🔺+0.4% 321.9 KB 322.89 KB 58.94 KB 59.15 KB FB_WWW_PROD
ReactDOM-profiling.js +0.3% +0.4% 327.83 KB 328.82 KB 60.21 KB 60.43 KB FB_WWW_PROFILING
react-dom-unstable-fire.development.js +0.4% +0.4% 776.66 KB 779.49 KB 176.98 KB 177.7 KB UMD_DEV
react-dom-unstable-fire.production.min.js 🔺+0.2% 🔺+0.2% 104.89 KB 105.12 KB 34.05 KB 34.12 KB UMD_PROD
react-dom-unstable-fire.profiling.min.js +0.2% +0.2% 107.9 KB 108.13 KB 34.72 KB 34.78 KB UMD_PROFILING
react-dom-unstable-fire.development.js +0.4% +0.4% 771.05 KB 773.88 KB 175.46 KB 176.2 KB NODE_DEV
react-dom-unstable-fire.production.min.js 🔺+0.2% 🔺+0.2% 104.87 KB 105.1 KB 33.55 KB 33.61 KB NODE_PROD
react-dom-unstable-fire.profiling.min.js +0.2% +0.2% 107.99 KB 108.22 KB 34.17 KB 34.22 KB NODE_PROFILING
ReactFire-dev.js +0.4% +0.4% 792.71 KB 795.74 KB 176.41 KB 177.16 KB FB_WWW_DEV
ReactFire-prod.js 🔺+0.3% 🔺+0.4% 310.44 KB 311.43 KB 56.56 KB 56.77 KB FB_WWW_PROD
ReactFire-profiling.js +0.3% +0.3% 316.46 KB 317.45 KB 57.88 KB 58.08 KB FB_WWW_PROFILING
react-dom-test-utils.production.min.js 0.0% 🔺+0.1% 10.27 KB 10.27 KB 3.8 KB 3.8 KB UMD_PROD
react-dom-test-utils.development.js 0.0% 0.0% 46.78 KB 46.78 KB 12.92 KB 12.92 KB NODE_DEV
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.61 KB 60.61 KB 15.92 KB 15.92 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 11.01 KB 11.01 KB 3.81 KB 3.81 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.28 KB 60.28 KB 15.79 KB 15.79 KB NODE_DEV
react-dom-server.browser.development.js +1.9% +1.9% 127.39 KB 129.86 KB 33.95 KB 34.61 KB UMD_DEV
react-dom-server.browser.production.min.js 🔺+1.6% 🔺+0.8% 18.89 KB 19.19 KB 7.22 KB 7.28 KB UMD_PROD
react-dom-server.browser.development.js +2.0% +2.0% 123.52 KB 125.99 KB 33.02 KB 33.68 KB NODE_DEV
react-dom-server.browser.production.min.js 🔺+1.6% 🔺+1.0% 18.81 KB 19.12 KB 7.21 KB 7.28 KB NODE_PROD
ReactDOMServer-dev.js +2.1% +1.9% 124.37 KB 127.04 KB 32.47 KB 33.09 KB FB_WWW_DEV
ReactDOMServer-prod.js 🔺+0.8% 🔺+1.6% 45.27 KB 45.62 KB 10.43 KB 10.6 KB FB_WWW_PROD
react-dom-server.node.development.js +2.0% +2.0% 125.58 KB 128.05 KB 33.56 KB 34.23 KB NODE_DEV
react-dom-server.node.production.min.js 🔺+1.5% 🔺+0.9% 19.69 KB 19.99 KB 7.52 KB 7.59 KB NODE_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.1% 1.21 KB 1.21 KB 705 B 706 B UMD_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.45 KB 3.45 KB 1.39 KB 1.39 KB NODE_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.2% 1.05 KB 1.05 KB 636 B 637 B NODE_PROD
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 3.7 KB 3.7 KB 1.42 KB 1.42 KB NODE_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.1% 1.1 KB 1.1 KB 667 B 668 B NODE_PROD

Generated by 🚫 dangerJS

@bloodyowl

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

@sebmarkbage would it be possible to add a prop to enable it for iframes, like the contentEditable suppress warning one? srcdoc support being quite bad, src="javascript:'some markup'" is the only other workaround available to polyfill

@facebook facebook deleted a comment from mayalo12 Mar 7, 2019

} else if (__DEV__) {
warning(
!isJavaScriptProtocol.test(url),
'A future version of React will block javascript: URLs as a security precaution. ' +

This comment has been minimized.

Copy link
@gaearon

gaearon Mar 11, 2019

Member

Can you dedupe these? Can get pretty noisy if you render a bunch of them.

This comment has been minimized.

Copy link
@gaearon

gaearon Mar 11, 2019

Member

Especially javascript:void(0) seems like it's still pretty common because it's copy pasted from old samples etc. Is it dangerous to whitelist that one? Is it a vector by itself?

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Mar 11, 2019

Author Member

I was thinking about that. I don't think it's a vector by itself. I guess you could use it to detect that navigation doesn't happen somehow.

If we allow it then reformatting it could start throwing in production so seems like a hazard.

I believe it is generally bad practice to rely on this pattern anyway (e.g. open in new tab and stuff) so this might be a good opportunity to teach alternative patterns.

Also, there are are Trusted Types and CSP rules that would have to mirror what we do, otherwise they'd fail too. Better off strictly enforcing it across the community.

@gaearon
Copy link
Member

left a comment

Need to fix Flow, I'd prefer to add deduping and strengthen tests a bit more

expect(e.href).toBe('http://javascript:0/thisisfine');
});

itRejects('a javascript protocol href', async render => {

This comment has been minimized.

Copy link
@gaearon

gaearon Mar 11, 2019

Member

It's really easy to miss the difference between itRenders and itRejects in this suite. It took me a while to notice it. Maybe rename to something like itRejectsDangerousInput?

resetModules();
});

runTests(

This comment has been minimized.

Copy link
@gaearon

gaearon Mar 11, 2019

Member

the metaprogramming intensifies

/* eslint-disable max-len */
const isJavaScriptProtocol = /^[\u0000-\u001F ]*j[\r\n\t]*a[\r\n\t]*v[\r\n\t]*a[\r\n\t]*s[\r\n\t]*c[\r\n\t]*r[\r\n\t]*i[\r\n\t]*p[\r\n\t]*t[\r\n\t]*\:/i;

function sanitizeURL(url: string) {

This comment has been minimized.

Copy link
@gaearon

gaearon Mar 11, 2019

Member

You're missing @flow on top. This isn't guaranteed to be a string (although I guess .test still works).

Might be worth adding tests for <a href={{ toString() { ... } }}>.

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Mar 11, 2019

Author Member

This should be guaranteed to be a string because we only call these after we've toStringed. That's intentional. To avoid toString returning different values to the first execution and the second. However, if you have a toString like that, you're probably p0wned anyway. So in the future we might only want to call this on things that are already strings to allow raw objects (like Trusted Types) to be passed through and validated by the DOM runtime.

);
});

// String SVG attributes with the xlink namespace.
[
'xlink:actuate',
'xlink:arcrole',
'xlink:href',

This comment has been minimized.

Copy link
@gaearon

gaearon Mar 11, 2019

Member

Would be nice if accidentally adding it back here would error or warn in dev. At least for our tests.

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Mar 11, 2019

Author Member

Meh. This whole file is the worst. Let's fix it in Fire.

// https://infra.spec.whatwg.org/#c0-control-or-space

/* eslint-disable max-len */
const isJavaScriptProtocol = /^[\u0000-\u001F ]*j[\r\n\t]*a[\r\n\t]*v[\r\n\t]*a[\r\n\t]*s[\r\n\t]*c[\r\n\t]*r[\r\n\t]*i[\r\n\t]*p[\r\n\t]*t[\r\n\t]*\:/i;

This comment has been minimized.

Copy link
@gaearon

gaearon Mar 11, 2019

Member

If someone accidentally adds g to this it would be pretty bad. Not that I actively expect it to happen but it would be nice to have a test that would fail if you recovered from the error and then tried it again.

This comment has been minimized.

Copy link
@mikesamuel

mikesamuel Apr 24, 2019

Why not just grab everything before the first [:/?#] and then strip out everything that isn't a valid protocol character per Std66 which I believes agrees with url.spec.whatwg:

scheme        = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )

So something like

function isJavascriptUrl(str) {
  return /^javascript$/i.test(  // Schemes are case insensitive
      /^(?:[^:/#?]*:)?/.exec(str)[0]  // Don't consider any part of any authority, path, query or fragment.
    .replace(/[^a-zA-Z0-9\+\-\.]+/g, '')  // Ignore non-scheme chars because Gareth Hayes
  );
}

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Apr 24, 2019

Author Member

Just intuition that this would be faster in the case where it's not a javascript URL, but I haven't actually benchmarked and I expect there to be a few alternatives benchmarked at some point. It's a fairly hot path since a lot of pages have many URLs.

This comment has been minimized.

Copy link
@mikesamuel

mikesamuel Apr 25, 2019

@sebmarkbage Fair enough. https://gist.github.com/mikesamuel/1b76779ec5206e258829914e2b0dec27 uses a fast path check so is more benchmarkable version of the same algo with comments explaining the derivation.

} else if (__DEV__) {
warning(
!isJavaScriptProtocol.test(url),
'A future version of React will block javascript: URLs as a security precaution. ' +

This comment has been minimized.

Copy link
@gaearon

gaearon Mar 11, 2019

Member

Especially javascript:void(0) seems like it's still pretty common because it's copy pasted from old samples etc. Is it dangerous to whitelist that one? Is it a vector by itself?

sebmarkbage added some commits Mar 6, 2019

Just warn when disableJavaScriptURLs is false
This avoids a breaking change.
Allow <html> to be used in integration tests
Full document renders requires server rendering so the client path
just uses the hydration path in this case to simplify writing these tests.
Detect leading and intermediate characters and test mixed case
These are considered valid javascript urls by browser so they must be
included in the filter.

This is an exact match according to the spec but maybe we should include
a super set to be safer?
Fix toString invocation and Flow types
Right now we invoke toString twice when we hydrate (three times
with the flag off). Ideally we should only do it once even in this case
but the code structure doesn't really allow for that right now.
Add test that fails if g is added to the sanitizer
This only affects the prod version since the warning is deduped anyway.

@sebmarkbage sebmarkbage force-pushed the sebmarkbage:javascripturls branch from d4be74e to be4f5c8 Mar 11, 2019

@sebmarkbage

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

@bloodyowl The escape hatch is to use dangerousSetInnerHTML or a ref and hope that you know what you're doing. :)

@sebmarkbage sebmarkbage merged commit 103378b into facebook:master Mar 11, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@mikesamuel

This comment has been minimized.

Copy link

commented Apr 24, 2019

How would this affect TrustedURL values whose content are javascript: values?

My understanding is that we don't need to cover these because they already don't execute javascript: URLs:

  • <base href>

I can execute script in modern Firefox & Safari with

<base href="javascript:alert(1)//">
<a href="#">click</a>

The link has the URL javascript:alert(1)//#.

  • <img src />

IIRC, this was a problem in IE 6 & 7 but I'm not sure about IE 8.

  • <img srcset />

This is correct, so there's no need to parse image candidate strings to filter javascript URLs in <img srcset="/bogus 999x, javascript:alert(1)"> and for the proposed <script srcset>, the attacker can just use an https URL.

@sebmarkbage

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

How would this affect TrustedURL values whose content are javascript: values?

Currently we "toString", non-string values before passing them to the DOM. This sanitization is applied to the resulting string. We couldn't do that with TrustedURL values. Instead, they'd pass through without sanitization so we wouldn't block them and would instead rely on the Trusted Types policy working.

An unresolved question is whether we should stop doing toString coercion completely and instead pass non-string values straight through. This would make it simple and fast to pass through TrustedURL values and we don't have to concern ourselves with any side-effects from toStringing. However, that means that objects with a toString method would be able to by-pass this check. Alternatively we could branch check the TrustedURL somehow (e.g. instanceof) to whitelist the objects that are allowed to pass through. It's a bit tricky to do this with polyfills, multiple realms etc. and it might be a growing list of object types that are allowed.

I can execute script in modern Firefox & Safari with

<base href="javascript:alert(1)//">
<a href="#">click</a>

Interesting! We already cover this since we cover all "href" attributes regardless of tag name but we should note this case, if that changes. I'll edit the PR description to make that clear.

<img src />
IIRC, this was a problem in IE 6 & 7 but I'm not sure about IE 8.

Officially, we don't really support either of these anymore. Not sure how deep we'd go into it. It also opens up related questions like executable expressions in style sheets.

@koto

This comment has been minimized.

Copy link

commented Apr 24, 2019

Kiku-Reise added a commit to Kiku-Reise/react that referenced this pull request May 16, 2019

Warn for javascript: URLs in DOM sinks (facebook#15047)
* Prevent javascript protocol URLs

* Just warn when disableJavaScriptURLs is false

This avoids a breaking change.

* Allow framesets

* Allow <html> to be used in integration tests

Full document renders requires server rendering so the client path
just uses the hydration path in this case to simplify writing these tests.

* Detect leading and intermediate characters and test mixed case

These are considered valid javascript urls by browser so they must be
included in the filter.

This is an exact match according to the spec but maybe we should include
a super set to be safer?

* Test updates to ensure we have coverage there too

* Fix toString invocation and Flow types

Right now we invoke toString twice when we hydrate (three times
with the flag off). Ideally we should only do it once even in this case
but the code structure doesn't really allow for that right now.

* s/itRejects/itRejectsRendering

* Dedupe warning and add the unsafe URL to the warning message

* Add test that fails if g is added to the sanitizer

This only affects the prod version since the warning is deduped anyway.

* Fix prod test
@apalm apalm referenced this pull request Jul 30, 2019
@or-else

This comment has been minimized.

Copy link

commented Aug 14, 2019

@mikesamuel

<a href="#" onclick={e => e.preventDefault()}>

That would replace the current URL fragment with an empty fragment which is not a desirable outcome when the app uses fragments for in-app navigation.

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

@or-else not with the preventDefault; that prevents the default behavior of altering the location in any way.

@martinmckenna martinmckenna referenced this pull request Aug 14, 2019

or-else added a commit to tinode/webapp that referenced this pull request Aug 15, 2019

@gaearon

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

@loopmode Yes, for your case dangerouslySetInnerHTML sounds like it would do the job.

@loopmode

This comment has been minimized.

Copy link

commented Aug 15, 2019

@gaearon Yes, at least almost :) if done on a parent element, I guess it can be done with dangerouslySetInnerHTML, but not on the <a> itself (because href, an attribute, is probably unavailable via innerHTML)

I think I'll go with a custom component: https://www.npmjs.com/package/@loopmode/react-link

@lukescott

This comment has been minimized.

Copy link

commented Aug 15, 2019

I understand the motivation behind this and I appreciate it. However, it seems the warning does nothing to actually prevent the vulnerability, unless you turn the option on. A variable is likely not supposed to have javascript: in it, so this won't be caught in development. This warning simply forces you to replace each and every static case.

The recommendation to replace it with # and preventDefault sounds simple on the surface, but:

  • Many apps have hundreds if not thousands of these cases. Each handler is unique, and it requires time and effort. This is not a simple find and replace. There is a lot of friction here.
  • If onClick throws, # causes the url to change. This is one of the reasons people prefer javascript:; over # in the first place. The "workaround" has always existed and if was a better solution people would be using that instead.

What is the benefit of forcing people to change this? Why not allow static cases of javascript:; and javascript:void(0)?

The only way I see making this transition easier is by making onClick call preventDefault for you by default. Then it would be a quick find and replace. We already have an onChange that acts differently from the browser anyway. Although... I'm not sure if that breaks any code in the wild or not.

@emileber

This comment has been minimized.

Copy link

commented Aug 15, 2019

@lukescott

Many apps have hundreds if not thousands of these cases.

They shouldn't use anchors as buttons and I don't feel React should limit its evolution to these wrong use-cases.

If onClick throws, # causes the url to change.

That's not true if you called .preventDefault() before doing anything dangerous.

I honestly never needed javascript:void(0) in any frontend applications I've worked on.

@gaearon

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

However, it seems the warning does nothing to actually prevent the vulnerability, unless you turn the option on. A variable is likely not supposed to have javascript: in it, so this won't be caught in development. This warning simply forces you to replace each and every static case.

Yes, because you shouldn't have dynamic cases at all. (Unless you're happy with security holes.) You're right the warning doesn't prevent anything today. The warning is to help you migrate the static cases before we turn on the enforcement in a future major. And if you truly need dynamic cases with javascript: URLs (highly not recommended), dangerouslySetInnerHTML is your friend.

Many apps have hundreds if not thousands of these cases. Each handler is unique, and it requires time and effort. This is not a simple find and replace. There is a lot of friction here.

Can you share how many exactly you see in your React apps? Why is this pattern prevalent in your code? Some examples would help.

@sebmarkbage

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

One of the things that convinced me is that after talking to experts on things like a11y and ways to simulate a button, they all seemed to agree that the javascript url pattern was a bad way of accomplishing this effect. It’s also not how infrastructure at large companies that have evaluated this seem to do it. So it seems like as an industry we just haven’t learned the correct way of achieving this effect with the need for href.

@lukescott

This comment has been minimized.

Copy link

commented Aug 15, 2019

I agree that buttons would be more appropriate. I'm working on an existing application that has around 300 instances of this pattern. The why is unclear. Perhaps someone liked the look of links more over buttons? At the moment I'm trying to figure out the transition plan with our team. I've put out a general message about the change, and to fix it where we see it. It's just not something we can fix all at once.

@gaearon

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

@lukescott Do you not have a Link component that encapsulates it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.