Skip to content

[Enhancement] DiscouragedFunction rule improvement #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 20, 2019
Merged

[Enhancement] DiscouragedFunction rule improvement #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f53758d
[Enhancement] DiscouragedFunction rule improvement
lenaorobei Mar 19, 2019
e8d0916
[Enhancement] DiscouragedFunction rule improvement
lenaorobei Mar 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 8 additions & 39 deletions ...2/Sniffs/PHP/DiscouragedFunctionSniff.php → ...fs/Functions/DiscouragedFunctionSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@
* Copyright © Magento. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento2\Sniffs\PHP;
namespace Magento2\Sniffs\Functions;

use PHP_CodeSniffer\Files\File;
use PHP_CodeSniffer\Standards\Generic\Sniffs\PHP\ForbiddenFunctionsSniff;

/**
Expand All @@ -20,13 +19,19 @@ class DiscouragedFunctionSniff extends ForbiddenFunctionsSniff
*/
protected $patternMatch = true;

/**
* If true, an error will be thrown; otherwise a warning.
*
* @var boolean
*/
public $error = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please validate the list against https://stackoverflow.com/questions/3115559/exploitable-php-functions and ask security team to review.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.


/**
* List of patterns for forbidden functions.
*
* @var array
*/
public $forbiddenFunctions = [
'^assert$' => null,
'^bind_textdomain_codeset$' => null,
'^bindtextdomain$' => null,
'^bz.*$' => null,
Expand All @@ -39,7 +44,6 @@ class DiscouragedFunctionSniff extends ForbiddenFunctionsSniff
'^chroot$' => null,
'^com_load_typelib$' => null,
'^copy$' => null,
'^create_function$' => null,
'^curl_.*$' => null,
'^cyrus_connect$' => null,
'^dba_.*$' => null,
Expand All @@ -52,7 +56,6 @@ class DiscouragedFunctionSniff extends ForbiddenFunctionsSniff
'^dirname$' => null,
'^dngettext$' => null,
'^domxml_.*$' => null,
'^exec$' => null,
'^fbsql_.*$' => null,
'^fdf_add_doc_javascript$' => null,
'^fdf_open$' => null,
Expand Down Expand Up @@ -93,18 +96,15 @@ class DiscouragedFunctionSniff extends ForbiddenFunctionsSniff
'^parse_str$' => null,
'^parse_url$' => null,
'^parsekit_compile_string$' => null,
'^passthru$' => null,
'^pathinfo$' => null,
'^pcntl_.*$' => null,
'^posix_.*$' => null,
'^pfpro_.*$' => null,
'^pfsockopen$' => null,
'^pg_.*$' => null,
'^php_check_syntax$' => null,
'^popen$' => null,
'^print_r$' => null,
'^printf$' => null,
'^proc_open$' => null,
'^putenv$' => null,
'^readfile$' => null,
'^readgzfile$' => null,
Expand All @@ -122,14 +122,12 @@ class DiscouragedFunctionSniff extends ForbiddenFunctionsSniff
'^setcookie$' => null,
'^setlocale$' => null,
'^setrawcookie$' => null,
'^shell_exec$' => null,
'^sleep$' => null,
'^socket_.*$' => null,
'^stream_.*$' => null,
'^sybase_.*$' => null,
'^symlink$' => null,
'^syslog$' => null,
'^system$' => null,
'^touch$' => null,
'^trigger_error$' => null,
'^unlink$' => null,
Expand Down Expand Up @@ -220,34 +218,5 @@ class DiscouragedFunctionSniff extends ForbiddenFunctionsSniff
'^is_null$' => 'strict comparison "=== null"',
'^intval$' => '(int) construction',
'^strval$' => '(string) construction',
'^md5$' => 'improved hash functions (SHA-256, SHA-512 etc.)',
'^serialize$' => 'json_encode',
'^unserialize$' => 'json_decode',
];

/**
* Generates warning for this sniff.
*
* @param File $phpcsFile The file being scanned.
* @param int $stackPtr The position of the forbidden function in the token array.
* @param string $function The name of the forbidden function.
* @param string $pattern The pattern used for the match.
*
* @return void
*/
protected function addError($phpcsFile, $stackPtr, $function, $pattern = null)
{
$data = [$function];
$warningMessage = 'The use of function %s() is discouraged';
$warningCode = 'Found';
if ($pattern === null) {
$pattern = $function;
}
if ($this->forbiddenFunctions[$pattern] !== null) {
$warningCode .= 'WithAlternative';
$data[] = $this->forbiddenFunctions[$pattern];
$warningMessage .= '; use %s instead.';
}
$phpcsFile->addWarning($warningMessage, $stackPtr, $warningCode, $data);
}
}
41 changes: 41 additions & 0 deletions Magento2/Sniffs/Security/InsecureFunctionSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?php
/**
* Copyright © Magento. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento2\Sniffs\Security;

use PHP_CodeSniffer\Standards\Generic\Sniffs\PHP\ForbiddenFunctionsSniff;

/**
* Detects the use of insecure functions.
*/
class InsecureFunctionSniff extends ForbiddenFunctionsSniff
{
/**
* If true, an error will be thrown; otherwise a warning.
*
* @var boolean
*/
public $error = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it a warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's under Severity 9 warnings: Possible security and issues that may cause bugs. section.
We cannot reject extensions because of such functions right now. We will work iteratively to make rules stricter.


/**
* List of patterns for forbidden functions.
*
* @var array
*/
public $forbiddenFunctions = [
'assert' => null,
'create_function' => null,
'exec' => null,
'md5' => 'improved hash functions (SHA-256, SHA-512 etc.)',
'passthru' => null,
'pcntl_exec' => null,
'popen' => null,
'proc_open' => null,
'serialize' => 'json_encode',
'shell_exec' => null,
'system' => null,
'unserialize' => 'json_decode',
];
}
0 ...Tests/PHP/DiscouragedFunctionUnitTest.inc → ...Functions/DiscouragedFunctionUnitTest.inc
View file
Open in desktop
File renamed without changes.
11 changes: 1 addition & 10 deletions ...Tests/PHP/DiscouragedFunctionUnitTest.php → ...Functions/DiscouragedFunctionUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* Copyright © Magento. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento2\Tests\PHP;
namespace Magento2\Tests\Functions;

use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;

Expand All @@ -26,7 +26,6 @@ public function getErrorList()
public function getWarningList()
{
return [
3 => 1,
6 => 1,
7 => 1,
9 => 1,
Expand All @@ -40,7 +39,6 @@ public function getWarningList()
28 => 1,
30 => 1,
32 => 1,
34 => 1,
36 => 1,
37 => 1,
38 => 1,
Expand All @@ -63,7 +61,6 @@ public function getWarningList()
65 => 1,
67 => 1,
69 => 1,
71 => 1,
73 => 1,
74 => 1,
76 => 1,
Expand Down Expand Up @@ -122,7 +119,6 @@ public function getWarningList()
166 => 1,
169 => 1,
171 => 1,
173 => 1,
175 => 1,
177 => 1,
179 => 1,
Expand All @@ -131,10 +127,8 @@ public function getWarningList()
184 => 1,
185 => 1,
187 => 1,
189 => 1,
191 => 1,
193 => 1,
195 => 1,
197 => 1,
199 => 1,
201 => 1,
Expand All @@ -152,7 +146,6 @@ public function getWarningList()
229 => 1,
231 => 1,
233 => 1,
235 => 1,
237 => 1,
239 => 1,
241 => 1,
Expand All @@ -162,7 +155,6 @@ public function getWarningList()
247 => 1,
249 => 1,
251 => 1,
253 => 1,
255 => 1,
258 => 1,
261 => 1,
Expand Down Expand Up @@ -257,7 +249,6 @@ public function getWarningList()
458 => 1,
460 => 1,
462 => 1,
464 => 1,
];
}
}
25 changes: 25 additions & 0 deletions Magento2/Tests/Security/InsecureFunctionUnitTest.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php

assert($a === true);

exec('echo 1;');

passthru('echo 1;');

shell_exec('echo 1;');

system('echo 1;');

md5($text);

serialize([]);

unserialize('');

popen('echo 1;');

proc_open('echo 1;');

create_function('args', 'code');

pcntl_exec('path/goes/here');
43 changes: 43 additions & 0 deletions Magento2/Tests/Security/InsecureFunctionUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php
/**
* Copyright © Magento. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento2\Tests\Security;

use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;

/**
* Class InsecureFunctionUnitTest
*/
class InsecureFunctionUnitTest extends AbstractSniffUnitTest
{
/**
* @inheritdoc
*/
public function getErrorList()
{
return [];
}

/**
* @inheritdoc
*/
public function getWarningList()
{
return [
3 => 1,
5 => 1,
7 => 1,
9 => 1,
11 => 1,
13 => 1,
15 => 1,
17 => 1,
19 => 1,
21 => 1,
23 => 1,
25 => 1,
];
}
}
16 changes: 15 additions & 1 deletion Magento2/ruleset.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
<rule ref="Magento2.Security.IncludeFile">
<severity>10</severity>
<type>error</type>
<exclude-pattern>*/Test/*</exclude-pattern>
</rule>
<rule ref="Magento2.Security.LanguageConstruct">
<severity>10</severity>
Expand All @@ -58,6 +59,7 @@
<rule ref="Magento2.Security.Superglobal.SuperglobalUsageError">
<severity>10</severity>
<type>error</type>
<exclude-pattern>*/lib/*</exclude-pattern>
</rule>
<rule ref="Magento2.Strings.ExecutableRegEx">
<severity>10</severity>
Expand Down Expand Up @@ -85,14 +87,16 @@
<rule ref="Magento2.PHP.DateTime">
<severity>9</severity>
<type>warning</type>
<exclude-pattern>*/lib/*</exclude-pattern>
</rule>
<rule ref="Magento2.PHP.DiscouragedFunction">
<rule ref="Magento2.Security.InsecureFunction">
<severity>9</severity>
<type>warning</type>
</rule>
<rule ref="Magento2.Security.Superglobal.SuperglobalUsageWarning">
<severity>9</severity>
<type>warning</type>
<exclude-pattern>*/lib/*</exclude-pattern>
</rule>
<rule ref="Magento2.Security.XssTemplate">
<include-pattern>*.phtml</include-pattern>
Expand All @@ -116,6 +120,7 @@
<rule ref="Magento2.Classes.ObjectInstantiation">
<severity>8</severity>
<type>warning</type>
<exclude-pattern>*/Test/*</exclude-pattern>
</rule>
<rule ref="Magento2.Exceptions.DirectThrow">
<severity>8</severity>
Expand All @@ -125,9 +130,17 @@
<severity>8</severity>
<type>warning</type>
</rule>
<rule ref="Magento2.Functions.DiscouragedFunction">
<severity>8</severity>
<type>warning</type>
<exclude-pattern>*/lib/*</exclude-pattern>
<exclude-pattern>*/Test/*</exclude-pattern>
</rule>
<rule ref="Magento2.Functions.StaticFunction">
<severity>8</severity>
<type>warning</type>
<exclude-pattern>*/Test/*</exclude-pattern>
<exclude-pattern>*/Setup/*</exclude-pattern>
</rule>
<rule ref="Magento2.Files.LineLength">
<severity>8</severity>
Expand Down Expand Up @@ -226,6 +239,7 @@
<rule ref="Squiz.Functions.GlobalFunction">
<severity>7</severity>
<type>warning</type>
<exclude-pattern>*/Test/*</exclude-pattern>
</rule>
<rule ref="Squiz.Operators.IncrementDecrementUsage">
<severity>7</severity>
Expand Down