Don't require sanitization with wp_verify_nonce()

[paulschreiber]

Don't require sanitization (wp_unslash, sanitize_text_field) with wp_verify_nonce().

cc @mjangda

[JDGrimes]

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.

[guillaumemolter]

Hmm I was of the opinion of @paulschreiber, why do I need to sanitize parameters to a core function that returns false or 1 or 2. But then I had no idea that wp_verify_nonce(). But then it seems to me that @JDGrimes argument's is that we need to sanitize the nonce in case someone is doing something with this value other than simply verifying it's a valid nonce. I would argue it is their responsability to sanitize if they do. I'm also very surprised WordPress let you fiddle with one of its core security feature.

[sybrew]

WordPress doesn't use this (consistently) in their code either. Any author that wishes to plug this function should be held responsible for this, should do their homework beforehand, and should expect unsanitized, arbitrary and potentially dangerous data.

[jrfnl]

While I don't believe the error should be removed for nonce verification calls, I do believe that we can make the error message more informative for that specific case by mentioning sanitize_key().

What do you think ?

[sybrew]

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.

[jrfnl]

@sybrew If we're talking about security best practices, I would say that the whole principle of those nonce generation and verification functions being pluggable is wrong™.

However, chances of that changing any time soon are slim to none, what with WPs track record of strenuously avoiding any BC-breaks.

So, until that has been changed, 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.

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.

Good point. If anyone has a better/alternative suggestion, please drop it in a comment.

[sybrew]

It being pluggable isn't wrong per se, as some nonces may require communication across different networks and (CMS) platforms. Having the same protocol without affecting WordPress' salts is a considerable use-case.

Again, we are taught to sanitize data at the point of critical usage, for obvious reasons. We don't know what the information necessarily contains nor pertains, and if we escape stuff upstream, we may cause contextual sensitive security issues (remember why Emojis were added in 4.2?).

And again, not all WordPress developers are collectively following the rules set in this repo. So, some may be forwarding nonces escaped, and other won't. In the end, the plugger must escape it regardless because of this discrepancy. No matter what we do, we won't be able to affect the security.

For wp_verify_nonce(), the input may even use backslashes as part of the plugger's key; although unlikely. This will cause double-escaping issues among headaches for the plugger, and it will exempt many plugins and themes that have followed the rules imposed by this repo. Therefore, in any method chain, we should use the input as-is unless the function is deemed insecure by default, like print(); none of the pluggable functions are insecure.

From the theming documentation:

vip.wordpress.com had some great details on this; unfortunately, they've made their content private. I believe some members of this repo can access it, however. Edit: Here's the archived link

Something could inadvertently change the variable between when it was first cast and when it’s output, introducing a potential vulnerability.

In any case, it is why sanitize_key() shouldn't be used (I'm glad you understood why), and it is why no form of sanitization will work perfectly across all implementations. Adding anything can and will break an application somewhere down the line. Mixing up wp_unslash() with other suggestions would add to this mayhem even more so.

Below you'll find an example in generating false negatives because of us blindly escaping information for pluggables:

// Plugger's private code ---------------------------------- //

// Let's say this is the $_REQUEST nonce hash value set by the user, generated via the plugger's services.
function get_request_nonce() {
    return 'some<-hash';
}

// Let's say this is an API function that runs on the plugger's server.
function get_expected_nonce_from_our_server() {
    return 'some<-hash';
}

// Plugged code -------------------------------------------- //

// The plugger asserts the validity of the nonce here.
function wp_verify_nonce( $nonce ) {
    // We just echo it, because that's how nonces work 🎈🤷‍♂️ 
    echo (int) hash_equals( $nonce, get_expected_nonce_from_our_server() );
}

// Developers code -------------------------------------------- //

// Without enforced input escaping:
wp_verify_nonce( get_request_nonce() );             // Output: 1

echo PHP_EOL;

// With arbitrarily enforced input escaping:
wp_verify_nonce( esc_html( get_request_nonce() ) ); // Output: 0 // !! false negative !!

In the wild (--in fact, in the same file), you can see WordPress Core clearly ignoring this imposition, too: https://core.trac.wordpress.org/browser/tags/5.1.1/src/wp-includes/pluggable.php#L1101

The implementer of the pluggable method should account for that bit of insanity in WordPress itself, and that automagically resolves the issue for all other developers, too. As such, you can remove this rule confidentially. The input parameter is insecure by design, and no one but the plugger can nor should resolve this.

[jrfnl]

@sybrew You seem to be confusing input sanitization with output escaping which are two completely different things and should be handled differently as well.

You also seem to forget that WordPress itself already tampers with the input by adding slashes.

[sybrew]

Do you mean the function wp_magic_quotes(), which uses addslashes()? This has been in WordPress for a very long time.

With that, then why doesn't WordPress Core use wp_unslash() consistently for the nonces? I found 4¹ outlying instances out of 22, which all run after the wp_magic_quotes() call.

¹ The outliers:

1. wp_handle_comment_submission(), it unslashes the complete $_POST input, not only the nonce. I'd call this a coincidence.
2. wp_edit_theme_plugin_file() also unslashes the complete $_POST input, not only the nonce, also a coincidence.
3. _get_list_table( 'WP_Plugin_Install_List_Table' )->prepare_items() does this conciously for $_GET['_wpnonce'].
4. theme-install.php does this for $_GET['_wpnonce'] as well.

If your assessment is correct, then the other 18 instances are considered buggy, and two are a fluke which need to be monitored closely.

This issue has never seen the surface, as the WordPress hash doesn't require slashing. Moreover, with the 18 instances deemed faulty, the plugger must (have already) account(ed) for that, and unslash it via their plugged function if they consider adding quotes, NUL bytes, or backslashes to their nonces.

[GaryJones]

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

[sybrew]

Thanks for that!

I must apologize for my mixup; I strayed off into output escaping the other day.

Now, I do think the moral of the story can be a metaphor for input sanitization: We shouldn't use anything but wp_unslash() (or a more performant non-scalar version thereof: stripslashes_from_strings_only()).

Nevertheless, if we (or the WP Core developers) can assert that WordPress should've used wp_unslash() for the nonce hash in the 18 disregarding internal calls, then I'll gladly adhere to this rule. Then, please consider adding the more direct stripslashes_from_strings_only() call as valid, too.

Please note that PHP's stripslashes() will cast the input into a string, which may be undesired if the hash is a number. So, we shouldn't use that.

$a = 1.01;
echo gettype( stripslashes( $a ) ); // string

[GaryJones]

@pento @swissspidy @ocean90 What was WordPress Core's intent with this function?

Is it expected that the value in the first arg for the pluggable wp_verify_nonce() should already be sanitized? Or should any sanitization be done inside of wp_verify_nonce() as needed (the instance in core seems to just cast to a string)?

[GaryJones]

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()?

[rogertheriault]

I agree with @GaryJones and @sybrew - If the intent of these sniffs is to guide developers to use best practices, why does it seem like this just forces them to add extra code for every use of a function that is actually not - upon a cursory inspection of the implementation of wp_verify_nonce - doing anything substantial or dangerous with said input? Almost everyone either ignores or disregards this warning.

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()?

I agree with @GaryJones that anyone replacing the function, or hooking into the do_action, first ought to know precisely what they're doing and not expect the input to be sanitized. And that seems to be where any checks should be.

[samsagaz]

So is possible to ignore that warning?

[stracker-phil]

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.

[sybrew]

It's the official recommendation.

Where was this engraved as an official recommendation? I'm about as much of a core member as the one who stated that, and I do not deem anything I say as official.

Nevertheless, we should escape as late as possible, and we shouldn't trust arbitrary input from untrusted sources.

Ultimately, WordPress, their pluggable functions, and the pluggers should do the escaping and sanitize the inputs. Since the function discussing in this topic coincidentally and solely handles validation, it should be good at that, and it shouldn't trust or force us to unslash or otherwise sanitize in any meaningful way.

Unless the function strictly accepts one (or a few) types of input (type hinting), I do not see a reason for warnings to be emitted.

[danieliser]

Any progress on getting this rule removed? I've recently also gone over wp_verify_nonce and had come to the same conclusion that its completely safe already, even given unsafe inputs.

+💯 for this change. Also highly approve of making warnings for pluggable function overrides.

[bgoewert]

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

[Mwalek]

Good discussion. I opted to ignore the warning, given that the dangers of unsanitized input are somewhat negligible. 👍

[sybrew]

A nonce isn't a key. sanitize_key() shouldn't be used and I consider it dangerous because it decreases entropy.

Please abolish this entire recommendation before people do more ethereal stuff stemming from an ephemeral error of judgement. Plugins already pose enough of a security risk for WordPress; it can't use another one.

[Abraham-Flutterwave]

what is to used ??

[Luc45]

In my opinion, the issue at hand isn't with the pluggable nonces. Rather, it lies with the user-controlled input - the provided nonce that we're sanitizing, irrespective of whether the actual nonce was plugged or not.

We must check instead whether there's any security risk associated with exposing user-supplied data, which hasn't been sanitized, to wp_verify_nonce.

wp_verify_nonce operates nearly like a pure function without side-effects. It simply verifies if String A (user-supplied nonce) matches String B (expected nonce generated dynamically). The content of String A is inconsequential, as we're essentially performing a string comparison.

However, there's one action in wp_verify_nonce that renders it impure:

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

Let's presume we are sanitizing it as expected: wp_verify_nonce( sanitize_text_field( wp_unslash( $_POST['nonce'] ) ), 'my-action' ) What are we precisely accomplishing here? How does this increase the safety for the listeners of wp_verify_nonce_failed? Are we protecting from what, exactly? XSS?

In my opinion, I'd be prefer to consider wp_verify_nonce as a pure function and not have to sanitize the nonce strings passed to it, I believe this will have a greater benefit of increasing adoption of PHPCS for things that really matters amongst developers, such as escaping their outputs instead. Otherwise, they might be frustrated with "fighting" with the seemingly false-positives and give up on the important fixes.

[Luc45]

Thinking about this a bit more, pluggable nonces are also a point of concern, as the fact of being pluggable also renders the call to wp_verify_nonce "impure", as in: If this is plugged, this unsanitized value could potentially be used either by the listeners of wp_verify_nonce_failed if not plugged, or by whathever the pluggable is doing, which is unknown.

So changing my mind here, I do think there's a good reason to sanitize them, especially if you are writing a distributable plugin.

What we can do is propose some changes to WordPress Core to improve this, so that we don't have to sanitize them:

• We can chime-in on the Trac ticket to remove wp_magic_quotes from WordPress Core, so that we don't have to call wp_unslash() on globals like $_POST or $_GET (Trac Ticket)
• I've opened a new Trac Ticket about a proposed change to sanitize the value in wp_verify_nonce so that we don't have to.

what is to used ??

@Abraham-Flutterwave Overall you could use a portable solution such as wp_verify_nonce( sanitize_text_field( wp_unslash( $_POST['my-nonce'] ) ), 'my-action' );. This should be used in plugins that is going to be installed in many different websites.

If you are writing code that is not meant to be distributed, and you have control over the WordPress installation where you know that a pluggable is not being used, you can use the simpler alternative: wp_verify_nonce( sanitize_key( $_POST['my-nonce'] ), 'my-action' );

[danieliser]

Can’t say I agree with that assessment. For those that are doing so they should also be following best security practices, even more so, in which case they should be escaping and sanitizing user inputs anyways inside it.

I think my grievance is the measure is that “it’s pluggable”. That takes all responsibility off the one overloading, and by that measure there are lots of pluggable functions which could be made completely insecure when overloaded.

TLDR - it should be a safe assumption that any replacement will handle its own security and not do stupid stuff. It is in fact a security function, so I’d hope the person doing so understood the implications.

I also want to point out, if they are plugging it, we can’t make any assumptions about format or or what else they may have plugged. For example if they were for some reason generating custom nonces that included slashes, and you unslash a string before passing it in, but they unslash it as well, it could lead to issues.

My point being that those issues are only handleable by the person overloading it, any presumption on our end is just that.

[peterwilsoncc]

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.

[sybrew]

@peterwilsoncc You haven't explained why it should require wp_unslash(), only why it shouldn't be sanitized.

[peterwilsoncc]

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

[sybrew]

Related to this slashing "insanity," with it unresolved we'll all keep digging this hole bigger: https://core.trac.wordpress.org/ticket/18322

[danieliser]

So if we can agree a developer extending or overloading these functions should escape their own sql, they should also handle slashing themselves as again they may or may not be using it.

any forced unslashing prior could break their implementation.

Safest thing is to enforce it based on the way core actually uses it. Anyone who changes that should be handling inputs as unsafe anyways.

Lastly unslashing is not required for core to function safely as intended, since it’s usage is negligible at best and possibly detrimental in situations of custom overloads I don’t think we should be recommended/enforcing it in CS

[peterwilsoncc]

So if we can agree a developer extending or overloading these functions should escape their own sql, they should also handle slashing themselves as again they may or may not be using it.

any forced unslashing prior could break their implementation.

I disagree because WordPress slashes the data, regardless of the server config.

Consider a user passing the parameter ?_wpnonce=quoted%22nonce%22, this results in the following:

// user input: quoted"nonce"

var_dump( $_GET['_wpnonce'] );
// Output: string 'quoted\"nonce\"' (length=15)
// Does not match user input

var_dump( wp_unslash( $_GET['_wpnonce'] ) );
// Output: string 'quoted"nonce"' (length=13)
// Matches user input

wp_verify_nonce() expects the first parameter, $nonce, to be the value of the user input (hidden fields are still user input) and needs to be passed as such. If the custom implementation reads the super globals directly then, yes, the implementation is responsible for unslashing the data.

Whether WP should be slashing the data is off topic here and best discussed elsewhere.

[samnajian]

Even the usage in check_admin_referer is without sanitizing the input! So I'd say it's a bit too much, even to WP Core's eyes, to sanitize $_GET['_wpnonce'] before passing it to wp_verify_nonce. I really like the analogy here by @Luc45.