WordPress / plugin-check

A repository for the new Plugin Check plugin from the WordPress Performance and Plugins Team.
https://wordpress.org/plugins/plugin-check/
GNU General Public License v2.0
266 stars 53 forks source link

Nonce verifcation check false positives #759

Closed DavidAnderson684 closed 3 weeks ago

DavidAnderson684 commented 3 weeks ago

In lots of plugins which I'm checking, every nonce check throws two warnings which are false positives. Here's an example:

if (empty($_POST) || !isset($_POST['_wpnonce']) || !wp_verify_nonce($_POST['_wpnonce'], 'my_nonce') die('Security check');

This will result in:

WARNING     WordPress.Security.ValidatedSanitizedInput.MissingUnslash   $_POST['_wpnonce'] not unslashed before sanitization. Use wp_unslash() or similar 
WARNING     WordPress.Security.ValidatedSanitizedInput.InputNotSanitized    Detected usage of a non-sanitized input variable: $_POST['_wpnonce'] 

The column numbers given for both warnings are for the wp_verify_nonce() call.

1) Anything going to wp_verify_nonce() should be exempted from requiring unslashing. Nonces don't include slashes, and if a logged-in user decided to throw some in the only result would be that he failed the nonce check anyway.

2) Similarly, anything going to wp_verify_nonce() doesn't need sanitising. Again, if the logged-in user decides to throw in characters that aren't found in nonces usually, he'll just fail the nonce check.

davidperezgar commented 3 weeks ago

We know that nonces has a lot of false positives. That's why it's a warning instead of error. This check is running WPCS, so it could be a good idea to create an issue in WPCS.

swissspidy commented 3 weeks ago

Agreed. Therefore closing as this would need to be handled upstream.