Exakat 1.7.7 Review
Exakat 1.7.7 brings two new analyses : implode() with one argument, and a sneaky security vulnerability. It is the infamous check with integers. Show me the Exakat 1.7.7 review now!
Implode() with one argument
Implode() is a PHP native function, which has been around since the last millennium. I am pretty certain it was in PHP-FI and even PHP 1.0, although I can’t find any proof (yet). So, as a prehistoric relic, it behaves like any function would behave during the last millennium : strangely.
In particular, implode() accepts only one argument. You may call it directly, without any mention of the $glue, which is the string inserted between two array’s elements.
<pre class="wp-block-syntaxhighlighter-code">
<?php
$letters = range('a', 'z');
// Recommended syntax.
print implode('', $letters);
// Old and not-recommended behavior.
print implode($letters);
?>
</pre>
When the glue is omitted, the empty string is used. The above script displays the alphabet abcdefghijklmnopqrstuvwxyz.
While it is, and was, convenient, it is recommended to always mention the glue when calling implode(). Using the empty string as glue as the same effect as omitting it, and is more readable. It is also consistent with explode(), which requires the delimiter as first argument.
Although, you may also notice that explode() accepts a third argument : the limit size of the returned array. This prevents explode() from eating up too much memory if the arguments are big. The default limit is PHPINTMAX.
Also, explode() doesn’t accept an empty string as delimiter : if you want to split a string into separate elements, you should use str_split(). Or you can also access to those characters by simply accessing its offset in the string, with the array syntax.
<pre class="wp-block-syntaxhighlighter-code">
<?php
$alphabet = 'abcdefghijklmnopqrstuvwxyz';
$letters = str_split($alphabet);
print_r($letters);
?>
</pre>
Note that str_split() also accepts a 2nd argument, which is the length() of the elements in the resulting array.
<pre class="wp-block-syntaxhighlighter-code">
<?php
$alphabet = 'abcdefghijklmnopqrstuvwxyz';
$letters = str_split($alphabet, 2);
print_r($letters);
/*
Array (
[0] => ab
[1] => cd
[2] => ef
[3] => gh
[4] => ij
[5] => kl
[6] => mn
[7] => op
[8] => qr
[9] => st
[10] => uv
[11] => wx
[12] => yz
)
*/
?>
</pre>
Although it seems like a very old usage of implode(), we managed to find it in Mautic, OpenEMR, Tikiwiki, or Zurmo.
Validating data with integers
Comparing incoming data with integers has to be handled with care. In particular, PHP may automagically adjust the types of both the operands, and validating the content, while the actual value holds an injection. See it in action in the code below :
<pre class="wp-block-syntaxhighlighter-code">
<?php
if ($_GET['x'] == 2) {
echo $_GET['x'];
}
?>
</pre>
This is such a short example that you may wonder where is the trick. It is actually hiding in plain sight.
$_GET['x'] contains string, and only strings (and sometimes, arrays). When it is compared with 2, PHP understand that it can’t compare the string '2' with the integer 2, so it cast the string to integers, and then, compares the two results. If the cast value is equal to the integer value, then the condition succeeds. As long as $_GET['x'] is 2 (integer), or '2' a simple string, that code is valid.
The problem arises when $_GET['x'] contains a string that may be converted to an integer, but is not an actual integer. For example, $_GET['x'] maybe '2 <hr>' :
<pre class="wp-block-syntaxhighlighter-code">
<?php
$x = '2 <hr>';
if ($x == 2) {
echo $x;
}
?>
</pre>
The conversion is silent, and the injection is now real. This has yielded a number of security warning, including Type Juggling Authentication Bypass Vulnerability in CMS Made Simple.
The problem affects any comparison with integers : the value used in the condition is different from the one manipulated after the condition has been satisfied. Other situations similar to this one may be more explicit, and use (int) :
<pre class="wp-block-syntaxhighlighter-code">
<?php
$x = '2 <hr>';
if ((int) $x == 2) {
echo $x;
}
?>
</pre>
The root of the problem is that (int) $x is not the same as $x, since the type cast changes the content of the variable.
No triple equal needed
The immediate solution to this problem is to use the ===. Then, the comparison will always fail with $_GET, since incoming values are always strings. Which will lead to the explicit type cast we described just above, and the initial problem is back, with the same vulnerability.
<pre class="wp-block-syntaxhighlighter-code">
<?php
$x = '2 <hr>';
if ($x === 2) { // always fail, $x is a string
echo $x;
}
if ((int) $x === 2) { // pass, but raw $x is still infected
echo $x;
}
?>
</pre>
Typeint Then compare
The important part in this vulnerability is to compare and manipulate the integer version of the value, not its raw version. So, explicitly applying the type cast into another variable and then using that variable is the right solution.
<pre class="wp-block-syntaxhighlighter-code">
<?php
$x = '2 <hr>';
$validatedX = (int) $x;
if ($validatedX === 2) {
echo $validatedX;
}
if ($validatedX == 2) {
echo 2;
}
?>
</pre>
As a side note, and tactically using this simple script, using the compared literally instead of the original injected value fixes the problem too : this solution also avoids using the incoming data in the output of the script.
Exakat now reports usage of PHP superglobals in comparisons with integers : this is the root of most of the problem. We’ll run experiments with other data sources, and see how we can track more of those issues : this should be a good example of in-depth security.
The Weekly Audits: 2019, Week #14
Exakat includes a ‘weekly’ report: this report is built with a selection of five analyses. This means a short audit report, with few issues to review. This is not a lot to read them, and review them in your code. Everyone in the PHP community can focus on one of the classic coding problems and fix it. Talk about the weekly audit around you: you’ll find programmers facing the same challenges.
To obtain the ‘weekly’ audit, run an audit, and request the ‘Weekly’ report.
# Init the project (skip when it is already done)
php exakat.phar init -p <yourproject> -R https://github.com/Seldaek/monolog.git -git
# Run the project (skip when it is already done)
php exakat.phar project -p <yourproject>
# Export the weekly project (every monday)
php exakat.phar report -p <yourproject> -format Weekly
# Open projects/<yourproject>/weekly/index.html in your browser
Every week, you can find here 5 new analysis to review in your code. In fact, when your code is clean, you can also take a quick look at the upcoming
Weekly recommendations for PHP code review : 2018, week 2019-18
- Return True False : These conditional expressions return true/false, depending on the condition.
- Useless Final : When a class is declared final, all of its methods are also final by default.
- Suspicious Comparison : The comparison seems to be misplaced.
- Invalid Constant Name : There is a naming convention for PHP constant names.
- Pathinfo() Returns May Vary : pathinfo() function returns an array whose content may vary.
Happy PHP Code Reviews
All the 352 analyzers are presented in the docs, including the grand : Non-constant Index In Array: : Undefined constants, used as an array index, revert as strings and still produce the expected behavior. Yet, it also produced a warning.
It is still an unusual bug (26%), though it should be only affecting older applications.
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.