wunderio/code-quality

PHPCS_SecurityAudit is too strict

hkirsman opened this issue · 2 comments

hkirsman commented

While I understand the need for static security scanners, it can be quite confusing to see those warnings that block your commit. There's no good documentation which also stated in the package issue queue FloeDesignTechnologies/phpcs-security-audit#44 You have to google and dig into the code to understand what is going on. All in all you get smarter if you're up for it but it can easily lead to frustration or skipping validation the default action.

Some of the examples and answers that I've found

38 | WARNING | The use of function fnmatch() is discouraged 
(Drupal.Functions.DiscouragedFunctions.Discouraged)

On php.net it says "Warning: For now, this function is not available on non-POSIX compliant systems except Windows." As I understand it does not concern us as we are not using non-POSIX systems. https://en.wikipedia.org/wiki/POSIX#POSIX-certified macOS os certififed POSIX. Linux is in the https://en.wikipedia.org/wiki/POSIX#Mostly_POSIX-compliant but obviously it works. So good to know but why is it blocking the commit, one might ask

33 | WARNING | Function array_map() that supports callback detected
|         | (PHPCS_SecurityAudit.BadFunctions.CallbackFunctions.WarnCallbackFunctions)

From code I found link and all the other callback functions in PHP:

// From RIPS and http://stackoverflow.com/questions/3115559/exploitable-php-functions
public static function getCallbackFunctions() {
    return array(
        'ob_start', 'array_diff_uassoc', 'array_diff_ukey', 'array_filter', 'array_intersect_uassoc', 'array_intersect_ukey', 'array_map', 'array_reduce',
        'array_udiff_assoc', 'array_udiff_uassoc', 'array_udiff', 'array_uintersect_assoc', 'array_uintersect_uassoc', 'array_uintersect', 'array_walk_recursive',
        'array_walk', 'assert_options', 'uasort', 'uksort', 'usort', 'preg_replace_callback', 'spl_autoload_register', 'iterator_apply', 'call_user_func',
        'call_user_func_array', 'register_shutdown_function', 'register_tick_function', 'set_error_handler', 'set_exception_handler', 'session_set_save_handler',
        'sqlite_create_aggregate', 'sqlite_create_function'
    );
}

So the link goes to http://stackoverflow.com/questions/3115559/exploitable-php-functions and there it says

These functions accept a string parameter which could be used to call a function of the attacker's choice. Depending on the function the attacker may or may not have the ability to pass a parameter. In that case an Information Disclosure function like phpinfo() could be used.

So all in all you have to be careful and there's another comment on the issue:

This means that programmers should take extra care when using these functions, but if they where all banned then you wouldn't be able to get much done.

At the moment we are banning them if commit does not go through.

Would it be possible to allow warning to go through? I think in the configuration it's possible to change the rule from warning to error and vice versa. It could mean lot of custom configuration though.

Or should we disable the security thing by default?

Should we create documentation in the security package wiki for example?

mgalang commented

I tried a few things here and there. I tried setting warning_severity to phpcs.xml but this didn't work, the only thing that work is passing warning_severity to the phpcs call argument, but this results in blocking all warning from all rules.

jmarcil commented

phpcs-security-audit main author here. I can confirm that by design if you want to block commit you should be only looking at errors, not warning. The tool is designed to report warnings for manual review required, and errors when it's pretty sure it's a vulnerability.

You should be able to change the severity inside the phpcs.xml file, but right now we have a bug preventing you to do that FloeDesignTechnologies/phpcs-security-audit#38.

Note that --warning-severity=0 does not change the severity value of the findings, but rather change the default cutoff on showing warnings. I suspect that this is broken inside the config from the same bug as I just described above. See https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#filtering-errors-and-warnings-based-on-severity for details.

Last but not least, there is still a way for you to block commits on all error or warning. You have to change the way you suppress the finding. Instead of changing the rule itself, you should consider inline annotation in the code that is generating the issue. This is very useful on high importance code base as you want to ensure a warning is raised on new code that is at risk, but not raising the same warning over and over on the same line of code that you previously manually identified as safe. See https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignoring-parts-of-a-file.

  • 👍2