WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 7 weeks ago

#39941 accepted enhancement

Allow using Content-Security-Policy without unsafe-inline

Reported by: tomdxw Owned by: johnbillion
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.8
Component: Security Keywords: has-patch needs-refresh
Focuses: javascript Cc:

Description

Currently when using Content-Security-Policy with WordPress, you must use the unsafe-inline directive because there are a lot of blocks of inline JavaScript in WordPress core. This means that the browser cannot protect the user from attacks using XSS vulnerabilities. This is an unsatisfying situation because XSS vulnerabilities can be found in a great number of WordPress plugins.

The patch I’m providing today makes it possible to write a plugin which uses CSP without unsafe-inline. Such a plugin would make the vast majority of XSS vulnerabilities found in WP plugins useless to an attacker.

I’ve just added one new function: inline_js(). Now instead of writing <script>doSomething()</script>, you would write <?php inline_js(‘doSomething()’) ?>.

I’ve changed enough instances of inline JavaScript to use inline_js() that you can try it out:

I’ve only changed some instances of inline JavaScript in this patch - enough to prove that it will work.

Attachments (1)

inline-js-patch.diff (14.6 KB) - added by tomdxw 2 years ago.

Download all attachments as: .zip

Change History (33)

#1 @swissspidy
2 years ago

How does this change anything? inline_js() still prints <script></script> tags. Because of that, I think it's really a duplicate of #32067.

#2 @johnbillion
2 years ago

  • Keywords reporter-feedback added

Thanks for the patch, Tom.

The resulting output of inline_js() still includes inline <script> ... </script> tags. Can you let us know how this allows a CSP without unsafe-inline to be implemented?

#3 @johnbillion
2 years ago

Ah sorry, I didn't click through to your plugin. The nonce attribute approach is interesting. Is this widely supported by browsers? Can the same nonce be re-used for all script tags?

Last edited 2 years ago by johnbillion (previous) (diff)

#4 @tomdxw
2 years ago

Sorry, I should have explained better.

The CSP specification ( https://www.w3.org/TR/CSP2/ ) has two ways of allowing inline JavaScript: hashes and nonces.

I looked at hashes, but they would have required much larger changes to WordPress. And it would also require calculating multiple hashes on each page load which would have slowed the page down a small amount.

But with nonces you add a header like this:

Content-Security-Policy: script-src 'nonce-123abc'

And then whenever you use inline JavaScript, you add a nonce attribute to the script element:

<script nonce="123abc">
doSomething()
</script>

And when the browser encounters a script tag with the wrong nonce (or no nonce), it refuses to execute that JavaScript.

These nonces function in pretty much the same way as WordPress's nonces: so long as the attacker doesn't know what they are, they can't execute JavaScript. So when a plugin author writes <input value="<?php echo $_GET['x'] ?>">, an attacker isn't able to inject their own JavaScript because they don't know what the nonce is.

Is this widely supported by browsers?

Chrome and Firefox support it. Edge 38 (the current version) doesn't support CSP nonces. But Edge 39 does.

Here's the caniuse page: http://caniuse.com/#feat=contentsecuritypolicy2

Here's a very basic test page (if everything works you will see one alert box; if CSP isn't supported at all you will see two alert boxes; and if CSP is supported but nonces aren't, you won't see any alert boxes): https://cdn.rawgit.com/tomdxw/95a22a1be010b2d07152be6b3f635fa1/raw/039d6fe0876dfc0c689be0f5787c038d1f27f5d5/nonce-test.html

Can the same nonce be re-used for all script tags?

That's correct, yes. In the proof-of-concept plugin it uses the same nonce for every script tag.

#5 @tomdxw
2 years ago

  • Focuses javascript added

#6 @tomdxw
2 years ago

  • Keywords reporter-feedback removed

#7 @knutsp
22 months ago

+1

Hard to secure a site using CSP with inline scripts and without a nonce on them.

#8 @maltris
21 months ago

+1

The earlier this is included, the better. Just fell in that pit.

#9 @herp
18 months ago

+1

Inpossible to get good security ratings with a CSP containing 'unsafe-inline'

#10 @johnbillion
16 months ago

  • Milestone changed from Awaiting Review to 5.0
  • Owner set to johnbillion
  • Status changed from new to accepted

#11 @jrchamp
16 months ago

@tomdxw I really like the idea here for inline. Can we extend it to external scripts too? 'strict-dynamic' seems to be the gold standard. https://www.w3.org/TR/CSP3/#strict-dynamic-usage

#12 @maestr055
14 months ago

+1
I'm busy with hardening my Wordpress sites. What is the status of this proposed change?

#13 @RagnarKarlsson
12 months ago

Has consideration been made to hook this, so that nonces can be included within security plugins (such as Ninjafirewall) which set a full CSP? Adding the header with just the script-src element as per the example plugin would be overwritten if a subsequent csp is defined in a second header.

#14 @giuse
12 months ago

How is it possible to include nonces for inline scripts? I haven't found any filter to do that. Is it possible with the current WordPress version? If not so, it would be great if WordPress gives the possibility to filter the output of <script type="text/javascript"> for inline scripts.

#15 @jadeddragoon
10 months ago

Doesn't this "solution" completely undermine CSP? If an attacker successfully injects script into a page won't the filter helpfully add a nonce to it? At that point why not just disable CSP entirely?

#16 follow-up: @giuse
10 months ago

I suppose you mean if an attacker is able to injects a script using a WordPress function as wp_add_inline_script. In that case no CSP can help, if an attacker was able to do that, he can do what he wants. Or what do you mean? Of course the filter has to work only if the scripts are introduced by a WordPress function, in no other cases.

#17 in reply to: ↑ 16 @jadeddragoon
10 months ago

Replying to giuse:

In that case no CSP can help, if an attacker was able to do that, he can do what he wants.

??? That's exactly the kind of case CSP is meant to stop. Why wouldn't it help?

If you mean that, at that point, an attacker could introduce their own CSP policy via <meta http-equiv="Content-Security-Policy" then this, while true, has no bearing here. As per the current CSP documentation here the most restrictive policy for a given directive "wins". This is because all policies are enforced and any use of feature matching a directive must pass all defined policies.

So if I configure a strong CSP header that doesn't allow inline scripts and styles and lists no nonces or hashes and then an attacker manages to inject a CSP of his own via meta tags with unsafe-inline (or a hash... or a nonce) for the injected code... his code still won't run. It will pass the policy he injected but fail the original policy... and it must pass all policies.

Last edited 10 months ago by jadeddragoon (previous) (diff)

#18 @johnbillion
7 months ago

  • Milestone changed from 5.0 to 5.1

#19 @swissspidy
5 months ago

#45531 was marked as a duplicate.

#20 @pento
4 months ago

  • Milestone changed from 5.1 to 5.2

#21 @alternativehosting
3 months ago

+1

Really need this to properly lock down my site.

#22 @desrosj
2 months ago

  • Keywords has-patch needs-refresh added
  • Milestone changed from 5.2 to 5.3

@johnbillion are you still interested in shepherding this one?

Since we are a little less than 2 days away from 5.2 beta 1 and this has not seen progress in several months, I am going to punt.

#23 @johnbillion
2 months ago

Yep I'll see if I can pick this up for 5.3.

#24 follow-up: @jadeddragoon
2 months ago

I really must re-iterate that this appears to be an end-run around CSP. It will not make WP sites more secure and, arguably, will make them less secure by giving site operators and visitors a false sense of security. WP should be made compliant with strong CSP policies... not try to defeat them.

Blocking "unsafe-inline" javascript via CSP exists explicitly to close one common vector attackers use to inject malicious code into a page by making sure any code that might be have been injected never gets executed by client browsers. The CSP is (when done correctly) set in the httpd... independent of the site code. It is enforced by the client browser. What this patch does is allow this vulnerability to remain open on wordpress sites without users being made aware of it by tricking the client browser into believing the code has been validated by the site operator when it has not.

Last edited 2 months ago by jadeddragoon (previous) (diff)

#25 in reply to: ↑ 24 ; follow-up: @jrchamp
2 months ago

Replying to jadeddragoon:

The CSP is (when done correctly) set in the httpd... independent of the site code.

Only the application itself will know the elements that need to be allowed, so only the application can accurately set the CSP. The best strict CSP example uses strict-dynamic and a random nonce which must be provided to each permitted script tag embedded in the page. This allows the browser to differentiate between the scripts that were sent by the server and those that were injected via strings.

#26 in reply to: ↑ 25 @jadeddragoon
2 months ago

Replying to jrchamp:

Replying to jadeddragoon:

The CSP is (when done correctly) set in the httpd... independent of the site code.

Only the application itself will know the elements that need to be allowed,

If you don't use inline javascript at all, then you will know exactly which elements need nonces: none of them. And if the javascript is inline but static then you can configure the CSP to use checksums at deploy time instead.

All javascript should be moved to separate files. The javascript itself should be static... not templated via php... and thus not at risk of code injection via php. This restiction then allows features of CSP (such as strict-dynamic) to work as intended. Then the needed CSP policy would be known at deploy time and can be set in an independent application (httpd), applying the principle of separation of duties.

I think people may actually be forgetting that the "scripts" component of CSP is referring to client side code only. Because the CSP is enforced client side it can't tell how or in what way the php is loading scripts or really even that php was used... and thus strict-dynamic cannot do anything to protect users from poor implementations.

In the examples given by the very page you linked to... only the nonce is templated... not the javascript. The javascript is static. I see nothing on this site that recommends the usage you claim it does such as when, like in the case of wordpress, the javascript is templated on the server side before being sent to the client.

If no one wants to put the time and effort into refactoring WP to allow this... fine. Make it clear that people need to allow unsafe-inline to use wordpress and accept the security hit... don't disguise the security risk like this. It's unethical.

Last edited 2 months ago by jadeddragoon (previous) (diff)

#27 follow-up: @mallorydxw
2 months ago

@jadeddragoon

It will not make WP sites more secure

Explain how. tbh it seems to me like you're making perfect the enemy of good.

Most XSS vulnerabilities in WordPress plugins are _not_ found inside SCRIPT tags. So by using CSP nonces we'll be preventing the majority of XSS vulnerabilities.

I agree it would be best to load all JS from .js files and not use any inline JS, but that's a gargantuan task for WordPress (and forget about convincing plugin devs to do the same). I opened this ticket as a compromise which is a much easier sell (to WordPress devs and to plugin devs) while still preventing the majority of XSS vulnerabilities found in plugins.

#28 in reply to: ↑ 27 @jadeddragoon
2 months ago

Replying to mallorydxw:

@jadeddragoon

It will not make WP sites more secure

Explain how.

Because it simultaneously provides a mechanism for evading the CSP entirely.

Hypothetical:

Plugin Developer A writes a plugin that templates JavaScript. To do this he uses the proposed function to generate nonces. However, the way the JavaScript in question is templated by the PHP of his plugin changes depending on user input and the developer fails to properly sanitize that input (this is a bog standard XSS scenario).

Without this patch the site operator would have to configure a CSP with unsafe-inline. The user and the user's client could therefore be warned of the risk of XSS when using said plugin. As CSP is adopted more and more eventually Google, Mozilla, Microsoft, Apple and the rest of the browser developers could add unsafe-inline to the warnings shown in the address bar specifically to let users know when they are at risk of having their personally identifiable information exposed.

With this patch, however, a malicious user could input specially formatted PHP code into the poorly sanitized inputs with the intent of injecting XSS JavaScript in the associated output, and your proposed patch would ensure that the user and the user's browser was told that this XSS JS code is trustworthy. And, as CSP is adopted more and more... when browser developers make the switch to warning users WordPress sites will be lying to the user about their level of risk.

CSP is designed specifically to reduce the risk to end users from poorly sanitized inputs and unintended connections between user input and html/css/js output. But if you go marking all of that output as trusted without knowing exactly what that output will be... then CSP can't work. How is that not obvious?

It doesn't matter if the exploitable code is in the JS itself or the PHP behind it. The reason inline JS is unsafe is exactly because there's no way to tell what inline JS is intended to be there and what inline JS is the result of an exploit. If inline JS is disallowed entirely then any inline JS is obviously not supposed to be there. The use of nonces to mark inline JS as safe is intended for use with carefully vetted, _static_ JavaScript.

But with your patch all inline JS templated via PHP will get marked as "supposed to be there" before being sent to the user... meaning that there is still no way to tell what inline JS is actually supposed to be there and what isn't. If the PHP turns out to be exploitable then malicious JS can be marked as trusted. This is functionally identical to using unsafe-inline in the CSP to begin with... except, again, the user isn't made aware of the issue.

I'm aware of just what a gargantuan task it would be to convert WordPress to use static JavaScript separated from the html. ::shrug:: That's what happens when you find yourself managing an application or system that defies best practices. Maybe it wasn't best practice five years ago... cry me a river. As a site operator my responsibility is to my users. Not WordPress. And one of the objectives of CSP adoption is to force software developers to stop using unsafe practices. Refactor WordPress... leave the plugin developers to justify doing otherwise to their users.

Or don't. Go ahead and make it official that WordPress requires running with unsafe-inline and the totally-not-a-problem isn't going to be solved. Hell, I'll even update the documentation myself. But this patch, well intentioned or not, is categorically The Wrong Way from a security standpoint.

Bottom line is bottom line: this patch is identical in security risk to running with unsafe-inline except that it hides the problem. And no one in this thread can say they weren't warned.

Last edited 2 months ago by jadeddragoon (previous) (diff)

#29 @mallorydxw
2 months ago

With this patch, however, a malicious user could input specially formatted PHP code into the poorly sanitized inputs with the intent of injecting XSS JavaScript in the associated output

But with your patch all inline JS templated via PHP will get marked as "supposed to be there" before being sent to the user... meaning that there is still no way to tell what inline JS is actually supposed to be there and what isn't.

Correction: this is not part of the WordPress patch I made. This is part of the proof-of-concept plugin I made.

If the server sends Content-Security-Policy: script-src 'nonce-123abc' then the client will only execute scripts if the opening script tag contains nonce="123abc". This example would be impossible unless the attacker was able to guess the nonce value.

#30 follow-up: @mallorydxw
2 months ago

By the way, the proof-of-concept plugin I mentioned in the description of the report is here now as I changed my github username: https://gist.github.com/mallorydxw/e2aee45ad5cb2a309c6bd0fc213efb97

#31 in reply to: ↑ 30 ; follow-up: @jadeddragoon
2 months ago

Replying to mallorydxw:

If the server sends Content-Security-Policy: script-src 'nonce-123abc' then the client will only execute scripts if the opening script tag contains nonce="123abc". This example would be impossible unless the attacker was able to guess the nonce value.

That would be true if the JavaScript was not templated via PHP. But client-enforced CSP cannot see what's happening in the PHP code on the server. I already explained how this works in my last post. By templating JS via PHP wordpress does and has always provided a means of JS injection. Because templating === injection. This is why WordPress has such a bad reputation for XSS exploits. And you're providing a means to make sure all the templated JS has valid nonces. That means that if someone manages to insert their own code into the templated JS by exploiting poorly formed PHP... the XSS JS code will be in a script tag that has a valid nonce. You're actually removing the need for the attacker to guess the nonce by adding it for them.

Replying to mallorydxw:

By the way, the proof-of-concept plugin I mentioned in the description of the report is here now as I changed my github username: https://gist.github.com/mallorydxw/e2aee45ad5cb2a309c6bd0fc213efb97

This would be even worse! With this they don't even have to find templated js that explicitly requests a nonce nor request one themselves... even existing WordPress XSS exploits can take advantage and future exploits don't have to ask for the nonce specifically.

Last edited 2 months ago by jadeddragoon (previous) (diff)

#32 in reply to: ↑ 31 @alinod
7 weeks ago

Replying to jadeddragoon:

Replying to mallorydxw:

If the server sends Content-Security-Policy: script-src 'nonce-123abc' then the client will only execute scripts if the opening script tag contains nonce="123abc". This example would be impossible unless the attacker was able to guess the nonce value.

That would be true if the JavaScript was not templated via PHP. But client-enforced CSP cannot see what's happening in the PHP code on the server. I already explained how this works in my last post. By templating JS via PHP wordpress does and has always provided a means of JS injection. Because templating === injection. This is why WordPress has such a bad reputation for XSS exploits. And you're providing a means to make sure all the templated JS has valid nonces. That means that if someone manages to insert their own code into the templated JS by exploiting poorly formed PHP... the XSS JS code will be in a script tag that has a valid nonce. You're actually removing the need for the attacker to guess the nonce by adding it for them.

Replying to mallorydxw:

By the way, the proof-of-concept plugin I mentioned in the description of the report is here now as I changed my github username: https://gist.github.com/mallorydxw/e2aee45ad5cb2a309c6bd0fc213efb97

This would be even worse! With this they don't even have to find templated js that explicitly requests a nonce nor request one themselves... even existing WordPress XSS exploits can take advantage and future exploits don't have to ask for the nonce specifically.

I sincerely hope that people are actually listening to @jadeddragoon here. The objective here is not to get a good security rating, but just plain better security. Automatically flagging all inline JS as safe so that you can remove the unsafe-inline is no more secure than having the 'unsafe-inline' directive. And it is actually worse because it hides the vulnerability.

It's like Volkswagen designing cars that modify their behaviour during emission tests to get a clean rating.

You can't make a system more secure by hiding its weaknesses. Furthermore, it removes the incentive for actually addressing the underlying problem because they are no longer getting the warnings that CSP was designed for.

Note: See TracTickets for help on using tickets.