Exakat 1.8.0 Review

Exakat 1.8.0 main new feature is the support for ‘in-code review’ : using .exakat.yml, you may run an audit from within the repository. This is particularly useful for CI. More PHP 7.4 support was added, with the upcoming change of precedence with addition and concatenation. For the current versions, Exakat is now reporting useless arguments in methods, and validation mistakes with functions returning -1. This is all you ever wanted to know about the Exakat 1.8.0 review!

Support for PHP 7.4

PHP 7.4 is packing up a lot of new features, and that will last until July 18th. This is the date of the feature freeze, where all expected features will be known, and all other features will be pushed to PHP 8.0, the following year. We are also seeing the first incompatibility with PHP 7.4 forming, so it is time to get prepared.

In particular, addition and concatenation are now of different precedence, while ternary operators are requiring parenthesis usage. This may affect your code.

Addition, concatenation precedence

Until PHP 7.3, addition and subtraction, had the same precedence than concatenation. That is, higher than comparison, but lower than multiplication. At least, that doesn’t change.

On the other hand, addition and concatenation have the same precedence, so their order of application depends on the order of writing. For example :

<?php   
  $a = 1;
  $b = 2;
  echo '$a + $b = ' . $a + $b;
?>

This displays 2. In fact, PHP finds the concatenation . first, and concatenate '$a + $b = ' with 1 first. This leads to a first string, '$a + $b = 1', which is then added to the variable $b. Since the first string can’t be turned into an integer, it is then turned into 0, with a nice warning : PHP Warning: A non-numeric value encountered. Eventually, 2 is displayed.

As humans, we’d like to read that $a + $b is computed first, then concatenated to the string, which acts as a label. This would be the case with parentheses :

<?php   
  $a = 1;
  $b = 2;
  echo '$a + $b = ' . ($a + $b);
?>

PHP 7.4 changes the precedence for concatenation and addition, leaving addition with a higher precedence. The previous code will then work without the parenthesis.

Until then, it is important to always add the parenthesis. It will also be important to keep those parenthesis when backward compatibility is important.

A new analysis ‘Php/ConcatAndAddition’ was added to Exakat to cover those situations. It reports any mix of ., + and - operators that are lacking parenthesis.

This analysis applies to every PHP version. This is not common, as such migration analysis are usually targeting one specific version, or a range of version. Yet, the problem of precedence may affect any piece of code, and it is recommended to check them, whatever the version.

Useless Arguments

Arguments may be useless for two reasons : they are never used inside the function (or method), or they are always the same.

Unused Arguments

Unused argument is bad practice. They must be provided at a function call, but later, they are not used. This is a waste of argument, and a pain for any API user. You can find those in your code with the Unused arguments rule.

  
<?php   
  function diviser($a, $b, $c) {  
    return intdiv($a, $c);
  }
?>

Arguments are not considered unused if they are enforced by an interface or an abstract method. Those are compulsory, and require update of the parent, and may be, the sister classes.

Always The Same Argument

Now, consider the following code :

<?php   
  function foo($a, $b) {  
    return intdiv($a, $b);
  }

  foo($a, 2);
  foo(100, 2);
  foo(201, 2);
?>

This piece of code is seriously academic, but the content is the following : one function definition, with 2 arguments; three usages of the function, where the second argument is always the same, 2. This is a strange piece of code, but it serves our purpose, and there is no point in displaying 3 pages of code, just to hide the calls to foo.

So, function foo was defined with 2 arguments, but the second argument is always the same. This leads to consider that this argument is actually useless. Think that if it is moved inside the function, and hard-coded, that will require fewer arguments.

This example illustrates the difference between planning and application. The foo function was built to return the remainder of the division of $a by $b. It does make sense assuming that a large range of values could be used with this function, and indeed, the native PHP function intval()does accept a lot of values (although a check on 0 would be nice here).

On the other hand, every call to this function uses the same second argument. The function may be simplified by reducing the number of arguments, and the number of decisions to make when calling it.

Keeping the arguments is a bet on the future : it may be useful later. Removing it is a check on the past : we haven’t used this. Depending on how the feature is perceived, you will be able to decide for your own code.

Validation mistakes

Where is this going wrong?

  
<?php   
  if (openssl_verify($data, $signature, $public_key)) {  
    user_login($user);
  }
?>

The variables are not important here, as the problem lies with openssl_verify and its usage. openssl_verify checks a signature for some data, with a public key. It return 1 in case of success, and 0 in case of failure. Since 1 is true for PHP, and 0 is false, then the condition above applies.

The trouble is that openssl_verify() returns -1 in case of error. For example, when the key is not a valid public key. This means that the previous condition is now returning -1, and in PHP, -1 is true. To gain access to the user, one simply has to botch the signature, and the error will grant way.

This problem is similar to the infamous Strpos()-like Comparison, where the absence of result (false) is mistaken with a value found on the first offset(0). Here, success and errors are the same.

The safe way with this function is to compare it to the actual return value, either 0 or 1. openssl_verify() return integers. This will ensure that 1 and -1 are not mistaken one for another.

There are another 13 native PHP functions that return -1 in case of error, such as pgfieldnum, pcntl_waitpid, and several openssl functions, like opensslpkcs7verify(), opensslx509checkpurpose(). Avoid using them as strait conditions, and always compare their result to a value.

Thanks to RIPS for raising awareness about this security problem, with their very challenging series of tweet : Can you spot the vulnerability? (openssl_verify). The problem has been at large for a while now : Incorrect Signature Verification.

The Weekly Audits: 2019, Week #21

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-21

__toString() Throws Exception : Magical method __toString() can’t throw exceptions.
Mismatched Default Arguments : Arguments are relayed from one method to the other, and the arguments have different default values.
Undefined ::class : ::class doesn’t check if a corresponding class exists.
Callback Needs Return : When used with array_map() functions, the callback must return something.
Incompatible Signature Methods : Methods should have the same signature when being overwritten.

Happy PHP Code Reviews

All the 352 analyzers are presented in the docs, including the grand : Test Then Cast: A test is run on the cast value, but the raw value is later used. It is a common bug (32%)

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.

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

Code auditing

Exakat 1.8.0 review