7 new PHP static analysis
No less than 7 new analyzers for Exakat, since last week version. Here is a quick review, as they are both varied and interesting.
Identical conditions
Identical conditions spots members of an complex logical expression that are identical. When several conditions are chained, it happens that some of the conditions are duplicated. This doesn’t hamper the normal execution of the code, but is useless. The following has been found in real code :
<?php // Actual code //... if(!isset($_SESSION['USER_ID']) or !isset($_SESSION['USER_ID']) or $_SESSION['USER_LEVEL'] > 3) die();
Logical Mistake
Those ones come from reading Andrey Karpov’s article about ‘Logical expressions in C/C++’ (http://www.viva64.com/en/b/0390/). Four out of five of those mistakes are legit in PHP, and I’m determined to see if they also apply in PHP Open Source. The fifth one is still being studied.
The logical mistake are expressions that may be always true, or false ; they may also simply not depends on one of the member of the expression. For example, in the example below, the first member of the expression is useless, and may be dropped :
<?php
if ( $a == 1 ||$a!= 2) { }
?>
At the moment, this new analyzers covers the article’s issues, and several additions will come in the next weeks.
Iffectations
Iffectations are an assignation of value in a if condition. Something that is common in PHP, and less frequent in other languages, for fear of mistaking the comparison to the assignation :
<?php
// This is OK
if ( $foo = check_value($bar)) { }
// This is a constant condition
if ( $foo = 1) { }
?>
This is one of the first analyzers that was written. It was retired as it was bringing too many false positive. It was revamped, now focusing on literal assignations, and it works much better.
Note that iffectations may also happen in other conditional structures, like switch, while, do..while, etc.
No Choice Alternative
‘No Choice Alternative’ is a situation where the conditional structure leads to the same output, no matter what. It’s kind of the opposite to the previous one : instead of a constant condition, this is a constant result.
<?php
// A legit condition
if ( $foo == 2) {
doSomething() ;
} else {
doSomething() ;
}
?>
From experience, this is often a left over of development, a massive search/replace or even a debugging line. One of them has to go, though it is difficult to know which one. This analyzer has already yielded results in Open Source code.
Common alternatives
Common alternatives is inspired from the previous analysis : it searches inside conditional blocks (then and else) for common expressions : those may be grouped by moving them out of the condition, and reducing them to one.
<?php
// Actual code
if($isLiteral) {
$tokens[]=$literal;
$literal='';
$isLiteral=false;
}
else
{
$isLiteral=true;
$literal='';
}
?>
In this extract of code, $literal=”; may be moved out of the conditional expression, before or after. Depending on the code, both locations, or only one of them is correct, so one may have to be careful when refactoring this code.
Unknown PCRE options
Based on the same query than the one that looks for /e options for PHP 7 compatibility, this analysis searches for regex options that are not valid in PHP. It catches invalid options, may be coming from another language, or wrongly escaped delimiter inside the regex itself.
<?php
preg_match('/<br />Hello/is', $html) ; // H, l and o are not Regex options.
?>
Random_* Without Try
Among the new features of PHP 7.0, there are random_bytes() and random_int(), which provides safe random values for security purposes. However, they do not behave like the old rand() or mt_rand() : when they fail, the return value should not be used for security purposes, as it may be partially available, or completely empty. To prevent this, those two functions emits exceptions when they fail, preventing the rest of the script to go. Using them implies setting them in a try {} catch () {} block.
<?php
try {
$salt = random_bytes($length);
} catch (TypeError $e) {
// Error while reading the provided parameter
} catch (Exception $e) {
// Insufficient random data generated
} catch (Error $e) {
// Error with the provided parameter: <= 0
}
?>
More about this : https://wiki.php.net/rfc/random-function-exceptions
Hardcoded hashes
Last analyzer for version 0.6.0 looks for possible hashes in the code. Just like passwords, IP or user name, hashes are best kept out of the code. System admin will be happy to change those secret hashes to something that depends on execution, and you’ll avoid releasing sensitive data in case of a source code leak or a github commit. Quite a number of those hashes are actually default values, that will be used if no other is provided. This may lead to significant security breaches, unless some fail strategy is used, like for the Exception feature for random_bytes() (see above).
<?php // Actual code private $_defhash = 'da39a3ee5e6b4b0d3255bfef95601890afd80709'; ?>
Exakat version 0.6.0 is available
All of those analysers were applied to Open Source code, and to exakat itself : if they look deceiptively simple, those errors tends to find their way in the code, and never leaves. It is the job of the reviewer, to read the code and find them. This is why Exakat was written : review code so that you know you’re not leaving any hidden bug behind.
You may try those analysis and the 230 others by downloading Exakat, the static analysis engine for PHP. It is available as Open source and soon as an online service.