Make WordPress Core

Opened 7 years ago

Closed 3 years ago

Last modified 6 months ago

#39941 closed enhancement (fixed)

Allow using Content-Security-Policy without unsafe-inline

Reported by: tomdxw's profile tomdxw Owned by: adamsilverstein's profile adamsilverstein
Milestone: 5.7 Priority: normal
Severity: normal Version: 4.8
Component: Security Keywords: has-patch has-unit-tests commit has-dev-note
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 (7)

inline-js-patch.diff (14.6 KB) - added by tomdxw 7 years ago.
39941.diff (13.5 KB) - added by adamsilverstein 3 years ago.
39941.2.diff (13.4 KB) - added by adamsilverstein 3 years ago.
39941.3.diff (13.3 KB) - added by adamsilverstein 3 years ago.
39941.4.diff (13.4 KB) - added by adamsilverstein 3 years ago.
39941.5.diff (13.4 KB) - added by adamsilverstein 3 years ago.
39941.6.diff (13.5 KB) - added by adamsilverstein 3 years ago.

Download all attachments as: .zip

Change History (121)

#1 @swissspidy
7 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
7 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
7 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 7 years ago by johnbillion (previous) (diff)

#4 @tomdxw
7 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
7 years ago

  • Focuses javascript added

#6 @tomdxw
7 years ago

  • Keywords reporter-feedback removed

#7 @knutsp
7 years ago

+1

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

#8 @maltris
7 years ago

+1

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

#9 @herp
6 years ago

+1

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

#10 @johnbillion
6 years ago

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

#11 @jrchamp
6 years 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
6 years ago

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

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

#18 @johnbillion
5 years ago

  • Milestone changed from 5.0 to 5.1

#19 @swissspidy
5 years ago

#45531 was marked as a duplicate.

#20 @pento
5 years ago

  • Milestone changed from 5.1 to 5.2

#21 @alternativehosting
5 years ago

+1

Really need this to properly lock down my site.

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

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

#24 follow-up: @jadeddragoon
5 years 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 such injected code 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.

Version 2, edited 5 years ago by jadeddragoon (previous) (next) (diff)

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

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

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

#32 in reply to: ↑ 31 ; follow-up: @alinod
5 years 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
5 years 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
5 years 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
5 years 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 5 years ago by alinod (previous) (diff)

#36 follow-up: @epicfaace
5 years 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
5 years 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.

#38 @ocean90
4 years ago

#49812 was marked as a duplicate.

This ticket was mentioned in PR #498 on WordPress/wordpress-develop by enricocarraro.


4 years ago
#39

  • Keywords has-unit-tests added; needs-refresh removed

I introduced two new functions that print <script> tags and enable attribute injection:

  • wp_print_script_loader_tag: for script tags that load JavaScript files through the src attribute;
  • wp_print_inline_script _tag: for inline scripts.

Both these functions filter the attributes passed to them through wp_script_attributes so that plugins can change script attributes in a controlled manner.

Instead of directly printing <script> tags, these functions should be used to ensure that every <script> tag is controllable.

I also included the refactoring of every script tag; now they are printed by either wp_print_inline_script_tag or wp_print_script_loader_tag. I followed this guide on adopting CSP, considering the strict-dynamic option enabled.
I made a plugin prototype that enables Strict CSP to test these changes.

Trac ticket: https://core.trac.wordpress.org/ticket/39941

#40 @enricocarraro
4 years ago

I picked up the work made by @tomdxw and adapted it to the current version of WordPress.

A plugin can now control every attribute of every script printed by PHP, but there are still three things to do to make WordPress fully compatible with Strict CSP:

  • refactor calls to document.write('<script...'), used to load additional scripts, to use document.createElement('script'), inside JavaScript files;
  • refactor inline event handlers and javascript URIs;
  • add to the test suite a rule that checks that scripts are printed only using either wp_print_inline_script_tag or wp_print_script_loader_tag.

These last points would be part of #32067, and could be addressed as a follow-up to this PR; more details on #32067.

Last edited 4 years ago by enricocarraro (previous) (diff)

This ticket was mentioned in Slack in #core by laura. View the logs.


3 years ago

#42 @johnbillion
3 years ago

  • Owner johnbillion deleted
  • Status changed from accepted to assigned

This ticket was mentioned in Slack in #core-js by enricocarraro. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-js by enricocarraro. View the logs.


3 years ago

#45 follow-up: @swissspidy
3 years ago

  • Keywords dev-feedback added; 2nd-opinion removed

@helen @whyisjake Any objections to getting https://github.com/WordPress/wordpress-develop/pull/498 in 5.6? No breaking change, but paves the way for people to use Content-Security-Policy headers if they want to.

#46 @adamsilverstein
3 years ago

@swissspidy That PR is a bit difficult to review and test with over 50 files changed!

Can we review and commit the core functionality first? That is the new incline script helpers - https://github.com/WordPress/wordpress-develop/pull/498/files#diff-6f2c2b01200cf58d447b9d3d63027da5R7636-R7738.

Then we can address testing and changing all core usage, taking on that project one file or feature at a time. We started this work on https://core.trac.wordpress.org/ticket/51407#comment:4

#47 @swissspidy
3 years ago

Yes that would be a step by step thing of course. Just wanted to give a heads up

#48 @enricocarraro
3 years ago

Currently the function responsible sanitizing script attributes does not sanitize attribute names, but it escapes attribute values.
Do you think it would be necessary to sanitize attribute names?

Last edited 3 years ago by enricocarraro (previous) (diff)

#49 @enricocarraro
3 years ago

I would also like to know your opinion on whether or not the src escaping should be delegated to wp_santitize_script_attributes.
Reasons to escape inside wp_santitize_script_attributes:

  • The function already escapes the other attributes;
  • Developers wouldn't have to worry about escaping the value of any attribute before passing it.

Reasons not to:

  • Gives developers less control over the attribute value;
  • esc_url() doesn't support relative URLs.

This ticket was mentioned in PR #591 on WordPress/wordpress-develop by enricocarraro.


3 years ago
#50

I introduced two new functions that print <script> tags and enable attribute injection:

  • wp_print_script_tag and wp_get_script_tag: for script tags that load JavaScript files through the src attribute;
  • wp_print_inline_script_tag and wp_get_inline_script_tag: for inline scripts.

All these functions filter the attributes passed to them through wp_script_attributes so that plugins can change script attributes in a controlled manner.

Instead of directly printing <script> tags, these functions should be used to ensure that every <script> tag is controllable.

Trac ticket: https://core.trac.wordpress.org/ticket/39941

#51 @enricocarraro
3 years ago

I added a third PR in which only the new <script> tag formatting functions are present.
I also rebased the PR with the inline script refactoring so that every commit is only about one single page/component.

adamsilverstein commented on PR #591:


3 years ago
#52

@enricocarraro Thank you for working on this!

I left a few comments and questions. It would be good explain what the filters do in their inline docs, and also to add a test that covers the wp_get_inline_script_tag action (the test could add add an action and verify it was called with the correct data).

Since this is a bit complex to use it will definitely need a "Dev Note" that explains how the API can be used, including stuff like this: https://github.com/WordPress/wordpress-develop/pull/498/files#r499066268.

Would you say merging this PR fixes https://core.trac.wordpress.org/ticket/39941?

enricocarraro commented on PR #591:


3 years ago
#53

@adamsilverstein thank you for taking the time to look at this.

I left a few comments and questions. It would be good explain what the filters do in their inline docs, and also to add a test that covers the wp_get_inline_script_tag action (the test could add add an action and verify it was called with the correct data).

I will shortly add tests to cover wp_get_inline_script_tag action.

Since this is a bit complex to use it will definitely need a "Dev Note" that explains how the API can be used, including stuff like this: https://github.com/WordPress/wordpress-develop/pull/498/files#r499066268.

Will look into it.

Would you say merging this PR fixes https://core.trac.wordpress.org/ticket/39941?

I wouldn't say so; "allowing a Content-Security-Policy without unsafe-inline" would require all script tags to have a nonce, and not having inline event handlers and javascript: URIs.

enricocarraro commented on PR #591:


3 years ago
#54

Thanks, @felixarntz.
I made the changes you suggested. Please, let me know if I can do anything else.

#55 @adamsilverstein
3 years ago

39941.diff Includes only the new API for adding CSP enabled script tags, plus related tests.

  • wp_print_script_tag and wp_get_script_tag: for script tags that load JavaScript files through the src attribute;
  • wp_print_inline_script_tag and wp_get_inline_script_tag: for inline scripts.

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


3 years ago

#57 in reply to: ↑ 45 @whyisjake
3 years ago

Replying to swissspidy:

@helen @whyisjake Any objections to getting https://github.com/WordPress/wordpress-develop/pull/498 in 5.6? No breaking change, but paves the way for people to use Content-Security-Policy headers if they want to.

No objection from me, this will be a great addition!

#58 @adamsilverstein
3 years ago

  • Keywords commit added; dev-feedback removed

Based on feedback here and in Slack, I'm going to commit 39941.diff so we have it available in the beta. Open to additional feedback here, especially re: naming if we need to change anything.

Last edited 3 years ago by adamsilverstein (previous) (diff)

#59 @adamsilverstein
3 years ago

  • Keywords needs-dev-note added
  • Milestone changed from Future Release to 5.6

#60 @ocean90
3 years ago

  • Keywords needs-refresh added; commit removed

Quick feedback on 39941.diff:

  • wp_sanitize_script_attributes() should only sanitize the attributes and not add any new ones. The filter should also be removed.
  • The check ! is_admin() && ! current_theme_supports( 'html5', 'script' ) in wp_sanitize_script_attributes() should only be called once, it's currently called in a loop
  • How about moving the wp_script_attributes filter into wp_get_script_tag() and wp_get_inline_script_tag() and then use this filter to add the type attribute if the theme doesn't support HTML5?
  • sprintf( ' %1$s="%1$s"', $attribute_name );: shouldn't the value escaped with esc_attr() for conistencety?
  • The nested conditions for the boolean attributes should probably be combined to is_bool( $attribute_value ) && $attribute_value
  • I think we should hold off on committing the patch as this without any actual use of the new functions in core, especially late in this cycle.
Last edited 3 years ago by ocean90 (previous) (diff)

#61 @adamsilverstein
3 years ago

  • Milestone changed from 5.6 to 5.7

I think we should hold off on committing the patch as this without any actual use of the new functions in core, especially late in this cycle.

Makes sense. I'll plan on continuing to refine, then committing these changes early in 5.7.

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


3 years ago

enricocarraro commented on PR #591:


3 years ago
#63

Thanks for suggesting the changes @ocean90.
I implemented most of the changes you requested and replied to the comment about combining ifs conditions.

#64 @enricocarraro
3 years ago

I think it would be nice to have phpcs rule to prevent developers from printing <script> tags without passing by wp_get_script_tag() or wp_get_inline_script_tag(), thus breaking strict CSP-compatibility.
Here I proposed a solution. Please, let me know what you think.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-js by hellofromtonya. View the logs.


3 years ago

#68 @adamsilverstein
3 years ago

@ocean90 Can you please review 39941.2.diff (or the PR) - this is a refreshed patch from the PR, I believe @enricocarraro addressed all of your feedback.

Once this is committed, core and plugins/themes will have an API to support CSP. I am working with @enricocarraro on a dev note explaining how plugins and themes can add CSP support.

The next steps to get wp-admin to Strict CSP mode are already well underway (though they deserve careful review and testing). They are:

  • Add nonces to all <script> elements. With strict CSP, every <script> element must have a nonce attribute which matches the value specified in the policy. Work started in https://core.trac.wordpress.org/ticket/39941 / https://github.com/WordPress/wordpress-develop/pull/498)
  • Refactor inline event handlers and javascript: URIs. Inline event handlers (onclick="...", onerror="...") and <a href="javascript:..."> links can be used to run scripts, so an attacker who finds an XSS bug could inject such HTML and execute malicious JavaScript. CSP requires refactoring those patterns into safer alternatives. Work started in https://core.trac.wordpress.org/ticket/51407 / https://github.com/WordPress/wordpress-develop/pull/551.
  • Refactor calls to JS APIs incompatible with CSP - includes things like document.write() used to load additional scripts and uses of eval(). I don't think we use either of these in core; worth checking Gutenberg to ensure it is compliant.
  • Serve the Content-Security-Policy header. This final step will be the reward at the end of the process, turning on Strict CSP.

I plan to continue working on these efforts in 5.7, my hope is to get to strict compliance by 5.8.

#69 @adamsilverstein
3 years ago

  • Keywords needs-refresh removed

cc: @johnbillion this should also be ready for 5.7 beta 1 and I would appreciate a review if you have a chance :)

This ticket was mentioned in PR #964 on WordPress/wordpress-develop by adamsilverstein.


3 years ago
#70

Trac ticket:

This ticket was mentioned in PR #965 on WordPress/wordpress-develop by adamsilverstein.


3 years ago
#71

Trac ticket:

adamsilverstein commented on PR #964:


3 years ago
#72

Thanks for the review @hellofromtonya - I fixed these in https://github.com/WordPress/wordpress-develop/pull/965 - sorry for the confusion :)

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

#74 @adamsilverstein
3 years ago

@jadeddragoon thanks for your detailed feedback on this proposal. Your in-depth knowledge of and passion for CSP is evident in your comments and I appreciate the input. Coming from the WordPress core side, our goal is to add CSP support to make WordPress more secure.

I completely agree with you that this approach adds no security once a site is compromised to the point that a user can call our CSP enabled API directly. (In general WordPress does not provide any PHP side boundaries for plugins or themes). A malicious plugin for example, could use our API to output a script tag with the correct nonce to pass as "secure".

Where CSP could help is in preventing content-based privilege escalation attacks, for example when an unprivileged user is able to create some content that includes a script tag. Then later (due to a lack of output escaping in a plugin for example) another more privileged user later triggers that JavaScript in wp-admin, causing some unknown side effect (user is escalated to admin). If wp-admin operated in Strict CSP mode, this hack would not be possible.

In other words, our CSP implementation does not hope to prevent malicious scripts served up by the WordPress/the web server from executing - this is probably not possible. Instead we aim to prevent scripts injected in content from executing. In addition, we aim to provide an API (and example) plugins and themes can use to also serve their scripts with strict CSP compliance.

Does this use case make more sense from your perspective?

johnbillion commented on PR #965:


3 years ago
#75

In general this seems fine, except the comment about the attribute names. That said, it would be good to see these changes in the context of where the functions will be used, eg. in the script loader tags, CSP-protected inline JS, etc.

adamsilverstein commented on PR #965:


3 years ago
#76

In general this seems fine, except the comment about the attribute names. That said, it would be good to see these changes in the context of where the functions will be used, eg. in the script loader tags, CSP-protected inline JS, etc.

@johnbillion I addressed your feedback here. In terms of how these will be used, take a look at https://core.trac.wordpress.org/ticket/39941 / https://github.com/WordPress/wordpress-develop/pull/498) and https://core.trac.wordpress.org/ticket/51407 / https://github.com/WordPress/wordpress-develop/pull/551 which are the follow up issues created to actually use this new API.

#77 @adamsilverstein
3 years ago

39941.6.diff includes the latest changes from feedback on the GitHub issue.

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


3 years ago

#79 @adamsilverstein
3 years ago

  • Owner set to adamsilverstein
  • Resolution set to fixed
  • Status changed from assigned to closed

In 50167:

Security: add Content-Security-Policy script loaders.

Add new functions wp_get_script_tag, wp_print_script_tag, wp_print_inline_script_tag and wp_get_inline_script_tag that support script attributes. Enables passing attributes such as async or nonce, creating a path forward for enabling a Content-Security-Policy in core, plugins and themes.

Props tomdxw, johnbillion, jadeddragoon, jrchamp, mallorydxw, epicfaace, alinod, enricocarraro, ocean90.
Fixes #39941.

swissspidy commented on PR #498:


3 years ago
#83

@hellofromtonya Actually, no. That commit only resolved #498.

#84 @SergeyBiryukov
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Since the functions added in [50167] are all script-specific, I think wp-includes/script-loader.php would be a better place for them than the wp-includes/functions.php catch-all.

#85 @whyisjake
3 years ago

@enricocarraro and @adamsilverstein any updates on a dev note for this?

#86 @adamsilverstein
3 years ago

@whyisjake yes: I chatted with @audrasjb and he is already working on the dev note. I shared the draft that @enricocarraro had already created.

#87 @whyisjake
3 years ago

Excellent.

#88 @adamsilverstein
3 years ago

Since the functions added in [50167] are all script-specific, I think wp-includes/script-loader.php would be a better place for them than the wp-includes/functions.php catch-all.

@SergeyBiryukov that makes sense, thanks for the feedback. I'll prepare a patch.

This ticket was mentioned in PR #1034 on WordPress/wordpress-develop by hellofromtonya.


3 years ago
#89

Trac ticket: https://core.trac.wordpress.org/ticket/39941

Per Sergey's notes:

Since the functions added in 50167 are all script-specific, I think wp-includes/script-loader.php would be a better place for them than the wp-includes/functions.php catch-all.

This PR relocates:

  • the new functions from wp-includes/functions.php to wp-includes/script-loader.php
  • the tests from tests/functions/ to tests/dependencies

#90 @hellofromTonya
3 years ago

  • Keywords commit added

With RC1 tomorrow, @adamsilverstein I hope you don't mind me popping in to create the patch/PR for this last part.

The PR is straight up copy-paste of relocating the existing code. Only code changes are the additional test annotations.

Marking ready for commit.

#91 @peterwilsoncc
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 50409:

Security: move Content-Security-Policy script loaders.

Move wp_get_script_tag(), wp_print_script_tag(), wp_print_inline_script_tag() and wp_get_inline_script_tag() functions from functions.php to script-loader.php.

Relocate related tests to dependencies sub-directory.

Follow up to [50167].
Props adamsilverstein, hellofromTonya, SergeyBiryukov.
Fixes #39941.

#94 @juliobox
3 years ago

Hey there
In https://core.trac.wordpress.org/attachment/ticket/39941/39941.diff we have :

sprintf( ' %1$s="%1$s"', $attribute_name );

which is not fine because of a lack of esc_attr, good.
So this one patched it: https://core.trac.wordpress.org/attachment/ticket/39941/39941.2.diff

sprintf( ' %1$s="%2$s"', $attribute_name, esc_attr( $attribute_name ) )

Good. Then in fact we wanted to secure the 2 things with escape.
So this patch was added: https://core.trac.wordpress.org/attachment/ticket/39941/39941.6.diff
In that way:

sprintf( ' %1$s="%2$s"', esc_attr( $attribute_name ), esc_attr( $attribute_name ) )

But we do not need the %2$s now.

This is the correct line:

sprintf( ' %1$s="%1$s"', esc_attr( $attribute_name ) )

Thanks :)

#95 @Rahe
3 years ago

Hello,

The wp_sanitize_script_attributes function seems add complexity where there is none.
We can remove the double if by a single if statement :

if ( is_bool( $attribute_value ) && $attribute_value ) {
...
}

Or remove the else and use a continue instead :

if ( is_bool( $attribute_value ) && $attribute_value ) {
        $attributes_string .= $html5_script_support ? sprintf( ' %1$s="%1$s"', esc_attr( $attribute_name ) ) : ' ' . $attribute_name;
        continue;
}
                
$attributes_string .= sprintf( ' %1$s="%2$s"', esc_attr( $attribute_name ), esc_attr( $attribute_value ) );
        
Last edited 3 years ago by Rahe (previous) (diff)

#96 follow-up: @jmlapam
2 years ago

bump up :)

Does someone know if there's now built-in alternative to "unsafe-inline" ?

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

#97 in reply to: ↑ 96 ; follow-up: @enricocarraro
2 years ago

Replying to jmlapam:

Does someone know if there's now built-in alternative to "unsafe-inline" ?

Can you be more clear about what built-in means?

#98 in reply to: ↑ 97 ; follow-up: @jmlapam
2 years ago

Replying to enricocarraro:

Replying to jmlapam:

Does someone know if there's now built-in alternative to "unsafe-inline" ?

Can you be more clear about what built-in means?

Is there an API to add nonces or any other id mechanism for inline scripts and styles?

#99 in reply to: ↑ 98 ; follow-up: @enricocarraro
2 years ago

Replying to jmlapam:

Is there an API to add nonces or any other id mechanism for inline scripts and styles?

Yes, take a look at this article: https://make.wordpress.org/core/2021/02/23/introducing-script-attributes-related-functions-in-wordpress-5-7/

#100 in reply to: ↑ 99 ; follow-up: @jmlapam
2 years ago

Replying to enricocarraro:

Yes, take a look at this article: https://make.wordpress.org/core/2021/02/23/introducing-script-attributes-related-functions-in-wordpress-5-7/

Yeah, I saw it. Would you say the filter wp_script_attributes can do the job, or do you have something still in progress on this?

#101 in reply to: ↑ 100 ; follow-ups: @enricocarraro
2 years ago

Replying to jmlapam:

Yeah, I saw it. Would you say the filter wp_script_attributes can do the job, or do you have something still in progress on this?

You can safely remove unsafe-inline from the CSP header on pages on which every piece of JavaScript is included via a nonced script tag.

You can inject nonces in script tags printed using wp_script_attributes. If a WordPress page contains a script tag that is not nonced, it will be blocked by Strict CSP.

You should check if the pages you are interested in satisfy the above requirements, if they don't, you can manually modify the pages and make them compliant.

There is an effort to refactor WordPress to make all script tags injectable, but it will require time.

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

#102 in reply to: ↑ 101 @jmlapam
2 years ago

Replying to enricocarraro:

There is an effort to refactor WordPress to make all script tags injectable, but it will require time.

Yeah, seems the filter does not apply everywhere 🥲

Thanks for your help @enricocarraro 👍🏻

#103 in reply to: ↑ 101 ; follow-up: @scofennell@…
2 years ago

Replying to enricocarraro:

You can safely remove unsafe-inline from the CSP header on pages on which every piece of JavaScript is included via a nonced script tag.

All of the sites I manage use Varnish or similar caching for non-logged in users. For example, WP Engine calls it "full page caching" I believe.

It's unclear to me what will happen when the nonce is cached in such a situation.

#104 in reply to: ↑ 103 ; follow-up: @enricocarraro
2 years ago

Replying to scofennell@…:

All of the sites I manage use Varnish or similar caching for non-logged in users. For example, WP Engine calls it "full page caching" I believe.

At every request the nonce must be different, so full page caching is not compatible with nonce-based strict CSP.

It's unclear to me what will happen when the nonce is cached in such a situation.

If the nonce is cached and the header too, the scripts will work but they cannot be considered secured by CSP, since the nonce will be reused.
The scripts with nonces that don't match the one(s) in the CSP header will be blocked.
I suggest you to read Mitigate cross-site scripting (XSS) with a strict Content Security Policy (CSP), specifically step 1.

#105 in reply to: ↑ 104 ; follow-up: @scofennell@…
2 years ago

Replying to enricocarraro:

I suggest you to read Mitigate cross-site scripting (XSS) with a strict Content Security Policy (CSP), specifically step 1.

Ah, yes, right, hashes. It seems that with a cached site (varnish or similar), I should be focused on hashes.

But that's really the core of my confusion on this ticket. I believe that the vast majority of ordinary WordPress sites implement server-side caching, so it's hard for me to understand why this ticket represents a significant improvement to the CSP landscape.

Am I missing something?

Maybe is the idea here that it's good and important to make SOME progress on CSP, for folks who can use nonces?

Or is it moreso that this ticket will allow wp-admin to be CSP-safe (obviously wp-admin is never cached)?

(regardless, thank you so much for your work and your responses)

Last edited 2 years ago by scofennell@… (previous) (diff)

#106 in reply to: ↑ 105 ; follow-ups: @enricocarraro
2 years ago

Replying to scofennell@…:

Am I missing something?

I think WordPress should give the option to use strict CSP to the users who want it, I can imagine that dynamic websites like e-commerces would at least consider implementing it for the most sensitive pages.

On caching: full page caching is not the only option, but I get that for mostly-static websites it is the best bang for your buck.

#107 in reply to: ↑ 106 @jmlapam
2 years ago

Replying to enricocarraro:

I think WordPress should give the option to use strict CSP to the users who want it, I can imagine that dynamic websites like e-commerces would at least consider implementing it for the most sensitive pages.

great idea!

#108 @westonruter
6 months ago

In 56687:

Script Loader: Use wp_get_script_tag() and wp_get_inline_script_tag()/wp_print_inline_script_tag() helper functions to output scripts on the frontend and login screen.

Using script tag helper functions allows plugins to employ the wp_script_attributes and wp_inline_script_attributes filters to inject the nonce attribute to apply Content Security Policy (e.g. Strict CSP). Use of helper functions also simplifies logic in WP_Scripts.

  • Update wp_get_inline_script_tag() to wrap inline script in CDATA blocks for XHTML-compatibility when not using HTML5.
  • Ensure the type attribute is printed first in wp_get_inline_script_tag() for back-compat.
  • Wrap existing <script> tags in output buffering to retain IDE supports.
  • In wp_get_inline_script_tag(), append the newline to $javascript before it is passed into the wp_inline_script_attributes filter so that the CSP hash can be computed properly.
  • In the_block_template_skip_link(), opt to enqueue the inline script rather than print it.
  • Add ext-php to composer.json under suggest as previously it was an undeclared dependency for running PHPUnit tests.
  • Update tests to rely on DOMDocument to compare script markup, normalizing unsemantic differences.

Props westonruter, spacedmonkey, flixos90, 10upsimon, dmsnell, mukesh27, joemcgill, swissspidy, azaozz.
Fixes #58664.
See #39941.

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


6 months ago

#110 in reply to: ↑ 106 ; follow-up: @westonruter
6 months ago

Replying to enricocarraro:

I think WordPress should give the option to use strict CSP to the users who want it, I can imagine that dynamic websites like e-commerces would at least consider implementing it for the most sensitive pages.

In [56687] it is now possible to enforce Strict CSP on the frontend and the login screen, assuming the theme and plugins aren't manually constructing script tags on their own.

I mistakenly didn't refer to your impressive PR in the development of the code for that commit, but I see now I should have! It looks like you've done a lot of the work to pave the way for a second phase of this effort, to be able to opt-in to Strict CSP for all of WordPress, including the admin. I intentionally reduced the scope to the frontend/login screen due to the level of effort, which I see you actually did. At present the effort is now complicated a bit by the block/site editor which includes JS-generated script tags in the editor iframe, which breaks Strict CSP. So we'll need to work out a solution to that.

See also #59444 which discusses how we can have better developer experience for JS embedded in string literals as opposed to <script> tags.

I'll create a follow-up ticket for us to track the remaining work since I missed committed work in this closed ticket.

#111 in reply to: ↑ 110 @westonruter
6 months ago

Replying to westonruter:

I'll create a follow-up ticket for us to track the remaining work since I missed committed work in this closed ticket.

See #59446.

This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.


6 months ago

@westonruter commented on PR #5301:


6 months ago
#114

Committed in r56748

Note: See TracTickets for help on using tickets.