Exakat 0.12.15 review

Exakat 0.12.15 is the second October release. Exakat has three new analysis: one target security with uploaded files, another unanchored regex, and the last is about variables that may hold different types. Also, every audit now sports a name, for easier differentiation: after a while, multiples audits may look the same. Now, they won’t ! Let ‘s review that together!

Uploaded files security

The security theme gets two new analysis this week: the first is about uploaded files. There are many important rules when accepting whole files from the Internet:

Check the content and only accept as little formats as possible
Store the data outside the web folder, and add moderation on them
Avoid reusing any part of the uploaded filename directly

The last one is the one that interests us. The storage filename should avoid using the uploaded filename, in part, or in total.

<?php
if (!move_uploaded_file($_FILES['name']['tmp_name'], STORAGE.'/'.$_FILES['name']['filename']) {
 error('couldn\'t save uploaded file');
}
?>

The issue may be extended to part of the name: a classic error is to collect only the extension.


<?php
// Validate $_FILES['name']['tmp_name'] for valid content
$tmpName = getTempFileName();
$filename = pathinfo($_FILES['name']['filename']);
if (!move_uploaded_file($_FILES['name']['tmp_name'], STORAGE.'/'.$tmpName.'.'.$filename['extension']) {
 error('couldn\'t save uploaded file');
} else {
 // Validate $_FILES['name']['filename'] for database insertion
 saveInDB('uploadedFiles', $_FILES['name']['filename'], $tmpName);
}
?>

Now, the body of the file is now secure, but the extension may still bear some malicious data: here,


<?php
$filename = pathinfo("/a/b/c.yes<script>");
print_r($filename);
?>

This displays:


Array
(
 [dirname] => /a/b
 [basename] => c.yes<script>
 [extension] => yes<script>
 [filename] => c
)

As a general rule for security, always avoid reusing the uploaded file name, for any operation on the server, such as saving it. Save that data in a storage, after checking the usual suspects for injections (SQL, HTML and co).

Avoid unanchored regex

When checking for incoming data, avoid using unanchored regex. This a rule extracted from the CWE list:

The example below is adapted from this documentation:


<?php

$phone = $_GET['phone'];
if (preg_mach('/\d+-\d+-\d+-\d+-\d+/', $phone)) {
 echo ("your phone number : $phone");
} else {
error("malformed number!");
}

?>

$phone is tested against a valid regex, but it only checks that the data contains the expected value but doesn’t check if it only contains ONLY those data. As such, the following values are valid :

01-23-45-67-89
01-23-45-67-89abc
abc 01-23-45-67-89
01-23-45-67-89 <script>xss</script>

As a general rule, it is important to add both ^ and $ in the regex, so as to avoid missing any extra data being inserted, on top of the one that are wanted.

At that point, it is also important to remember that $ actually mark the end of a line or the string. The upgraded regex below will still let some data pass : namely, the one that contains new lines. An extra check on the presence of new lines, or the reduction of the string to its first line is important.


/^\d+-\d+-\d+-\d+-\d+$/
01-23-45-67-89 
<script>xss</script>

Multiple type variables

Although it is not possible to put simultaneously different types in a one variable, it is possible to change the type in a variable. For example:


<?php

$list = getNameList();

$list = implode(', ', $list);

echo $list;

?>

In this situation, the initial array stored in $list is actually replaced by the same array, in a string representation. Such stunt goes usually unnoticed, as the flow of the example leads to an echo. But on larger methods, it may be difficult to follow the actual content of the variable. At best, one have to track back several lines


<?php

$list = getNameList();

$list = implode(', ', $list);

$c = count($list); // $c can't be counted here anymore!

echo "($c element) : $list";

?>

Avoid reusing the same variable for different types. In case you need the conversion, it is advised to put the new value in a new variable. This saves the current one for later reuse, if needed, and keeps the code readable.

This analysis is currently being worked on.


<?php

$list = getNameList();

// Be explicit, or move that into a method call. 
$listAsString = implode(', ', $list);

$c = count($list); // $c can be counted here too!

echo "($c element) : $listAsString";

?>

This analysis is still under work, and is currently yielding a low volume of issues. Expect more of them in the future.

Audit names

Since version 0.12.15, a run audit gets a name. You’ll find this audit name in the Ambassador report, in the dashboard. This helps differentiate between reports.

Names for audits are build with an adjective and a first name from a member of the PHP community. This is a way for us to thanks everyone for their contribution. And, at this point, we are missing many many names still.

So, the list is way too short : there are only 158 names. You must know anyone to add ? Any contributors to PHP is welcome : enablers, documenter, elephpant owner, coder, hacker, configuration master, speaker, anonymous… Keep them coming! Tweet your suggestions or send them as a PR on https://github.com/exakat/exakat/blob/master/data/audit_names.ini.

Happy PHP code reviews

Exakat 0.12.15 is builds more support for the upcoming Exakat Saas version : it is now possible to run audits directly on exakat.io. No need for installation, just submit your repository and get the audits directly online. We’ll are now working on stabilizing the platform, and adding more features!
All the 320+ analyzers are presented in the docs, including the common ‘Simplify regex‘ , which suggest speeding up PHP by replacing preg_match() with the lighter strpos(). Try Exakat on exakat.io, download it it on https://github.com/exakat/exakat or upgrade it with ‘exakat.phar upgrade -u’.

Want to Keep in touch with us, subscribe to our newsletter !

Code auditing

Exakat 0.12.15 review