WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 6 weeks ago

#39941 accepted enhancement

Allow using Content-Security-Policy without unsafe-inline

Reported by: tomdxw Owned by: johnbillion
Milestone: Future Release Priority: normal
Severity: normal Version: 4.8
Component: Security Keywords: has-patch needs-refresh 2nd-opinion
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 (38)

#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
2 years ago

+1

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

#8 @maltris
2 years ago

+1

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

#9 @herp
21 months ago

+1

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

#10 @johnbillion
19 months ago

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

#11 @jrchamp
19 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
17 months ago

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

#13 @RagnarKarlsson
15 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
15 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
13 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
13 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
13 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 13 months ago by jadeddragoon (previous) (diff)

#18 @johnbillion
10 months ago

  • Milestone changed from 5.0 to 5.1

#19 @swissspidy
9 months ago

#45531 was marked as a duplicate.

#20 @pento
7 months ago

  • Milestone changed from 5.1 to 5.2

#21 @alternativehosting
6 months ago

+1

Really need this to properly lock down my site.

#22 @desrosj
5 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
5 months ago

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

#24 follow-up: @jadeddragoon
5 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 5 months ago by jadeddragoon (previous) (diff)

#25 in reply to: ↑ 24 ; follow-up: @jrchamp
5 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
5 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 5 months ago by jadeddragoon (previous) (diff)

#27 follow-up: @mallorydxw
5 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
5 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 intended to inject XSS JavaScript into the poorly sanitized inputs, 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.

Version 8, edited 5 months ago by jadeddragoon (previous) (next) (diff)

#29 @mallorydxw
5 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
5 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
5 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 5 months ago by jadeddragoon (previous) (diff)

#32 in reply to: ↑ 31 ; follow-up: @alinod
5 months 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.

#33 @johnbillion
3 months ago

  • Keywords 2nd-opinion added
  • Milestone changed from 5.3 to Future Release

Bumping only because I likely won't have the time to read and consider the concerns raised in this thread in time for 5.3.

#34 in reply to: ↑ 32 ; follow-up: @epicfaace
6 weeks ago

What if we instead created a nonce that is only used for scripts loaded from WordPress Core, and thus cannot be accessed by the developer for use by arbitrary scripts? What do you think, @jadeddragoon and others?

#35 in reply to: ↑ 34 @alinod
6 weeks ago

Replying to epicfaace:

What if we instead created a nonce that is only used for scripts loaded from WordPress Core, and thus cannot be accessed by the developer for use by arbitrary scripts? What do you think, @jadeddragoon and others?

It seems to me that you might be missing the definition of a nonce. A nonce is a number used only once. The MDN reference explains it pretty well:

'nonce-<base64-value>'
A whitelist for specific inline scripts using a cryptographic nonce (number used once). The server must generate a unique nonce value each time it transmits a policy. It is critical to provide an unguessable nonce, as bypassing a resource’s policy is otherwise trivial. See unsafe inline script for an example. Specifying nonce makes a modern browser ignore 'unsafe-inline' which could still be set for older browsers without nonce support.

Therefore a new random value must be generated for each inline script and it must be unique across page requests. In addition, a CSP header must be issued that contains the exact same nonce that was randomly generated for each inline script. Without these controls, the CSP is not providing any protection since it would be trivial to circumvent. Clearly, if nonces are used correctly, any kind of page caching would break the site.

Hashes are a much better choice. However, it is imperative that the hash be a static value that only changes when upgrading to a new revision of WordPress; calculating the hash at runtime based on the existing content of the inline script is no safer than using 'unsafe-inline'. We would essentially be telling the browser to trust everything in the inline script, regardless of whether it is malicious or not. Worse, this would mask the underlying vulnerability and give users a false sense of security.

Security practitioners like to call this "security theatre" and for good reason.

Last edited 6 weeks ago by alinod (previous) (diff)

#36 follow-up: @epicfaace
6 weeks ago

@alinod Good point about nonces and caching. Of course, nonces are already commonly used in items such as forms on WordPress, and thus already prevent caching for those pages -- but adding nonces to every single page would end up negating a lot of the effect of caching, as you said.

So it seems like the two options are 1) calculating static hashes for all inline scripts used in WordPress core, adding a build process to add these in to the source code or 2) switching all of WP's inline JS to external JavaScript instead. At this point, it seems like the latter might be simpler. What are the main challenges with doing so?

#37 in reply to: ↑ 36 @alinod
6 weeks ago

Replying to epicfaace:

So it seems like the two options are 1) calculating static hashes for all inline scripts used in WordPress core, adding a build process to add these in to the source code or 2) switching all of WP's inline JS to external JavaScript instead. At this point, it seems like the latter might be simpler. What are the main challenges with doing so?

I'm not sure whether you were addressing this last part to me, or not. I would certainly agree that the latter is preferable. Unfortunately, I'm not well versed enough in WordPress core to assess the level of difficulty in doing so. My assumption was that if it were easy, someone would have done it already... but I'd gladly be wrong on that point.

Note: See TracTickets for help on using tickets.