WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#41617 closed defect (bug) (wontfix)

wp_verify_nonce() check fails on several websites because of filter possibility in wp_nonce_tick()

Reported by: ReneHermi Owned by:
Milestone: Priority: normal
Severity: critical Version: 4.8.1
Component: Security Keywords: dev-feedback 2nd-opinion close
Focuses: Cc:

Description

wp_nonce_tick() is an essential part of the hash which is used as the nonce for creating and checking if a request is authenticated. Unfortunately wp_nonce_tick() is filterable by third party developers!

So it happens occasionally that other plugins hook in there and are changing the nonce life time. Usually not a big deal if these filters would always be running globally and with highest priority BUT as every plugin developer is baking their own cake and ever plugin is loaded with another priority, these filters get overwritten inconsistently over time and over load order, depending on in which hook the nonce is created and where it is checked.

I experienced this issue on two different customer websites this week.

Example to reproduce it:

  • Create a nonce with wp_create_nonce() in hook admin_enqueue_scripts(). Use a plugin to do this
  • Populate the nonce there with wp_localize_script(). just as you would like to access it with js.
  • Create a filter and overwrite the life span in wp_nonce_tick()
  • Put this filter into ANOTHER plugin:
function overwrite($seconds){
	 return 10600;
 }
 add_filter('nonce_life', 'overwrite', 1);

Now check the wp_nonce_tick() in the first plugin from another hook like admin_init and you will notice that the results differ. This is not unusual and unexpected in the way these filters are working but as wp_nonce_tick() is part of the nonce hash, the whole nonce will differ as well and as a result the wp_nonce_check fails completely than.

In my opinion, this filter should be removed entirely to ensure that the nonce is always consistent and can not be changed by third parties. There should be no way to change the value of a hash by filters.

This is not such a rare possible issue if you look how many plugins are changing the value of the nonce_life value https://github.com/search?utf8=%E2%9C%93&q=nonce_life&type=Code

To make my plugin working for all users i also need to play the same game and need to use a filter to change the nonce_life value to ensure it is everytime the same in my plugin instance. The alternative would be to remove the nonce check at all. Not really something i like to do.

Change History (3)

#1 @ReneHermi
2 years ago

  • Component changed from General to Security
  • Keywords dev-feedback 2nd-opinion needs-patch added
  • Severity changed from normal to critical

Change pluggable.php

function wp_nonce_tick() {
	/**
	 * Filters the lifespan of nonces in seconds.
	 *
	 * @since 2.5.0
	 *
	 * @param int $lifespan Lifespan of nonces in seconds. Default 86,400 seconds, or one day.
	 */
	$nonce_life = apply_filters( 'nonce_life', DAY_IN_SECONDS );

	return ceil(time() / ( $nonce_life / 2 ));
}

To

function wp_nonce_tick() {
	return ceil(time() / ( DAY_IN_SECONDS / 2 ));
}

#2 @dd32
2 years ago

  • Keywords close added; needs-patch removed

IMHO plugins which filter this, but do not do so conditionally (ie. add_filter, perform action, remove_filter) are probably doing it wrong. Additionally, plugins using this are probably not using it correctly and should be using something like a signed url instead.

The filter is designed to be very generic, so change it from 1 day to something such as 1 hour or 1 week globally, rather than for single nonce checks.

I personally don't think anything should/need be changed here, badly written plugins can break sites in many unintended ways.

#3 @johnbillion
2 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

It's unfortunate that this filter allows a plugin to easily break a site, but there are dozens of filters in WordPress which are equally as powerful. A wrong return value from map_meta_cap, user_has_cap, authenticate, or salt for example will easily break your site, but it doesn't serve anyone well to remove filters which are being misused.

I'd support some improved documentation for this filter, but beyond that the best approach is to notify developers who are misusing this filter and ask them to correct or improve their code.

Note: See TracTickets for help on using tickets.