WordPress-Coding-Standards: Don't require sanitization with wp_verify_nonce()

This has come up again for us at VIPCS.

Use case:

if ( ! isset( $_POST['my_nonce'] ) ||
    ! wp_verify_nonce( $_POST['my_nonce'], 'my_action' ) ) {
	return;
}

Currently that throws a:

Warning: Detected usage of a non-sanitized input variable: `$_POST[‘my_nonce’] (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized).

Let’s look at the definition of wp_verify_nonce():

function wp_verify_nonce( $nonce, $action = -1 ) {
    $nonce = (string) $nonce;
    $user  = wp_get_current_user();
    $uid   = (int) $user->ID;
    if ( ! $uid ) {
        /**
         * Filters whether the user who generated the nonce is logged out.
         *
         * @since 3.5.0
         *
         * @param int    $uid    ID of the nonce-owning user.
         * @param string $action The nonce action.
         */
        $uid = apply_filters( 'nonce_user_logged_out', $uid, $action );
    }
 
    if ( empty( $nonce ) ) {
        return false;
    }
 
    $token = wp_get_session_token();
    $i     = wp_nonce_tick();
 
    // Nonce generated 0-12 hours ago
    $expected = substr( wp_hash( $i . '|' . $action . '|' . $uid . '|' . $token, 'nonce' ), -12, 10 );
    if ( hash_equals( $expected, $nonce ) ) {
        return 1;
    }
 
    // Nonce generated 12-24 hours ago
    $expected = substr( wp_hash( ( $i - 1 ) . '|' . $action . '|' . $uid . '|' . $token, 'nonce' ), -12, 10 );
    if ( hash_equals( $expected, $nonce ) ) {
        return 2;
    }
 
    /**
     * Fires when nonce verification fails.
     *
     * @since 4.4.0
     *
     * @param string     $nonce  The invalid nonce.
     * @param string|int $action The nonce action.
     * @param WP_User    $user   The current user object.
     * @param string     $token  The user's session token.
     */
    do_action( 'wp_verify_nonce_failed', $nonce, $action, $user, $token );
 
    // Invalid nonce
    return false;
}

Taken with a consideration that $nonce may contain a value that could be considered unsafe, we can see that:

1. It is cast to a string. At worst, we now have an unsafe string to handle.
2. If that string is empty, then return false.
3. It’s used in a comparison (via hash_equals()), and may return 1. At worse§, our unsafe string fails to be equal to the generated hash - no harm done.
4. If the comparison fails, then it’s used in a second comparison (again, with hash_equals()). At worse§, the unsafe string again fails to equal the generated hash.
5. The $nonce is passed as the second arg of do_action().

§ For 3. and 4., the only way for the nonce to be verified when it shouldn’t would be for the the nonce generation to have been plugged such that the generated nonce was also an unsafe string. This seems unlikely and would imply bigger problems anyway.

So, we don’t store the first arg wp_verify_nonce() in the database, nor do we output it. In fact, it’s only used for a comparison, and we already have code that doesn’t trigger this violation for simple comparisons - it just doesn’t include a comparison using hash_equals().

All told, I think I’m happy that a sanitization wrapper is not needed for the first arg of wp_verify_nonce().

---

you should presume that the end-user will have a plugin installed which overrules the WP native wp_verify_nonce() function and therefore the value should be sanitized before passing it off.

I think the balance would be that the majority of installs don’t plug this function.

Instead of looking at wp_verify_nonce() calls, can we look for instances of that function being plugged, and add a Warning there that the first arg should only be used in comparisons, ideally with hash_equals()?

@sybrew WordPress slashes all user input. Failing to unslash will be result in an incorrect value for custom implementations containing characters that get slashed.

Having considered this, I think user input passed to wp_verify_nonce() should require wp_unslash() but is probably unique in that it should not be sanitized.

Consider the situation in which the valid value is correct and the user input is !correct. As the user input is not correct, the nonce verification should fail.

Sanitizing the user input with sanitize_key() will result in the incorrect value being converted to the correct value, allowing the nonce check to pass:

wp> sanitize_key( '!correct' )
=> string(7) "correct"
wp> 

Recommending sanitize_key also fails to account for systems in which the nonce function has been plugged and replaced with a more complex value than WordPress uses, for example a system that is case sensitive or uses symbols:

wp> sanitize_key( 'ThisIsCorrect!!' )
=> string(13) "thisiscorrect"

In the case of WordPress’s default functions, the user value is passed directly to hash_equals(). In the event a replacement system has been written that uses a proper nonce (rather than a time based cross site protection token, like WordPress) and requires checking a value in the database, then the developer of that system should be responsible for preparing the SQL statement.

It’s easy enough to ignore or add sanitize_key(), as is the suggested resolution. But would like some more info in the error message specific to wp_verify_nonce() instead of searching for an answer because it’s not really documented except in tickets (AFAIK).

+1 for either removing or making it more informative to suggest using sanitize_key()

Personally I think the warning can be ignored - as @GaryJones explained, the wp_verify_nonce() function already does sufficient sanitation.

However, also @JDGrimes gave a perfect solution for this: Use sanitize_key() to sanitize/escape the value.

It’s true, that sanitize_key() will convert the result to lowercase. That’s perfectly fine, because wp_generate_nonce() creates a nonce using wp_hash(), which returns a hash_hmac() value.

[The function hash_hmac] returns a string containing the calculated message digest as lowercase hexits …

So, using the sanitize_key() function AND the wp_generate_nonce() function both return lowercase strings and can be used together.

Also, in another issue (raised by a WP Core member) the case was already discussed and closed without any change: https://github.com/WordPress/WordPress-Coding-Standards/issues/414

TL;DR;

Use sanitize_key() to “fix” that warning. It’s safe. It’s the official recommendation.

vip.wordpress.com had some great details on this; unfortunately, they’ve made their content private.

https://vip.wordpress.com/documentation/vip-go/code-review-blockers-warnings-notices/#validation-sanitization-and-escaping is likely the link you’re looking for (with further links).

I don’t think we should impose this rule on thousands of WordPress developers, of which most aren’t using these rules, for the one developer that wishes to do something arbitrary with the $nonce value whilst assuming it is safe.

Implying that we should all follow these rules so that the one developer can assume this is secure is a wrong design pattern. It would invoke security issues that should instead be handled at the moment it’s critically used, like during storage, evaluation, and output.

With this in mind, satirically, why aren’t we forced escape each and every string when we use it?

Wrong:

$string = $some_var; // WARNING: $some_var isn't trusted. Use `esc_html()`.
echo $string;

Correct:

$string = $some_var; 
echo $string; // WARNING: $string isn't trusted. Use `esc_html()`.

The standard (reasonable) rules should already be applicable when creating functions. We shouldn’t ever trust input data unless it’s from a controlled source. Pluggable functions aren’t such a source, they’re arbitrary, and any other developer can call them.

A great example in this is get_avatar(), which clearly shows it doesn’t trust its input ($args): https://core.trac.wordpress.org/browser/tags/5.1.1/src/wp-includes/pluggable.php#L2631

More pluggable examples can be found in methods such as wp_redirect(), cache_users(), get_userdata(), which all assume that their input isn’t secure down the line.

As for sanitize_key(): This runs strtolower(), and thus can cause issues with the pluggable implementation of the code, where the creator of the $nonce value expects capital characters.

To conclude, the creator of the pluggable functions should be held responsible for handling the validity, sanity, and security of the raw inputs. Every twist we add to this chain will ripple as issues down the line.

Why, specifically? It is a pluggable function, we have no idea what it is going to do with the value you pass it. So I think it is safer to sanitize it first.

As a side note, you can use sanitize_key() and avoid the wp_unslash() call.