#43308 closed enhancement (fixed)
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: |
Description
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
Attachments (2)
Change History (27)
#1
@
7 years 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
@
7 years 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:
↓ 6
@
7 years 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:
↓ 8
@
7 years 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. Also, loading default scripts and styles has to work for non-authenticated users on the front-end.
#6
in reply to:
↑ 4
@
7 years 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.
#7
follow-up:
↓ 9
@
7 years 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
@
7 years 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 havingload-scripts.php
andload-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
@
7 years 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.
#11
@
7 years 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
@
7 years 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.
#13
@
7 years 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.
#14
@
7 years ago
Sorting needs to be fixed - needs to match some predefined sequence to respect implicit (edit: not explicit) dependencies.
#15
@
7 years ago
Will submit a patch that uses $wp_scripts->do_concat = true;
and ->do_items
- solves dependency sorting.
#16
@
7 years 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
@
7 years 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
@
7 years 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?
#24
@
6 years ago
- Milestone changed from 5.1 to 5.0
- Resolution set to fixed
- Status changed from new to closed
#25
@
20 months ago
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?
Patch