Warn for javascript: URLs in DOM sinks #15047
Conversation
|
ReactDOM: size: 🔺+0.2%, gzip: 🔺+0.2% Details of bundled changes.Comparing: 5d0c3c6...c8979c0 react-dom
Generated by 🚫 dangerJS |
|
@sebmarkbage would it be possible to add a prop to enable it for iframes, like the contentEditable suppress warning one? |
| } else if (__DEV__) { | ||
| warning( | ||
| !isJavaScriptProtocol.test(url), | ||
| 'A future version of React will block javascript: URLs as a security precaution. ' + |
There was a problem hiding this comment.
Can you dedupe these? Can get pretty noisy if you render a bunch of them.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| expect(e.href).toBe('http://javascript:0/thisisfine'); | ||
| }); | ||
|
|
||
| itRejects('a javascript protocol href', async render => { |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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() { ... } }}>.
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
Would be nice if accidentally adding it back here would error or warn in dev. At least for our tests.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
);
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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. ' + |
There was a problem hiding this comment.
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?
Full document renders requires server rendering so the client path just uses the hydration path in this case to simplify writing these tests.
This avoids a breaking change.
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?
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.
This only affects the prod version since the warning is deduped anyway.
sebmarkbage
force-pushed
the
javascripturls
branch
from
d4be74e
to
be4f5c8
Compare
|
@bloodyowl The escape hatch is to use dangerousSetInnerHTML or a ref and hope that you know what you're doing. :) |
|
How would this affect TrustedURL values whose content are
I can execute script in modern Firefox & Safari with <base href="javascript:alert(1)//">
<a href="#">click</a>The link has the URL
IIRC, this was a problem in IE 6 & 7 but I'm not sure about IE 8.
This is correct, so there's no need to parse image candidate strings to filter javascript URLs in |
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.
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.
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. |
|
For the check, you could use TrustedTypes.isURL function, it has stronger
guarantees than an instance of check (bypassable by e.g. Object.create). A
bypass in this model would be either a user-created polyfill for TT API or
a user-created policy, but both of those sound like intentional behavior by
a developer (they could just as well monkey patch DOM).
+1 for not toStringifying objects that pass TT.isURL check.
…On Wed, Apr 24, 2019, 21:56 Sebastian Markbåge ***@***.***> wrote:
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.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#15047 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAA7JKYPHFDDUKMYEDMUWZLPSC3QDANCNFSM4G4JEJ5Q>
.
|
ghost
mentioned this pull request
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.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
SVG
The
<script src />case is inherently broken since it executes script anyway.We don't validate
<script text />,ref.innerHTMLand<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: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:PR
For code reviewers: The first two commits just enables us to write tests against
frameset.