When i attempt to run phpcs with the example rulesets, I get the following errors:

Fatal error: Uncaught exception 'PHP_CodeSniffer_Exception' with message 'Referenced sniff Security.BadFunctions.Asserts does not exist' in /Users/tim/dev/bcc_test/vendor/squizlabs/php_codesniffer/CodeSniffer.php on line 876

It seems like it is unable to convert the rules in the ruleset file into paths to the PHP files. Could you possibly provide any info on how this works and or a way to fix the issue?

The way the project is currently set up with the standard name being Security, but the namespace name being PHPCS_SecurityAudit is non-standard and breaks expectations.

It is the cause of issues #38 and #45 and the reason hacks like the symlink are needed, while the normal installed_paths functionality in PHPCS will not work for this standard.

I'd be willing to fix this for this project, but only if a PR to that effect would be considered welcome.

It will involve renaming all namespaces, removing the symlink and more, but can be done without breaking existing rulesets using Security.Category.SniffName references.

Would you be willing to consider such a PR ?

Hi,

I've just built phpcs from master 66845e0efab9ae09ae6545c1e49455151468b49a using docker build -t phpcs .. After I'm running it with docker run -i -t --rm -v "$(pwd)":/opt/mount phpcs I'm getting ERROR: Referenced sniff "Security.BadFunctions.Asserts" does not exist.

It looks like bug was introduced in c36e8c6fae2be53b37e361e7e314302de0fbeb5c since I've just build & run suing the same commands but from b16843d48abb1809159eb500a51c5153e1ed8e31 (it's a commit right before c36e8c6fae2be53b37e361e7e314302de0fbeb5c) and all works OK.

Environment

• macOS 10.15.3
• Docker version 19.03.5, build 633a0ea

Hey,

I was wondering if there is any plan to add explanation of found issues to this project.

For example, we don't really understand why Function array_filter() that supports callback detected is detected as vulnerability. (Identifier:PHPCS_SecurityAudit.BadFunctions.CallbackFunctions.WarnFringestuff)

Is similar feature planned somewhere in the future?

This is an initial setup for the unit tests.

To allow for testing with different configuration variables - ParanoiaMode and CmsFramework -, I've chosen to create a setup which will automatically set these variables based on the test case file name.

This initial setup includes unit tests for the BadFunctions.Backticks sniff, as well as three commits making small fixes to the sniff.

Related to #57

Commit summary

CI: Initial unit test setup

• Adds PHPUnit to the dev requirements in the composer.json file.
  • As the PHPCS PHPUnit framework isn't compatible with PHPUnit 7 until PHPCS 3.2.3, I've chosen to explicitly only support PHPUnit 4, 5, 6 via Composer to prevent having to make extra allowances for PHPUnit 7 in the Travis script.
  • Includes raising the minimum PHPCS requirement from PHPCS 3.0.2 to 3.1.0 as PHPCS 3.0.2 does not yet contain the PHPUnit bootstrap.php file for cross-version PHPUnit compatibility.
  • Includes adding a convenience script to easily run the unit tests.
• Adds a phpunit.xml.dist configuration file.
• Adds a phpunit-bootstrap.php file to sort out the autoloading for the unit tests and to prevent PHPCS from trying to run unit tests for other installed external standards.
• Adds an abstract base test case for the security sniffs which handles setting the configuration variables for the tests. The configuration variables are set based on the test case file name. See the explanation in the file docblock.
• Adds running the unit tests to the Travis configuration. As the full unit test matrix can take a little while to run, I've set this up in a way that on pushes only a quick check is being run and that the full build is only run on PRs and merges to master. The updated build matrix takes compatibility of PHPCS with various PHP versions into account.

Includes:

• Adding the test related files to the .gitattributes export-ignore list so they don't get shipped in the release packages.
• Adding typical PHPUnit overload/cache files to the .gitignore file.

BadFunctions/Backticks: add unit tests

BadFunctions/Backticks: bug fix - report on each variable

The sniff would only report on the first variable found in the shell command, not on each variable.

Even though there would be a notice, the level could have been warning as the first variable seen was a non-user input variable, while a more serious error for a subsequently used user input variable would not be reported.

This has now been fixed by changing the check for a variable to a loop which will report a separate error/warning for each variable encountered in the command.

BadFunctions/Backticks: error message line precision

A backtick-shell command can be spread out over several lines.

This minor change make it so the error/warning will be reported on the line containing the offending variable, not the line containing the open-backtick, as these may not be the same.

BadFunctions/Backticks: minor efficiency fix

Only set variables if they are actually needed, not before.

As partially discussed in #50, now the namespace has been fixed, sniff specific unit tests could be added based on the PHPCS native unit test framework.

I'd be willing to do the initial setup for this if it helps,.

Open questions:

• Are you interested in this ?
• What to do with all the unit tests I can come up with which would currently fail ?
• Anything I need to know before I start this ?

• Add some badges.
• Improve some of the grammar of various texts.
• Improve use of markdown.
• Let the default suggested way of running the tooling be with --standard=Security instead of pointing to a specific example ruleset.
• Update the example output.

The file name may be build up of several parts. The sniff - until now - would only look at the first text string token for the extension, while the extension will normally be in the last text string token.

This commit changes the sniff to walk back from the end of the statement instead of forward.

Includes unit test.

I get this:

Fatal error: Uncaught Error: Call to undefined method Security_Sniffs_Utils::drupal_parse_info_format() in /Users/alejandromoreno/projects/[...]/vendor/pheromone/phpcs-security-audit/Security/Sniffs/Drupal7/AdvisoriesContribSniff.php:34

but apart of that, the --standard=example_drupal7_ruleset.xml does not work, it points to use Security instead:

./vendor/bin/phpcs --standard=Security --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md docroot/sites/all/modules/custom/

Thanks in advance

Related to #56, this adds an initial set of CI checks to be run via Travis.

To turn this check on:

• Go to the Travis.com website.
• Log in via GitHub ID.
• Let it sync with GitHub to retrieve the repositories & turn the check on for this repo.

I've tested this PR - see here for a passing build: https://travis-ci.org/jrfnl/phpcs-security-audit/builds/651998291

Rulesets: Add XSD schema tags (PHPCS 3.2+/3.3.2+)

As of PHPCS 3.2.0, PHPCS includes an XSD schema for rulesets which defines what can be used in the XML ruleset file.

In PHPCS 3.3.0 a new array format for properties was introduced and as of PHPCS 3.3.2, the new array format is now also covered by this schema.

This commit adds the schema tags to the ruleset.xml file and the example rulesets.

Rulesets: tidy up

Tidy up the XML rulesets so they can pass validation against the XSD as well as for XML code style.

QA: add Parallel-Lint for easy syntax checking of the repo

Includes adding a composer lint convenience script to run the command.

The script as-is, is platform independent and will work on both Linux, Mac as well as Windows and respects the PHP version used by Composer.

Travis: add initial CI check

• Validate the composer.json file.
• Validate the XML ruleset files against the PHPCS XSD schema.
• Verify the XML code style consistency of the ruleset files.
• Run PHP linter over all PHP files on PHP 5.4, 7.4 and nightly (= PHP 8).

This sniff throws a warning if you use == instead of ===. Most of the time it's best to use === to prevent accidentally having types converted and evaluating in an unexpected way.

I get phpcs: PHPCS_SecurityAudit.BadFunctions.FilesystemFunctions.WarnFilesystem: Filesystem function file_put_contents() detected with dynamic parameter with this: file_put_contents('test.txt', 1, FILE_APPEND); So FILE_APPEND is a dynamic parameter? If its the content or filename, i would say okay, but the flag?

Hi! Is it ok that the ruleset complains about every array_filter, array_map, array_reduce call? What's wrong with functions supporting callbacks when those callbacks are explicitly passed as closures?

The following code fails to scan due to line 6 (use function usort).

<?php
namespace Demo;

use function strlen;
use function usort;
use function var_dump;


$data = [
    "test-one",
    "test-three",
    "test-two",
    "test-four",
];

$sortCallback = function ($a, $b) {
    $lA = strlen($a);
    $lB = strlen($b);

    if ($lA == $lB) {
        return 0;
    }

    return  $lA < $lB ? -1 : 1;
};

usort($data, $sortCallback);
var_dump($data);

Getting error: An error occurred during processing; checking has been aborted. The error message was: Undefined index: parenthesis_closer in ...\Sniffs\BadFunctions\CallbackFunctionsSniff.php on line 34

Can you allow for native function imports?

From a fresh build of Ubuntu 20 LTS with 7.4, running composer install after cloning produces:

Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - dealerdirect/phpcodesniffer-composer-installer[v0.4.1, ..., v0.6.2] require composer-plugin-api ^1.0 -> found composer-plugin-api[2.1.0] but it does not match the constraint.
    - Root composer.json requires dealerdirect/phpcodesniffer-composer-installer ^0.4.1 || ^0.5 || ^0.6 -> satisfiable by dealerdirect/phpcodesniffer-composer-installer[v0.4.1, ..., v0.6.2].

You get a little further Installing with composer require --dev pheromone/phpcs-security-audit, but the Security standards are not available

smurf:/home/ubuntu/static4# composer require --dev pheromone/phpcs-security-audit
Do not run Composer as root/super user! See https://getcomposer.org/root for details
Continue as root/super user [yes]? 
Using version ^2.0 for pheromone/phpcs-security-audit
./composer.json has been created
Running composer update pheromone/phpcs-security-audit
Loading composer repositories with package information
Updating dependencies
Lock file operations: 2 installs, 0 updates, 0 removals
  - Locking pheromone/phpcs-security-audit (2.0.1)
  - Locking squizlabs/php_codesniffer (3.6.0)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 2 installs, 0 updates, 0 removals
  - Installing squizlabs/php_codesniffer (3.6.0): Extracting archive
  - Installing pheromone/phpcs-security-audit (2.0.1): Extracting archive
Generating autoload files

smurf:/home/ubuntu/static4# vendor/bin/phpcs -i 
The installed coding standards are PEAR, PSR12, Squiz, Zend, PSR1, MySource and PSR2
smurf/home/ubuntu/static4# 

Variables, constants and function calls are not included in the report if concatenated with something which has already been included. Is there any way to override this behaviour other than adding the safe constant to getXSSMitigationFunctions?

echo SAFE_CONSTANT . vulnerableFunction();
// Warning: Possible XSS detected with SAFE_CONSTANT on echo

When reviewing the report, it can give the impression that an issue is safe when it is not.

Here are some examples from a quick test:

/**
 * Warning: Possible XSS detected with CONSTANT on echo
 * Warning: Possible XSS detected with $var on echo
 */
echo CONSTANT; echo $var;

/**
 * Warning: Possible XSS detected with CONSTANT on echo
 */
echo CONSTANT . $var;

/**
 * Error: Easy XSS detected because of direct user input with $_GET on echo
 * Error: Easy XSS detected because of direct user input with $_GET on echo
 */
echo $_GET['one'] . $_GET['two'];

/**
 * Warning: Possible XSS detected with $varOne on echo
 */
echo $varOne . $varTwo;

/**
 * Warning: Possible XSS detected with SAFE_CONSTANT on echo
 */
echo SAFE_CONSTANT . vulnerableFunction();

Many thanks