Look this code:
if ( isset( $_POST['cartflows-action-nonce'] ) && ! wp_verify_nonce( sanitize_text_field( wp_unslash( $_POST['cartflows-action-nonce'] ) ), 'cartflows-action-nonce' ) ) {
return;
}
This code was taken from a WordPress plugin with more than 100,000 active downloads, Cartflows (https://wordpress.org/plugins/cartflows/).
This condition is a security validation to block CSRF, it works to know that you are yourself, wp_verify_nonce checks if you really came from the previous page and not a request from anywhere, it keeps a “sequence”.
But that validation is wrong, and that security item can easily be overcome. This is not such a serious failure, but with other errors it can be fatal.
This video in the sequence scans the code of all plugins to validate who has this same type of problem, and we found a total of 420 plugins possibly with the same problem.
Let’s understand what’s going on:
I think everyone has seen a table of logical operators like this:
https://www.php.net/manual/pt_BR/language.operators.logical.php
We always think we know, we are experienced and we never make mistakes, so I will ask a question and you will answer me quickly.
How do we validate this variable, $ _GET [‘test’]?
We must validate if it exists and if the index has the letter a, otherwise, stop the execution, remembering that it cannot break (Undefined Error) if we don’t use it.
Look the options:
I-if( strpos($_GET['test'],'a') === false ) die();
II-if( isset($_GET['test']) && strpos($_GET['test'],'a') === false ) die();
III-if( ! isset($_GET['test']) && strpos($_GET['test'],'a') === false ) die();
IV-if( ! empty($_GET['test']) && strpos($_GET['test'],'a') === false ) die();
Answer:
I – This generates an Undefined error.
II – if you don’t use the test parameter it doesn’t stop;
III – use? Test = bbb and it will pass
IV – if you do not use the test parameter it does not stop;
Result, only the first will work, but it will generate Undefined, all the others will not work the way we need, so no result is what we expect.
But what is the technical reason for this to happen and what happens in the minds of the developers?
- _GET, _POST and _REQUEST are arrays and as every array needs to go through an index check or it will always generate Undefined if it does not exist, that is, it needs an isset or empty before.
- In this case, as the “if truth” is for the system not to continue, the code has to check all validations separately (with ||). To put it simply, it would be like an if for each condition.
- This is often because we identify an Undefined after creating the condition and after that and the first thing that comes to mind is to validate if it exists and add with the first condition, but we saw that it is not quite so.
So one of the correct answers is:
if(! isset($_GET['test']) || strpos($_GET['test'],'a') === false ) die();
Some tips for this not to happen:
- Attention when you want to use return early, it can confuse the conditions and quality can become a vulnerability.
- If in doubt for a return early, for safety, create an if for each condition (People who like the pattern will not like it).
- Check all the indexes of _POST, _GET, _REQUEST before, if not, fill in variable with false. Frameworks already do that.
- Test by not sending parameters.
- Most of the time empty will do, it already does the isset service.
Obs:
PHP 8 has a method for this, str_contains.
if (str_contains(‘How are you’, ‘are’)) { echo ‘true’; }
References:
https://www.php.net/manual/en/index.php
Tahnks for revision by Thiago Dieb and Leonn Leite