From e9a212d35c3c0d37267afe0b7331d2a273ad973e Mon Sep 17 00:00:00 2001 From: JDGrimes Date: Sat, 30 May 2015 11:28:24 -0400 Subject: [PATCH] Require unlashing of input superglobals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This updates the validated/sanitized input sniff to also check for slashing. This could have been made into another sniff instead, however, it would have required lots of duplicated logic and this sniff would need to be updated to accommodate the use of `wp_unslash()` anyway. Currently only `wp_unslash()` is recognized as an unlashing function, but this can be changed in the future if needed. The sniff currently requires that `wp_unslash()` be used *before* the data is passed through a sanitizing function. Sanitizing first and then wrapping that in `wp_unslash()` is not accepted. The error for missing the use of `wp_unslash()` is independent of the missing sanitizing function error, so an error will be given for missing use of an unslashing function whether or not a sanitizing function is used, and vice versa. Unslashing is not required when sanitization is provided via casting, or when certain sanitization functions are used which implicitly or explicitly perform an unslash or for which unslashing isn’t necessary. `absint()` implicitly unslashes, and `sanitize_key()` will remove slashes explicitly. And unslashing isn’t necessary when testing a value with `is_array()`. This list can be expanded in the future, and is configurable via the `customUnslashingSanitizingFunctions` property. See #172 --- WordPress/Sniff.php | 71 +++++++++++++++++-- .../VIP/ValidatedSanitizedInputSniff.php | 16 ++++- .../VIP/ValidatedSanitizedInputUnitTest.inc | 55 ++++++++------ .../VIP/ValidatedSanitizedInputUnitTest.php | 9 ++- 4 files changed, 120 insertions(+), 31 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 7a5fdb93d5..1b2fc7cbde 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -271,6 +271,19 @@ abstract class WordPress_Sniff implements PHP_CodeSniffer_Sniff { 'wp_safe_redirect' => true, ); + /** + * Sanitizing functions that implicitly unslash the data passed to them. + * + * @since 0.5.0 + * + * @var array + */ + public static $unslashingSanitizingFunctions = array( + 'absint' => true, + 'is_array' => true, + 'sanitize_key' => true, + ); + /** * Functions that format strings. * @@ -291,7 +304,6 @@ abstract class WordPress_Sniff implements PHP_CodeSniffer_Sniff { 'wp_sprintf' => true, ); - /** * Functions which print output incorporating the values passed to them. * @@ -675,11 +687,13 @@ protected function is_safe_casted( $stackPtr ) { * * @since 0.5.0 * - * @param int $stackPtr The index of the token in the stack. + * @param int $stackPtr The index of the token in the stack. + * @param bool $require_unslash Whether to give an error if wp_unslash() isn't + * used on the variable before sanitization. * * @return bool Whether the token being sanitized. */ - protected function is_sanitized( $stackPtr ) { + protected function is_sanitized( $stackPtr, $require_unslash = false ) { // First we check if it is being casted to a safe value. if ( $this->is_safe_casted( $stackPtr ) ) { @@ -688,14 +702,16 @@ protected function is_sanitized( $stackPtr ) { // If this isn't within a function call, we know already that it's not safe. if ( ! isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) { + if ( $require_unslash ) { + $this->add_unslash_error( $stackPtr ); + } return false; } // Get the function that it's in. $function_closer = end( $this->tokens[ $stackPtr ]['nested_parenthesis'] ); $function_opener = key( $this->tokens[ $stackPtr ]['nested_parenthesis'] ); - $functionPtr = $function_opener - 1; - $function = $this->tokens[ $functionPtr ]; + $function = $this->tokens[ $function_opener - 1 ]; // If it is just being unset, the value isn't used at all, so it's safe. if ( T_UNSET === $function['code'] ) { @@ -704,11 +720,34 @@ protected function is_sanitized( $stackPtr ) { // If this isn't a call to a function, it sure isn't sanitizing function. if ( T_STRING !== $function['code'] ) { + if ( $require_unslash ) { + $this->add_unslash_error( $stackPtr ); + } return false; } $functionName = $function['content']; + // Check if wp_unslash() is being used. + if ( 'wp_unslash' === $functionName ) { + + $is_unslashed = true; + + $function_closer = prev( $this->tokens[ $stackPtr ]['nested_parenthesis'] ); + + // If there is no other function being used, this value is unsanitized. + if ( ! $function_closer ) { + return false; + } + + $function_opener = key( $this->tokens[ $stackPtr ]['nested_parenthesis'] ); + $functionName = $this->tokens[ $function_opener - 1 ]['content']; + + } else { + + $is_unslashed = false; + } + // Arrays might be sanitized via array_map(). if ( 'array_map' === $functionName ) { @@ -726,10 +765,32 @@ protected function is_sanitized( $stackPtr ) { } } + // If slashing is required, give an error. + if ( ! $is_unslashed && $require_unslash && ! isset( self::$unslashingSanitizingFunctions[ $functionName ] ) ) { + $this->add_unslash_error( $stackPtr ); + } + // Check if this is a sanitizing function. return isset( self::$sanitizingFunctions[ $functionName ] ); } + /** + * Add an error for missing use of wp_unslash(). + * + * @since 0.5.0 + * + * @param int $stackPtr The index of the token in the stack. + */ + public function add_unslash_error( $stackPtr ) { + + $this->phpcsFile->addError( + 'Missing wp_unslash() before sanitization.', + $stackPtr, + 'MissingUnslash', + array( $this->tokens[ $stackPtr ]['content'] ) + ); + } + /** * Get the index key of an array variable. * diff --git a/WordPress/Sniffs/VIP/ValidatedSanitizedInputSniff.php b/WordPress/Sniffs/VIP/ValidatedSanitizedInputSniff.php index 516ce2ea9b..a672a1fad7 100644 --- a/WordPress/Sniffs/VIP/ValidatedSanitizedInputSniff.php +++ b/WordPress/Sniffs/VIP/ValidatedSanitizedInputSniff.php @@ -27,6 +27,15 @@ class WordPress_Sniffs_VIP_ValidatedSanitizedInputSniff extends WordPress_Sniff */ public $customSanitizingFunctions = array(); + /** + * Custom sanitizing functions that implicitly unslash the values passed to them. + * + * @since 0.5.0 + * + * @var string[] + */ + public $customUnslashingSanitizingFunctions = array(); + /** * Whether the custom list of functions has been added to the defaults yet. * @@ -67,6 +76,11 @@ public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) { array_flip( $this->customSanitizingFunctions ) ); + WordPress_Sniff::$unslashingSanitizingFunctions = array_merge( + WordPress_Sniff::$unslashingSanitizingFunctions, + array_flip( $this->customUnslashingSanitizingFunctions ) + ); + self::$addedCustomFunctions = true; } @@ -123,7 +137,7 @@ public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) { } // Now look for sanitizing functions - if ( ! $this->is_sanitized( $stackPtr ) ) { + if ( ! $this->is_sanitized( $stackPtr, true ) ) { $phpcsFile->addError( 'Detected usage of a non-sanitized input variable: %s', $stackPtr, 'InputNotSanitized', array( $tokens[ $stackPtr ]['content'] ) ); } diff --git a/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.inc b/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.inc index ca1cf0a7b8..46a1f18269 100644 --- a/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.inc +++ b/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.inc @@ -2,12 +2,12 @@ do_something( $_POST ); // OK -do_something_with( $_POST['hello'] ); // Error for no validation, Error for no sanitizing +do_something_with( $_POST['hello'] ); // Error for no validation, Error for no sanitizing, Error for no unslashing. -echo sanitize_text_field( $_POST['foo1'] ); // Error for no validation +echo sanitize_text_field( wp_unslash( $_POST['foo1'] ) ); // Error for no validation if ( isset( $_POST['foo2'] ) ) { - bar( $_POST['foo2'] ); // Error for no sanitizing + bar( wp_unslash( $_POST['foo2'] ) ); // Error for no sanitizing } // @TODO: Cover non-parenthesis'd conditions @@ -16,9 +16,9 @@ if ( isset( $_POST['foo2'] ) ) { if ( isset( $_POST['foo3'] ) ) { - bar( sanitize_text_field( $_POST['foo3'] ) ); // Good, validated and sanitized - bar( $_POST['foo3'] ); // Error, validated but not sanitized - bar( sanitize_text_field( $_POST['foo3'] ) ); // Good, validated and sanitized + bar( sanitize_text_field( wp_unslash( $_POST['foo3'] ) ) ); // Good, validated and sanitized + bar( wp_unslash( $_POST['foo3'] ) ); // Error, validated but not sanitized + bar( sanitize_text_field( wp_unslash( $_POST['foo3'] ) ) ); // Good, validated and sanitized } // Should all be OK @@ -30,17 +30,17 @@ $empty = ( empty( $_POST['foo4'] ) ); -$foo = $_POST['bar']; // Bad x 2 +$foo = $_POST['bar']; // Bad x 3 function foo() { // Ok, if WordPress_Sniffs_VIP_ValidatedSanitizedInputSniff::$check_validation_in_scope_only == false - if ( ! isset( $_REQUEST['bar1'] ) || ! foo( sanitize_text_field( $_REQUEST['bar1'] ) ) ) { + if ( ! isset( $_REQUEST['bar1'] ) || ! foo( sanitize_text_field( wp_unslash( $_REQUEST['bar1'] ) ) ) ) { wp_die( 1 ); } } // Ok, if WordPress_Sniffs_VIP_ValidatedSanitizedInputSniff::$check_validation_in_scope_only == false -if ( ! isset( $_REQUEST['bar2'] ) || ! foo( sanitize_text_field( $_REQUEST['bar2'] ) ) ) { // Ok +if ( ! isset( $_REQUEST['bar2'] ) || ! foo( sanitize_text_field( wp_unslash( $_REQUEST['bar2'] ) ) ) ) { // Ok wp_die( 1 ); } @@ -48,7 +48,7 @@ function bar() { if ( ! isset( $_GET['test'] ) ) { return ; } - echo sanitize_text_field( $_GET['test'] ); // Good + echo sanitize_text_field( wp_unslash( $_GET['test'] ) ); // Good } $_REQUEST['wp_customize'] = 'on'; // ok @@ -69,17 +69,17 @@ some_func( $some_arg, (int) $_GET['test'] ); // ok function zebra() { if ( isset( $_GET['foo'], $_POST['bar'] ) ) { - echo sanitize_text_field( $_POST['bar'] ); // ok + echo sanitize_text_field( wp_unslash( $_POST['bar'] ) ); // ok } } echo $_GET['test']; // WPCS: sanitization OK. -echo array_map( 'sanitize_text_field', $_GET['test'] ); // OK -echo array_map( 'foo', $_GET['test'] ); // Bad -echo array_map( $something, $_GET['test'] ); // Bad -echo array_map( array( $obj, 'func' ), $_GET['test'] ); // Bad -echo array_map( array( $obj, 'sanitize_text_field' ), $_GET['test'] ); // Bad +echo array_map( 'sanitize_text_field', wp_unslash( $_GET['test'] ) ); // OK +echo array_map( 'foo', wp_unslash( $_GET['test'] ) ); // Bad +echo array_map( $something, wp_unslash( $_GET['test'] ) ); // Bad +echo array_map( array( $obj, 'func' ), wp_unslash( $_GET['test'] ) ); // Bad +echo array_map( array( $obj, 'sanitize_text_field' ), wp_unslash( $_GET['test'] ) ); // Bad // Sanitized but not validated. $foo = (int) $_POST['foo6']; // Bad @@ -87,14 +87,25 @@ $foo = (int) $_POST['foo6']; // Bad // Non-assignment checks are OK. if ( 'bar' === $_POST['foo'] ) {} // OK if ( $_GET['test'] != 'a' ) {} // OK -if ( 'bar' === do_something( $_POST['foo'] ) ) {} // Bad +if ( 'bar' === do_something( wp_unslash( $_POST['foo'] ) ) ) {} // Bad switch ( $_POST['foo'] ) {} // OK -switch ( do_something( $_POST['foo'] ) ) {} // Bad +switch ( do_something( wp_unslash( $_POST['foo'] ) ) ) {} // Bad // Sanitization is required even when the value is being escaped. -echo esc_html( $_POST['foo'] ); // Bad -echo esc_html( sanitize_text_field( $_POST['foo'] ) ); // OK +echo esc_html( wp_unslash( $_POST['foo'] ) ); // Bad +echo esc_html( sanitize_text_field( wp_unslash( $_POST['foo'] ) ) ); // OK -$current_tax_slug = isset( $_GET['a'] ) ? sanitize_key( $_GET['a'] ) : false; -$current_tax_slug = isset( $_GET['a'] ) ? $_GET['a'] : false; +$current_tax_slug = isset( $_GET['a'] ) ? sanitize_key( $_GET['a'] ) : false; // OK +$current_tax_slug = isset( $_GET['a'] ) ? $_GET['a'] : false; // Bad x 2 +$current_tax_slug = isset( $_GET['a'] ) ? wp_unslash( $_GET['a'] ) : false; // Bad +$current_tax_slug = isset( $_GET['a'] ) ? sanitize_text_field( wp_unslash( $_GET['a'] ) ) : false; // OK + +echo sanitize_text_field( $_POST['foo545'] ); // Error for no validation, unslashing +echo array_map( 'sanitize_text_field', $_GET['test'] ); // Bad, no unslashing +echo array_map( 'sanitize_key', $_GET['test'] ); // OK + +foo( absint( $_GET['foo'] ) ); // OK +$ids = array_map( 'absint', $_GET['test'] ); // OK + +if ( is_array( $_GET['test'] ) ) {} // OK diff --git a/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.php b/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.php index c8a0190d71..d050291153 100644 --- a/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.php +++ b/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.php @@ -25,11 +25,11 @@ class WordPress_Tests_VIP_ValidatedSanitizedInputUnitTest extends AbstractSniffU public function getErrorList() { return array( - 5 => 2, + 5 => 3, 7 => 1, 10 => 1, 20 => 1, - 33 => 2, + 33 => 3, 65 => 1, 79 => 1, 80 => 1, @@ -39,7 +39,10 @@ public function getErrorList() 90 => 1, 93 => 1, 96 => 1, - 100 => 1, + 100 => 2, + 101 => 1, + 104 => 2, + 105 => 1, ); }//end getErrorList()