Get WordPress

Make WordPress Core

Opened 7 months ago

Closed 6 months ago

Last modified 6 months ago

#60205 closed enhancement (maybelater)

Automatically protect misconfigured sites from BREACH attacks

Reported by: kkmuffme's profile kkmuffme Owned by:
Milestone: Priority: normal
Severity: minor Version: 6.5
Component: Security Keywords:
Focuses: Cc:

Description

Foreword:
I'm by no means an expert in security issues, I'm reporting this, since I encountered this vulnerability on a service/CDN that was supposed to have this patched, but didn't properly, indicating the difficulty to prevent this vulnerability downstream.
All feedback and discussion welcome.

---

This is not a security issue in WP itself, but rather an issue of and/or:

  • page cache plugins
  • misconfigured servers
  • misconfigured CDNs

and helped by the fact that WP's CSRF/nonce system is not unique per request.
In some cases/plugins, the vulnerability is made worse by only checking the nonce without checking user permission (current_user_can)

---

The vulnerability:
Lots of servers use apache/nginx's gzip (and/or brotli), as well as some CDNs and page cache plugins offer automatic compression.
Unfortunately, this allows for BREACH attacks with nonce when $_GET or $_POST data are output in the page (even when escaped/sanitized for output, since nonce are alphanumeric only)

Theoretically, this also applies when $_COOKIE data is output in the page directly, however

---

The solution: (if I understood it correctly)
HTP ​https://ieeexplore.ieee.org/document/9754554

Basically, whenever the page has ! empty( $_GET ) || ! empty( $_POST ) we output a random number with length n - to further secure this, n is a number between x and y.
Practically a string with length 10 is sufficient for most cases as shown in the research paper.

Practically, I guess using a <meta name="<?php echo random_int( 20, 30 ); ?>"> in the <head> is sufficient.
Ideally we would hook it to multiple places (header, body, footer) and internally call remove_action once it's been output once.
This prevents issues for where the header/body/whatever isn't executed (e.g. bc it's cached as a fragment)

Addiitonally, it should be filterable (to disable) in case a page cache already implements this feature or in cases where people know they have correctly configured their server(s), to prevent duplicate HTB unnecessarily increasing the response size*

*since the only case this solution will not prevent BREACH is when using HTTP 1.1 with "chunked" where each chunk is gzipped too. This can only be mitigated by a page cache or apache/nginx/CDN itself, bc adding the randomness to all chunks or disabling compression when serving chunked.

---

Why in WP?

  • fixing this otherwise requires lots of servers to be reconfigured correctly
  • CDNs get it wrong too
  • it's a super simple fix
  • the increase in total request size is minimal (<= n bytes in the best case, n+markup in the worst case, which is still probably <0.1% of an average WP response)

Why in all cases and not only when e.g. nonce is called?
The effort to comb through the whole codebase and identify functions/places that can cause this, is not worth the effort, given the disadvantages are minimal.

Why not only if Accept-Encoding header is present?
Since the request might be from a CDN that does not support encoding for origin requests, will transform the request for the client, introducing the vulnerability.

Why not in a HTML comment (<!-- HTB-here -->)?
HTML comments are removed from output depending on PHP opcache configuration.

Change History (3)

#1 @kkmuffme
7 months ago

Turns out random_int isn't enough.

but this is:
<meta name="<?php echo base64_encode( random_bytes( random_int( 50, 100 ) ) ); ?>">

As the referenced link explains a 10 byte string will increase 500 fold, a 100 byte 500k fold. Therefore I chose a longer value now too.

---

Also adding this to any of header/body/footer isn't enough if HTTP 1.1 with chunked encoding is used or with HTTP2/3 - since each "chunk" is encoded separately, and the length of each chunk can be analyzed and therefore making HTB useless.
This is the reason why the paper changes the gzip header value directly, as putting it there at a random length ensures all subsequent chunks won't have the same length as a previous request (since all are shifted by the length of the random token in the beginning)

Therefore we need to add it as early as possible in all cases - which means in the <head> section.

For pages that do not have a hook there, adding a do_action( 'breach_protection' ); could be an option, which we hook to and output the meta tag.

e.g.

<?php
/**
 * output meta tag for SSL BREACH attack protection
 * https://core.trac.wordpress.org/ticket/60205
 * @return void
 */
function wp_output_breach_protection() {
        if ( empty( $_GET ) && empty( $_POST ) ) {
                return;
        }

        /**
         * if the user isn't logged in any nonces/CSRF tokens aren't user specific
         * if you hooked on the 'nonce_user_logged_out' hook, you also need to hook here
         * 
         * @param bool whether the page can contain user specific CSRF tokens like nonces
         */     
        $can_contain_user_specific_csrf = apply_filters( 'can_contain_user_specific_csrf', is_user_logged_in() );
        if ( ! $can_contain_user_specific_csrf ) {
                return;
        }

        echo '<meta name="' . base64_encode( random_bytes( random_int( 50, 100 ) ) ) . '">';
}
// only hooks where the expected HTML and we might have CSRF tokens on the page
add_action( 'wp_head', 'wp_output_breach_protection', PHP_INT_MIN, 0 );
add_action( 'admin_head', 'wp_output_breach_protection', PHP_INT_MIN, 0 );
add_action( 'customize_controls_head', 'wp_output_breach_protection', PHP_INT_MIN, 0 );
add_action( 'embed_head', 'wp_output_breach_protection', PHP_INT_MIN, 0 );
add_action( 'breach_protection', 'wp_output_breach_protection', 10, 0 );

#2 @kkmuffme
6 months ago

  • Resolution set to maybelater
  • Severity changed from normal to minor
  • Status changed from new to closed

#3 @swissspidy
6 months ago

  • Milestone Awaiting Review deleted
Note: See TracTickets for help on using tickets.