Exakat 1.4.3 review
Welcome back to a new edition of the Exakat 1.4.3 review. This Monday, we have a lot new analyzers : they deal with Classes that could be final, closure that could be simplified and inconsistent if/elseif situations. Also, the PHP directive report has been augmented with a full list of functions that should be disabled when they are not used in the code : this helps with security. The tribe has spoken : here is a perfect Exakat 1.4.3 review.
Classes that could be final
From the manual, about Final Keyword: “PHP 5 introduces the final keyword, which prevents child classes from overriding a method by prefixing the definition with final. If the class itself is being defined final then it cannot be extended.”
Final should be used whenever possible. From that rule, and given any code base, any class that isn’t derived is now recommended as a final class. Notwithstanding an imminent update, adding finalto such classes won’t change anything. It will be a safeguard for future code evolution.
<?php
// This class can't be final
class A {}
// This class can be final
class B extends A {}
?>
This is probably a very simple an analysis. May be too simple. Yet, since final is such an underused feature of PHP, it may be worth being pointed to every possible case of usage. If you have ways to lessen the amount of reported classes, feel free to ping us on Slack or @exakat.
If this seems too simple for your code, you can always read a more refined and opinionated article from Marco Pivetta: When to declare classes final.
Of final methods
In fact, both classes and methods may be declared final. Recommending methods for final based on the fact that they aren’t overwritten will lead to an 80% of final method. This is even larger number than classes.
Note also that private methods are final by default: even if a subclass has an identical method, the private keyword makes the method only accessible to the current class. Then, a privatemethod and its subclass version are two distinct methods. final and private are probably a repeat.
Simplify Closure Into Callback
Closures were implemented in PHP 5.3. They have a PHP native class, aptly called closure.
<?php
$y = 2;
$closure = function ($x) use ($y) { return $x + $y + 1;}
echo $closure(3);
?>
Closures are a modern version of PHP’s callback. They are unnamed functions, also called anonymous functions, and are often used when naming is superfluous: for examples, with array_map, array_filter…
Backward compatibility is ensured, by keep the old style callbacks working. And, when a simple closure is mixed with a native PHP call, we end up with this:
<?php
$closure = function ($x) { return strtoupper($x);}
$array = array_map(range('a', 'z'), $closure);
?>
Anytime the closure is simple enough to be a single PHP or custom function call, or even a static or non-static method call, it is recommended to drop the closure and keep the call. The above code may be rewritten:
<?php
$array = array_map(range('a', 'z'), 'strtoupper');
?>
This syntax is actually 50% faster, as PHP doesn’t optimize away those simple closures. Given that closures are often used in loops, it is probably more than a micro-optimization.
Inconsistent if/elseif
if/elseif is a string of condition that are mutually exclusive. Contrary to a switch, which may be executed in a different order than written, the n-Th condition of a if/elseif is actually the negation of all the previous n – 1 conditions, augmented with the new one.
<?php
if ($a < 1) { }
elseif ($a < 2) {}
elseif ($a < 4) {}
elseif ($a < 90) {} // This point has tested 4 conditions
else {}
?>
If/elseif acts as a large AND condition: reaching condition 3 means that not(condition 1) and not(condition 2) and condition 3 is valid. Of course, those condition shouldn’t be mutually exclusive, but they also should cover different realities.
<?php
if(isset($_SERVER['SCRIPT_NAME']) && basename($_SERVER['SCRIPT_NAME']) === $filename) { /**/ }
elseif(isset($_SERVER['PHP_SELF']) && basename($_SERVER['PHP_SELF']) === $filename) { /**/ }
elseif(isset($_SERVER['ORIG_SCRIPT_NAME']) && basename($_SERVER['ORIG_SCRIPT_NAME']) === $filename) { /**/ }
else { /**/ }
?>
In a complex string of condition like the one above, there is still a common element: $_SERVER. The variable is tested for different values, and each of them leads to a fallback to another part of the variable. This is a consistent if/elseif.
<?php
if(!@file_exists(__TYPECHO_ROOT_DIR__ . '/config.inc.php')) : /**/
elseif(!Typecho_Cookie::get('__typecho_config')) : /**/
else : /**/ endif
?>
This structure has seemingly disconnected features : a file, then a cookie are tested. And finally, a default behavior is applied. It is not obvious that the three steps are related (code is hidden as comment here), and that may lead to questions like : what happens if I need to overwrite a config ?
With this new analysis, if/elseif structures which don’t share at least one common variable are reported as inconsistent. This should trigger a review of the code, to avoid impossible situations.
Typecasting json_decode()
json_decode() comes with the default behavior to decode JSON as it is specified. Arrays and objects are converted to PHP arrays and objects.
There is also a json_decode() option that forces the decoding to create arrays and not objects : JSON_OBJECT_AS_ARRAY.
<?php
$json = '{"a":1,"b":2}';
print_r(json_decode($json));
print_r(json_decode($json, JSON_OBJECT_AS_ARRAY));
/*
stdClass Object
(
[a] => 1
[b] => 2
[c] => 3
[d] => 4
[e] => 5
)
Array
(
[a] => 1
[b] => 2
[c] => 3
[d] => 4
[e] => 5
)
*/
?>
Using JSON_OBJECT_AS_ARRAY is recommended, and should replace usage of (array) : this operator doesn’t do a deep transformation and only converts the first level of the JSON representation to an array.
<?php
$json = '{"a":1,"b":["c",2]}';
print_r(json_decode($json));
print_r(json_decode($json, JSON_OBJECT_AS_ARRAY));
/*
stdClass Object
(
[a] => 1
[b] => Array
(
[0] => c
[1] => 2
)
)
Array
(
[a] => 1
[b] => Array >= Still an array, as in JSON
(
[0] => c
[1] => 2
)
)
*/
?>
Note that there are only two modes : the default one, which is close to the JSON syntax, and the All into array one, with JSON_OBJECT_AS_ARRAY.
disable_functions in directive
One good way to secure your PHP executable is to reduce the amount of compiled libraries in the binary. You can get the optimized list of compilation directives in the Exakat Ambassador report : for example, here are the optimized configuration for Sylius and Code igniter.
When you are provided with a standard PHP compilation, through an accredited channel, you can still disable some of the features by using the disable_functions and disable_classes PHP directives.
By configuring those directives, PHP will prevent the usage of those native functions and classes respectively.
The list of functions that can be disabled is build as a list of recommended functions to disable, while omitting the one being used in the audited code.
The list of classes to disable is actually very short : there was none in Exakat 1.4.3, but we’ll start adding some with Phar. Phar has been identified as an alternative vector for unserialize style of injection. Read more about it : New PHP Exploitation Technique Added.
Happy PHP code reviews
All the 357 analyzers are presented in the docs, including the uptight Logical Should Use Symbolic Operators : Logical operators come in two flavors : and / &&, || / or, ^ / xor. The symbolic operators have the level of precedence that you expect while the lettered operators may run into strange behavior, compared to other operators such as =.
Although correct code may be written with both versions, it may come back fast to bite you. Besides it is a common bug : 38% applications make use of them.
You can check all of the exakat reports at the gallery: exakat gallery.
Download Exakat on exakat.io, install it with Docker, upgrade it with ‘exakat.phar upgrade -u’ and like us on github.