Merge pull request #1678 from WordPress-Coding-Standards/feature/security-sniffs-allow-for-type-testing

Security: allow for type testing superglobals

Author: GaryJones

WordPress/Sniff.php

@@ -312,11 +312,42 @@ abstract class Sniff implements PHPCS_Sniff {
 		'doubleval'    => true,
 		'floatval'     => true,
 		'intval'       => true,
-		'is_array'     => true,
 		'sanitize_key' => true,
 		'sizeof'       => true,
 	);
 
+	/**
+	 * List of PHP native functions to test the type of a variable.
+	 *
+	 * Using these functions is safe in combination with superglobals without
+	 * unslashing or sanitization.
+	 *
+	 * They should, however, not be regarded as unslashing or sanitization functions.
+	 *
+	 * @since 2.1.0
+	 *
+	 * @var array
+	 */
+	protected $typeTestFunctions = array(
+		'is_array'     => true,
+		'is_bool'      => true,
+		'is_callable'  => true,
+		'is_countable' => true,
+		'is_double'    => true,
+		'is_float'     => true,
+		'is_int'       => true,
+		'is_integer'   => true,
+		'is_iterable'  => true,
+		'is_long'      => true,
+		'is_null'      => true,
+		'is_numeric'   => true,
+		'is_object'    => true,
+		'is_real'      => true,
+		'is_resource'  => true,
+		'is_scalar'    => true,
+		'is_string'    => true,
+	);
+
 	/**
 	 * Token which when they preceed code indicate the value is safely casted.
 	 *
@@ -1372,12 +1403,17 @@ protected function has_nonce_check( $stackPtr ) {
 			}
 		}
 
-		$in_isset = $this->is_in_isset_or_empty( $stackPtr );
+		$allow_nonce_after = false;
+		if ( $this->is_in_isset_or_empty( $stackPtr )
+			|| $this->is_in_type_test( $stackPtr )
+		) {
+			$allow_nonce_after = true;
+		}
 
-		// We allow for isset( $_POST['var'] ) checks to come before the nonce check.
-		// If this is inside an isset(), check after it as well, all the way to the
-		// end of the scope.
-		if ( $in_isset ) {
+		// We allow for certain actions, such as an isset() check to come before the nonce check.
+		// If this superglobal is inside such a check, look for the nonce after it as well,
+		// all the way to the end of the scope.
+		if ( true === $allow_nonce_after ) {
 			$end = ( 0 === $start ) ? $this->phpcsFile->numTokens : $tokens[ $start ]['scope_closer'];
 		}
 
@@ -1393,7 +1429,7 @@ protected function has_nonce_check( $stackPtr ) {
 				// If we have already found an nonce check in this scope, we just
 				// need to check whether it comes before this token. It is OK if the
 				// check is after the token though, if this was only a isset() check.
-				return ( $in_isset || $last['nonce_check'] < $stackPtr );
+				return ( true === $allow_nonce_after || $last['nonce_check'] < $stackPtr );
 			} elseif ( $end <= $last['end'] ) {
 				// If not, we can still go ahead and return false if we've already
 				// checked to the end of the search area.
@@ -1624,6 +1660,24 @@ protected function is_in_function_call( $stackPtr, $valid_functions, $global = t
 		return false;
 	}
 
+	/**
+	 * Check if a token is inside of an is_...() statement.
+	 *
+	 * @since 2.1.0
+	 *
+	 * @param int $stackPtr The index of the token in the stack.
+	 *
+	 * @return bool Whether the token is being type tested.
+	 */
+	protected function is_in_type_test( $stackPtr ) {
+		/*
+		 * Casting the potential integer stack pointer return value to boolean here is fine.
+		 * The return can never be `0` as there will always be a PHP open tag before the
+		 * function call.
+		 */
+		return (bool) $this->is_in_function_call( $stackPtr, $this->typeTestFunctions );
+	}
+
 	/**
 	 * Check if something is only being sanitized.
 	 *

WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php

@@ -145,6 +145,11 @@ function ( $symbol ) {
 			return;
 		}
 
+		// If this variable is being tested with one of the `is_..()` functions, sanitization isn't needed.
+		if ( $this->is_in_type_test( $stackPtr ) ) {
+			return;
+		}
+
 		// If this is a comparison ('a' == $_POST['foo']), sanitization isn't needed.
 		if ( $this->is_comparison( $stackPtr ) ) {
 			return;

WordPress/Tests/Security/NonceVerificationUnitTest.inc

@@ -17,7 +17,7 @@ function ajax_process() {
 }
 add_action( 'wp_ajax_process', 'ajax_process' );
 
-// It's also OK to check with isset() before the the nonce check.
+// It's also OK to check with isset() before the nonce check.
 function foo() {
 	if ( ! isset( $_POST['test'] ) || ! wp_verify_nonce( 'some_action' ) ) {
 		exit;
@@ -160,3 +160,21 @@ function foo_8() {
 	sanitize_pc( $_POST['bar'] ); // Bad.
 	my_nonce_check( sanitize_twitter( $_POST['tweet'] ) ); // Bad.
 }
+
+/*
+ * Using a superglobal in a is_...() function is OK as long as a nonce check is done
+ * before the variable is *really* used.
+ */
+function test_whitelisting_use_in_type_test_functions() {
+	if ( ! is_numeric ( $_POST['foo'] ) ) { // OK.
+		return;
+	}
+
+	wp_verify_nonce( 'some_action' );
+}
+
+function test_incorrect_use_in_type_test_functions() {
+	if ( ! is_numeric ( $_POST['foo'] ) ) { // Bad.
+		return;
+	}
+}

WordPress/Tests/Security/NonceVerificationUnitTest.php

@@ -46,6 +46,7 @@ public function getErrorList() {
 			159 => 1,
 			160 => 1,
 			161 => 1,
+			177 => 1,
 		);
 	}
 

WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc

@@ -257,3 +257,13 @@ function test_invalid_array_key_exists_call() {
 		$id = (int) $_POST['bar']; // Bad x 1 - validate.
 	}
 }
+
+function test_ignoring_is_type_function_calls() {
+	// Usage within variable type tests does not need unslashing or sanitization.
+	if ( isset( $_POST[ $key ] ) && is_numeric( $_POST[ $key ] ) ) {} // OK.
+	if ( isset($_POST['plugin']) && is_string( $_POST['plugin'] ) ) {} // OK.
+
+	if ( ! is_null( $_GET['not_set'] ) ) {} // Bad x1 - missing validation.
+	if ( array_key_exists( 'null', $_GET ) && ! is_null( $_GET['null'] ) ) {} // OK.
+	if ( array_key_exists( 'null', $_POST ) && $_POST['null'] !== null ) {} // OK.
+}

WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php

@@ -65,6 +65,7 @@ public function getErrorList() {
 			245 => 1,
 			251 => 1,
 			257 => 1,
+			266 => 1,
 		);
 	}