Skip to content

Commit de77765

Browse files
GaryJonesGaryJones
authored
Merge pull request #846 from Automattic/feature/security-phpfilterfunctions-sniff-review
2 parents c949fb6 + 824bb1b commit de77765

File tree

3 files changed

+176
-59
lines changed

3 files changed

+176
-59
lines changed

‎WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php

Lines changed: 66 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,17 @@
99

1010
namespace WordPressVIPMinimum\Sniffs\Security;
1111

12+
use PHP_CodeSniffer\Util\Tokens;
13+
use PHPCSUtils\Utils\PassedParameters;
1214
use WordPressCS\WordPress\AbstractFunctionParameterSniff;
1315

1416
/**
1517
* This sniff ensures that proper sanitization is occurring when PHP's filter_* functions are used.
1618
*
19+
* {@internal The $options parameter for filter_var_array() and filter_input_array() can take either an
20+
* integer (filter constant) or an array with options, which could include an option setting the filter constant.
21+
* At this time, this sniff does not handle an array with options being passed.}
22+
*
1723
* @since 0.4.0
1824
*/
1925
class PHPFilterFunctionsSniff extends AbstractFunctionParameterSniff {
@@ -26,15 +32,27 @@ class PHPFilterFunctionsSniff extends AbstractFunctionParameterSniff {
2632
protected $group_name = 'php_filter_functions';
2733

2834
/**
29-
* Functions this sniff is looking for.
35+
* Functions this sniff is looking for and information on which parameter to check for in those function calls.
3036
*
31-
* @var array<string, bool> Key is the function name, value irrelevant.
37+
* @var array<string, array{param_position: int, param_name: string}> Key is the function name.
3238
*/
3339
protected $target_functions = [
34-
'filter_var' => true,
35-
'filter_input' => true,
36-
'filter_var_array' => true,
37-
'filter_input_array' => true,
40+
'filter_var' => [
41+
'param_position' => 2,
42+
'param_name' => 'filter',
43+
],
44+
'filter_input' => [
45+
'param_position' => 3,
46+
'param_name' => 'filter',
47+
],
48+
'filter_var_array' => [
49+
'param_position' => 2,
50+
'param_name' => 'options',
51+
],
52+
'filter_input_array' => [
53+
'param_position' => 2,
54+
'param_name' => 'options',
55+
],
3856
];
3957

4058
/**
@@ -60,30 +78,52 @@ class PHPFilterFunctionsSniff extends AbstractFunctionParameterSniff {
6078
* normal file processing.
6179
*/
6280
public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
63-
if ( $matched_content === 'filter_input' ) {
64-
if ( count( $parameters ) === 2 ) {
65-
$message = 'Missing third parameter for "%s".';
66-
$data = [ $matched_content ];
67-
$this->phpcsFile->addWarning( $message, $stackPtr, 'MissingThirdParameter', $data );
68-
}
81+
$param_position = $this->target_functions[ $matched_content ]['param_position'];
82+
$param_name = $this->target_functions[ $matched_content ]['param_name'];
6983

70-
if ( isset( $parameters[3], $this->restricted_filters[ $parameters[3]['raw'] ] ) ) {
71-
$message = 'Please use an appropriate filter to sanitize, as "%s" does no filtering, see: http://php.net/manual/en/filter.filters.sanitize.php.';
72-
$data = [ strtoupper( $parameters[3]['raw'] ) ];
73-
$this->phpcsFile->addWarning( $message, $stackPtr, 'RestrictedFilter', $data );
74-
}
75-
} else {
76-
if ( count( $parameters ) === 1 ) {
77-
$message = 'Missing second parameter for "%s".';
78-
$data = [ $matched_content ];
79-
$this->phpcsFile->addWarning( $message, $stackPtr, 'MissingSecondParameter', $data );
84+
$target_param = PassedParameters::getParameterFromStack( $parameters, $param_position, $param_name );
85+
if ( $target_param === false ) {
86+
/*
87+
* Check for PHP 5.6+ argument unpacking.
88+
*
89+
* No need for extensive defensive coding, we already know this is syntactically a valid function call,
90+
* otherwise this method would not have been reached.
91+
*/
92+
$tokens = $this->phpcsFile->getTokens();
93+
$open_parens = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true );
94+
$has_ellipses = $this->phpcsFile->findNext( T_ELLIPSIS, ( $open_parens + 1 ), $tokens[ $open_parens ]['parenthesis_closer'] );
95+
96+
if ( $has_ellipses !== false ) {
97+
$target_nesting_level = 1;
98+
if ( isset( $tokens[ $open_parens ]['nested_parenthesis'] ) ) {
99+
$target_nesting_level = ( count( $tokens[ $open_parens ]['nested_parenthesis'] ) + 1 );
100+
}
101+
102+
if ( $target_nesting_level === count( $tokens[ $has_ellipses ]['nested_parenthesis'] ) ) {
103+
// Bow out as undetermined.
104+
return;
105+
}
80106
}
81107

82-
if ( isset( $parameters[2], $this->restricted_filters[ $parameters[2]['raw'] ] ) ) {
83-
$message = 'Please use an appropriate filter to sanitize, as "%s" does no filtering, see http://php.net/manual/en/filter.filters.sanitize.php.';
84-
$data = [ strtoupper( $parameters[2]['raw'] ) ];
85-
$this->phpcsFile->addWarning( $message, $stackPtr, 'RestrictedFilter', $data );
108+
$message = 'Missing $%s parameter for "%s()".';
109+
$data = [ $param_name, $matched_content ];
110+
111+
// Error codes should probably be made more descriptive, but that would be a BC-break.
112+
$error_code = 'MissingSecondParameter';
113+
if ( $matched_content === 'filter_input' ) {
114+
$error_code = 'MissingThirdParameter';
86115
}
116+
117+
$this->phpcsFile->addWarning( $message, $stackPtr, $error_code, $data );
118+
return;
119+
}
120+
121+
if ( isset( $this->restricted_filters[ $target_param['clean'] ] ) ) {
122+
$first_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $target_param['start'], ( $target_param['end'] + 1 ), true );
123+
124+
$message = 'Please use an appropriate filter to sanitize, as "%s" does no filtering, see: http://php.net/manual/en/filter.filters.sanitize.php.';
125+
$data = [ $target_param['clean'] ];
126+
$this->phpcsFile->addWarning( $message, $first_non_empty, 'RestrictedFilter', $data );
87127
}
88128
}
89129
}

‎WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.inc

Lines changed: 92 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,100 @@
11
<?php
22

3-
$url = 'http://www.google.ca';
4-
$_GET['foo'] = 'bar';
5-
$array = [ 'something_something', 'https://www.google.com', '6' ];
3+
/*
4+
* Not the sniff target.
5+
*/
6+
use filter_var;
67

7-
// Ok.
8+
my\ns\filter_input($a, $b);
9+
$this->filter_var_array($a, $b);
10+
$this?->filter_input_array($a, $b);
11+
MyClass::filter_var($a, $b);
12+
echo FILTER_INPUT;
13+
namespace\filter_var_array($a, $b);
14+
15+
// Looks like a function call, but is a PHP 8.0+ class instantiation via an attribute.
16+
#[Filter_Input('text')]
17+
function foo() {}
18+
19+
// PHP 8.1 first class callable.
20+
// As we have no knowledge about what parameters will be passed, we shouldn't flag this.
21+
add_filter('my_filter', filter_var(...));
22+
23+
24+
/*
25+
* These should all be okay.
26+
*/
827
filter_var( $url, FILTER_SANITIZE_URL );
9-
filter_var( 'test', FILTER_SANITIZE_STRING );
10-
filter_var( "test", FILTER_SANITIZE_STRING );
11-
filter_input( INPUT_GET, 'foo', FILTER_SANITIZE_STRING );
28+
\filter_var( 'test', FILTER_SANITIZE_STRING );
29+
FILTER_INPUT( INPUT_GET, 'foo', FILTER_SANITIZE_STRING, );
1230
filter_input( INPUT_GET, "foo" , FILTER_SANITIZE_STRING );
13-
filter_var_array( $array, FILTER_SANITIZE_STRING );
14-
filter_input_array( $array, FILTER_SANITIZE_STRING );
15-
filter_input_array( $array,FILTER_SANITIZE_STRING );
16-
17-
// Bad.
18-
filter_input( INPUT_GET, 'foo' ); // Missing third parameter.
19-
filter_input( INPUT_GET, 'foo', FILTER_DEFAULT ); // This filter ID does nothing.
20-
filter_input( INPUT_GET, "foo", FILTER_UNSAFE_RAW ); // This filter ID does nothing.
21-
filter_var( $url ); // Missing second parameter.
22-
filter_var( $url, FILTER_DEFAULT ); // This filter ID does nothing.
31+
filter_input_array( $array, filter_default ); // Constants are case-sensitive, so this is not the FILTER_DEFAULT constant.
32+
33+
// Ignore as undetermined.
34+
filter_var( "test", get_filter() );
35+
\Filter_Var_Array( $array, $filterName );
36+
filter_input_array( $array,$obj->get_filter() , );
37+
38+
// Incomplete function call, should be ignored by the sniff.
39+
$incorrect_but_ok = filter_input();
40+
41+
/*
42+
* These should all be flagged with a warning.
43+
*/
44+
filter_input( INPUT_GET, 'foo' ); // Missing $filter parameter.
45+
\filter_input( INPUT_GET, 'foo', FILTER_DEFAULT ); // This filter ID does nothing.
46+
filter_input( INPUT_GET, "foo", FILTER_UNSAFE_RAW /* comment */ ,); // This filter ID does nothing.
47+
48+
filter_var( $url ); // Missing $filter parameter.
49+
Filter_Var( $url, FILTER_DEFAULT ); // This filter ID does nothing.
2350
filter_var( 'https://google.com', FILTER_UNSAFE_RAW ); // This filter ID does nothing.
24-
filter_var_array( $array ); // Missing second parameter.
51+
52+
filter_var_array( $array, ); // Missing $options parameter.
2553
filter_var_array( $array, FILTER_DEFAULT ); // This filter ID does nothing.
2654
filter_var_array( $array, FILTER_UNSAFE_RAW ); // This filter ID does nothing.
27-
filter_input_array( $array ); // Missing second parameter.
28-
filter_input_array( $array, FILTER_DEFAULT ); // This filter ID does nothing.
29-
filter_input_array( $array, FILTER_UNSAFE_RAW ); // This filter ID does nothing.
55+
56+
filter_input_array( $array ); // Missing $options parameter.
57+
\FILTER_INPUT_ARRAY( $array, FILTER_DEFAULT ); // This filter ID does nothing.
58+
filter_input_array( $array, FILTER_UNSAFE_RAW, ); // This filter ID does nothing.
59+
60+
// Safeguard handling of function calls using PHP 8.0+ named parameters.
61+
filter_input(var_name: $var_name, filter: FILTER_SANITIZE_STRING, type: FILTER_DEFAULT); // OK, invalid input value for $type, but that's not our concern.
62+
filter_input(var_name: $var_name, filter: $filter, type: $type); // Ignore, undetermined.
63+
filter_input(
64+
var_name: $var_name,
65+
filter: FILTER_DEFAULT, // This filter ID does nothing.
66+
type: $type,
67+
);
68+
69+
filter_var(filter: FILTER_SANITIZE_URL); // OK, well not really, missing required parameter, but that's not our concern.
70+
filter_var($value, options: FILTER_NULL_ON_FAILURE); // Missing $filter parameter.
71+
filter_var(value: $value, filters: FILTER_SANITIZE_URL); // Typo in parameter name, report as missing $filter parameter.
72+
73+
filter_var_array(options: FILTER_UNSAFE_RAW, array: $array); // This filter ID does nothing.
74+
75+
filter_input_array($type, add_empty: false, options: FILTER_DEFAULT,); // This filter ID does nothing.
76+
77+
// Ignore function calls using PHP 5.6 argument unpacking as we don't know what parameters were passed.
78+
filter_input(INPUT_GET, ...$params);
79+
trim(filter_input(INPUT_GET, ...$params));
80+
// ... but only ignore unpacking if done at the correct nesting level.
81+
filter_input(INPUT_GET, $obj->getVarname(...$params)); // Missing $filter parameter.
82+
83+
// False negatives: $options arrays are currently not (yet) supported by this sniff.
84+
// See: https://www.php.net/manual/en/function.filter-var-array.php and https://www.php.net/manual/en/function.filter-input-array.php
85+
filter_var_array(
86+
$array,
87+
array('keyA' => FILTER_DEFAULT), // This filter ID does nothing.
88+
);
89+
filter_input_array(
90+
$array,
91+
[
92+
'keyA' => [
93+
'filter' => FILTER_UNSAFE_RAW, // This filter ID does nothing.
94+
'flags' => FILTER_FORCE_ARRAY,
95+
],
96+
'keyB' => [
97+
'filter' => FILTER_SANITIZE_ENCODED,
98+
],
99+
]
100+
);

‎WordPressVIPMinimum/Tests/Security/PHPFilterFunctionsUnitTest.php

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -32,18 +32,24 @@ public function getErrorList() {
3232
*/
3333
public function getWarningList() {
3434
return [
35-
18 => 1,
36-
19 => 1,
37-
20 => 1,
38-
21 => 1,
39-
22 => 1,
40-
23 => 1,
41-
24 => 1,
42-
25 => 1,
43-
26 => 1,
44-
27 => 1,
45-
28 => 1,
46-
29 => 1,
35+
44 => 1,
36+
45 => 1,
37+
46 => 1,
38+
48 => 1,
39+
49 => 1,
40+
50 => 1,
41+
52 => 1,
42+
53 => 1,
43+
54 => 1,
44+
56 => 1,
45+
57 => 1,
46+
58 => 1,
47+
65 => 1,
48+
70 => 1,
49+
71 => 1,
50+
73 => 1,
51+
75 => 1,
52+
81 => 1,
4753
];
4854
}
4955
}

0 commit comments

Comments
 (0)