WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 3 months ago

#43308 new enhancement

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

Reported by: youngcp Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.9.4
Component: Script Loader Keywords: has-patch
Focuses: Cc:

Attachments (2)

wordpress-mitigate-cve-2018-6389.patch (5.0 KB) - added by youngcp 4 months ago.
Patch
43308.patch (2.5 KB) - added by azaozz 4 months ago.

Download all attachments as: .zip

Change History (20)

#1 @youngcp
4 months ago

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

#2 @ocean90
4 months ago

  • Keywords has-patch added
  • Summary changed from Mitigate CVE 2018-6389 to Require authentication for load-scripts.php and load-styles.php
  • Type changed from defect (bug) to enhancement

#4 follow-up: @Clorith
4 months ago

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.

#5 follow-up: @azaozz
4 months ago

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.

Last edited 4 months ago by azaozz (previous) (diff)

#6 in reply to: ↑ 4 @youngcp
4 months ago

  • Summary changed from Require authentication for load-scripts.php and load-styles.php to Alter behavior load-scripts.php and load-styles.php to reduce potentially adverse scenarios

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.

Last edited 4 months ago by youngcp (previous) (diff)

#7 follow-up: @Clorith
4 months ago

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

#8 in reply to: ↑ 5 @youngcp
4 months ago

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?

#9 in reply to: ↑ 7 @youngcp
4 months ago

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.

#10 @SergeyBiryukov
4 months ago

  • Milestone changed from Awaiting Review to 5.0

#11 @youngcp
4 months ago

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
);

#12 @youngcp
4 months ago

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.

Last edited 4 months ago by youngcp (previous) (diff)

#13 @youngcp
4 months ago

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 uncached requests to wp-admin/load-scripts.php and wp-admin/load-styles.php. Permanent (HTTP 301) redirect and fully cached result is browser friendly.

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.

Last edited 4 months ago by youngcp (previous) (diff)

#14 @youngcp
4 months ago

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

Last edited 4 months ago by youngcp (previous) (diff)

#15 @youngcp
4 months ago

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

@azaozz
4 months ago

#16 @azaozz
4 months ago

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.

#17 @Clorith
4 months ago

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?

#18 @soulseekah
3 months ago

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?

Note: See TracTickets for help on using tickets.