Alter behavior load-scripts.php and load-styles.php to reduce potentially adverse scenarios

[youngcp]

Based mostly on ​https://github.com/WazeHell/CVE-2018-638

​https://baraktawily.blogspot.fr/2018/02/how-to-dos-29-of-world-wide-websites.html

​https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-6389

Loads full wp-admin/admin.php during wp-admin/load-scripts.php and wp-admin/load-styles.php

Removes wp-admin/includes/noop.php

Patch: ​https://patch-diff.githubusercontent.com/raw/WordPress/WordPress/pull/343.patch

[youngcp]

Patch

[youngcp]

Patch link missing part of host segment in URL due to copy-pasta... attached patch is still correct.

Correct external patch link: ​https://github.com/WordPress/WordPress/pull/343.patch

[swissspidy]

Related: #36779

[Clorith]

Hi there, and welcome to Trac.

This patch has a few issues, and a missing consideration, that I'd like to address, I'd also like to mention that this is best mitigated using tools like WAFs, fail2ban or similar, due ot the nature of the file you are trying to modify.

The patch should adhere to the ​WordPress coding standards, that's a bit besides the point, but nice to just mention and get out of the way right off the bat.

Now, your solution in this patch is to just include wp-admin/admin.php, this does two things;

• It sends a nocache header (there's a not-modified code in load-scripts.php to reduce repeat requests for legitimate users)
• It forced you to be logged in

This second point creates an awkward situation, as some themes and plugins use this file to concatenate scripts on the front end for visitors as well. This is a consideration we need to account for that may lead to broken sites if we implement something with just authentication requirements.

[azaozz]

I also don't think loading admin.php is a good idea. This basically loads all of WordPress. The point of having load-scripts.php and load-styles.php is to not load WordPress three times on every page load. That's why they use a whitelist to process only default scripts and stylesheets.

As @Clorith points out, there are better ways to prevent malicious use of the loading mechanism, and loading of default scripts and styles has to work for non-authenticated users on the front-end.

[youngcp]

Replying to Clorith:

WAFs mitigate cachable requests, but there's too many non-cached combinations that can be invited by this thing. Fail2ban works ok, but it puts the stressers on; banning entire institutions due to a single compromised source is not good. Neither are good alternatives.

The patch should adhere to the ​WordPress coding standards, that's a bit besides the point, but nice to just mention and get out of the way right off the bat.

Is there a common IDE configuration file somewhere, e.g. for phpStorm, vim, some linters?

Now, your solution in this patch is to just include wp-admin/admin.php, this does two things;

• It sends a nocache header (there's a not-modified code in load-scripts.php to reduce repeat requests for legitimate users)
• It forced you to be logged in

I think I can address both of these in short order.
I see where the no-cache is in code, don't see a no-cache header being sent (I see cache-control: public, max-age=31536000) - I assume it's being overwritten?

This second point creates an awkward situation, as some themes and plugins use this file to concatenate scripts on the front end for visitors as well. This is a consideration we need to account for that may lead to broken sites if we implement something with just authentication requirements.

It's already "hold my beer," but let's see what we can do about controlling non-authenticated access and allowing the result to be cached. Nonces prevent caching, so any variation of is straight out. Anything that generates a lot of potential URL patterns for the same request is out.

So there needs to be a no-cache interstitial that can't be hammered at load-scripts and load-styles with a cache-friendly redirect (a 301 with long expiry) that takes a generated key (maybe a hash) that combines the unique load request and some validation salt to prevent guessing (easy enough). I think I can do that.

Replying to azaozz:

I think I can accommodate you, but the white-list is significantly large. Loading WordPress a third time takes less total time than it took for loading the entire white-list.

[Clorith]

Really, what we need is a "new" approach altogether, see this ticket spurred by Gutenberg and it's increased demand for JS: ​https://github.com/WordPress/gutenberg/issues/4150

Basically users are already experiencing issues due to the scripts loader on shared hosts where the file fails to load, and thus leads to lost functionality on sites. The only fix in those cases is to disable concatenation which requires editing wp-config.php, and we don't want users to need to do this.

We could therefore focus this ticket on improving the various overheads (memory, processing time and the likes) and hit two birds with one stone (hopefully).

Some scenarios that cause the concatenation to fail in one way or another:

• Required too much memory
• Execution took too long
• Query string too long, some libraries not included

[youngcp]

Replying to azaozz:

I also don't think loading admin.php is a good idea. This basically loads all of WordPress. The point of having load-scripts.php and load-styles.php is to not load WordPress three times on every page load. That's why they use a whitelist to process only default scripts and stylesheets.

Would using define( 'SHORTINIT', true ); with wp-load.php count as half-loaded or less than that?

[youngcp]

Replying to Clorith:

Really, what we need is a "new" approach altogether, see this ticket spurred by Gutenberg and it's increased demand for JS: ​https://github.com/WordPress/gutenberg/issues/4150

I like Gutenberg, but I'm not on the bandwagon for waiting for load-scripts and load-styles to get the boot. Creating a new version of WordPress that drops backwards compatibility would be a good thing, but I would hope the concerns introduced by load-scripts and load-styles could be mitigated before that.
There's a lot invested in alternatives to TinyMCE.

Basically users are already experiencing issues due to the scripts loader on shared hosts where the file fails to load, and thus leads to lost functionality on sites. The only fix in those cases is to disable concatenation which requires editing wp-config.php, and we don't want users to need to do this.

We could therefore focus this ticket on improving the various overheads (memory, processing time and the likes) and hit two birds with one stone (hopefully).

Some scenarios that cause the concatenation to fail in one way or another:

• Required too much memory
• Execution took too long
• Query string too long, some libraries not included

I suppose it might be possible to seek an earlier exit once memory usage or execution exceeds some configurable threshold.
PHP (FPM, Apache, Nginx, et. al.) provide this functionality intuitively once a threshold is reached with varying results.
Limiting the quantity of loadable files by size or in count is definitely something to consider.
The relative file sizes could be coded alongside/into the white-list to allow for a summation of the weight of the request to allow an early rejection precluding failure.

[youngcp]

It's pretty clear the patch as is in this issue is not acceptable for the current goals of WP. I'm in favor of closing this ticket and creating a new one with a new patch.

Preliminary pseudo-PHP for brainstorming reference;

<?php
/**
 * Disable error reporting
 *
 * Set this to error_reporting( -1 ) for debugging.
 */
error_reporting( 0 );

define( 'SHORTINIT', true );

require( dirname(dirname( __FILE__ )) . '/wp-load.php' );
require( ABSPATH . WPINC . '/version.php' );

$load = $_GET['load'];
if ( is_array( $load ) ) {
        $load = implode( '', $load );
}

$load = preg_replace( '/[^a-z0-9,_-]+/i', '', $load );

// Sorting reduces cache surface
$targets = sort( array_unique( explode( ',', $load ) ) );

if ( count($targets) ) {
        // Allow client to get a cached empty response
        wp_redirect( admin_url( 'load-scripts-keyed.php?load[]=' ), 301 );
        exit;
}

$load = implode(',', $targets );

// TODO: actions, filters, cache surface reduction

// Is NONCE_KEY acceptable to use as a shared secret here?
$integrity = 'sha256-' . hash( 'sha256', $load . NONCE_KEY );

wp_redirect(
        admin_url(
                'load-scripts-keyed.php?key=' . $integrity .
                        '&load[]=' . $load .
                        '&ver=' . $_GET['ver'] .
                        '&c=' . $_GET['c']
        ),
        301
);

[youngcp]

The 301s can be sent with (or without) expiry headers.

Client will get a one-time cost per unique load-scripts/load-styles URL before the 301 to a fully cached resource.

DoS attempts will no longer be able to succeed in generating 1.551121e+25 (25 factorial) unique URLs, and are only able to invoke a smaller subset (325) of that.

The cache surface area is reduced by making the URLs cache friendly. (uniform; load is unique, sorted)

load-scripts-keyed.php and load-styles-keyed.php should be strict about the URLs that invoke them, expecting parameters in specific sequence, and validate against the key.

Multiple URLs pulling the same content won't/can't exist and WAFs will be happy to absorb a DoS.

If a plugin can act at that //TODO, is there anything left to consider?

The mitigation of DoS attacks based on cache bypassing is then reasonably possible from 3rd party plugins at the expense of speed.

[youngcp]

Current work is as follows...

Code Review: ​https://github.com/WordPress/WordPress/pull/344/files

Diff: ​https://github.com/WordPress/WordPress/pull/344.diff

Patch: ​https://github.com/WordPress/WordPress/pull/344.patch

Reduces possible cache surface for WAL friendliness to 650 (325 combinations of files * 2 for compression) URLs.

Changes noop.php to function_exists check each function definition.

Performs 'SHORTINIT' of WordPress (up to) 2 times on an uncached requests to wp-admin/load-scripts.php and wp-admin/load-styles.php.

Let me know if I messed up any code styling or anything.

The patch can be broken into 2 stages; to first rename load-scripts and load-styles to *-keyed, then to create the new load-scripts and load-styles. Let me know if this is desired.

[youngcp]

Sorting needs to be fixed - needs to match some predefined sequence to respect implicit (edit: not explicit) dependencies.

[youngcp]

Will submit a patch that uses $wp_scripts->do_concat = true; and ->do_items - solves dependency sorting.

[azaozz]

Did some testing and seems the worst part is compressing all the scripts. Removing compression from PHP (see 43308.patch) seems to prevent misuse.

Also see the discussion on #12009 for possibly switching to HTTP/2 + defer, retiring load-scripts.php and outputting individual script tags.

[Clorith]

Looking into HTTP/2 like mentioned would definitely help, and is something we should keep on the table in that ticket (I see it's picked up some pace again).

Removing compression is perfectly fine, many servers will handle the gzipping outside the PHP layer any way, won't they, so we don't need to worry about that much just concat them and leave it at that?

[soulseekah]

Some tests with the PoC publically available:

load-scripts.php 181 scripts no caching, no compression: cpu 0.021 sec
    load-scripts.php 181 scripts no caching, compressed: cpu 0.084 sec
                                   index.php no caching: cpu 0.157 sec

Requesting the frontend consumes twice the resources of load-scripts.php on PHP 7.2. The PoC would have gotten better results requesting just the frontend. This doesn't really seem to be an issue at all.

Am I missing something?

[azaozz]

In 43580:

Script loader: remove (PHP based) compression from load-styles.php and load-scripts.php. WIth the amount of scripts and stylesheets grown a lot over the years, it has become pretty slow and consumes a lot of server resources. Also, most servers are set to compress PHP output anyway.

Props LucasRolff, azaozz.

Fixes #44815.
See #43308.

[SergeyBiryukov]

In 43618:

Script loader: remove (PHP based) compression from load-styles.php and load-scripts.php. WIth the amount of scripts and stylesheets grown a lot over the years, it has become pretty slow and consumes a lot of server resources. Also, most servers are set to compress PHP output anyway.

Props LucasRolff, azaozz.
Merges [43580] to the 4.9 branch.
Fixes #44815. See #43308.

[ocean90]

#44943 was marked as a duplicate.

[SergeyBiryukov]

In 43697:

Script loader: remove (PHP based) compression from load-styles.php and load-scripts.php. WIth the amount of scripts and stylesheets grown a lot over the years, it has become pretty slow and consumes a lot of server resources. Also, most servers are set to compress PHP output anyway.

Props LucasRolff, azaozz.
Merges [43580] to the 5.0 branch.
Fixes #44815. See #43308.

[dgilfillan]

Hi, I'm hoping one of you might be able to help. I realise this ticket is marked as closed and suggests the CVE-2018-6389 exploit has been patched. But in the latest WordPress v6.1.1 its still seems an unauthenticated user can make a request to /wp-admin/load-scripts.php and pull a concataned file full of js scripts?

If it has been fixed, would someone be able to give me a plain english run down of how the exploit is now prevented?