Skip to content

Commit d7a55ea

Browse files
committed
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.
1 parent 93ca2d7 commit d7a55ea

File tree

3 files changed

+8
-4
lines changed

3 files changed

+8
-4
lines changed

Security/Sniffs/BadFunctions/BackticksSniff.php

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,13 @@ public function register() {
2828
public function process(File $phpcsFile, $stackPtr) {
2929
$utils = \PHPCS_SecurityAudit\Security\Sniffs\UtilsFactory::getInstance();
3030
$tokens = $phpcsFile->getTokens();
31-
$closer = $phpcsFile->findNext(T_BACKTICK, $stackPtr + 1, null, false, null, true);
31+
$closer = $phpcsFile->findNext(T_BACKTICK, $stackPtr + 1, null, false, null, true);
3232
if (!$closer) {
3333
return;
3434
}
35-
$s = $stackPtr + 1;
36-
$s = $phpcsFile->findNext(T_VARIABLE, $s, $closer);
37-
if ($s) {
35+
36+
$s = $stackPtr;
37+
while (($s = $phpcsFile->findNext(T_VARIABLE, ($s + 1), $closer)) !== false) {
3838
$msg = 'System execution with backticks detected with dynamic parameter';
3939
if ($utils::is_token_user_input($tokens[$s])) {
4040
$phpcsFile->addError($msg . ' directly from user input', $stackPtr, 'ErrSystemExec');

Security/Tests/BadFunctions/BackticksUnitTest.inc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ $output = `$request['field']`; // Warning.
1010

1111
$output = `git blame --date=short "$filename"`; // Warning.
1212

13+
$output = `git blame --date=$_POST['key'] "$filename"`; // Warning + error.
14+
1315
// Incomplete command. Ignore.
1416
// Intentional parse error. This should be the last test in the file.
1517
$output = `ls

Security/Tests/BadFunctions/BackticksUnitTest.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ public function getErrorList($testFile = '')
2626
case 'BackticksUnitTest.inc':
2727
return [
2828
9 => 1,
29+
13 => 1,
2930
];
3031

3132
case 'BackticksUnitTest.Drupal7.inc':
@@ -70,6 +71,7 @@ public function getWarningList($testFile = '')
7071
7 => 1,
7172
8 => 1,
7273
11 => 1,
74+
13 => 1,
7375
];
7476

7577
case 'BackticksUnitTest.Drupal7.inc':

0 commit comments

Comments
 (0)