From 3e9a42ed277fd865b7b64093035238d7b4592925 Mon Sep 17 00:00:00 2001
From: John Blackbourn <johnbillion@git.wordpress.org>
Date: Tue, 24 Oct 2017 23:14:33 +0000
Subject: [PATCH] Filesystem API: Add more specificity to the rules for valid
 files in `validate_file()`.

This now treats files containing `./` as valid, and also treats files containing a trailing `../` as valid due to widespread use of this pattern in theme and plugin zip files.

Adds tests.

Props Ipstenu, borgesbruno, DavidAnderson, philipjohn, birgire
Fixes #42016, #36170


git-svn-id: https://develop.svn.wordpress.org/trunk@42011 602fd350-edb4-49c9-b593-d223f7449a82
---
 src/wp-admin/includes/file.php    |   2 +-
 src/wp-includes/functions.php     |  17 +++-
 tests/phpunit/tests/functions.php | 143 ++++++++++++++++++++++++++++++
 3 files changed, 158 insertions(+), 4 deletions(-)

diff --git a/src/wp-admin/includes/file.php b/src/wp-admin/includes/file.php
index 416433e5a5..a834e5e161 100644
--- a/src/wp-admin/includes/file.php
+++ b/src/wp-admin/includes/file.php
@@ -663,7 +663,7 @@ function wp_tempnam( $filename = '', $dir = '' ) {
  * @param array  $allowed_files Optional. Array of allowed files to edit, $file must match an entry exactly.
  * @return string|null
  */
-function validate_file_to_edit( $file, $allowed_files = '' ) {
+function validate_file_to_edit( $file, $allowed_files = array() ) {
 	$code = validate_file( $file, $allowed_files );
 
 	if (!$code )
diff --git a/src/wp-includes/functions.php b/src/wp-includes/functions.php
index a9c53d31e4..b5d21285b6 100644
--- a/src/wp-includes/functions.php
+++ b/src/wp-includes/functions.php
@@ -4252,16 +4252,27 @@ function iis7_supports_permalinks() {
  * @param array  $allowed_files Optional. List of allowed files.
  * @return int 0 means nothing is wrong, greater than 0 means something was wrong.
  */
-function validate_file( $file, $allowed_files = '' ) {
-	if ( false !== strpos( $file, '..' ) )
+function validate_file( $file, $allowed_files = array() ) {
+	// `../` on its own is not allowed:
+	if ( '../' === $file ) {
 		return 1;
+	}
 
-	if ( false !== strpos( $file, './' ) )
+	// More than one occurence of `../` is not allowed:
+	if ( preg_match_all( '#\.\./#', $file, $matches, PREG_SET_ORDER ) && ( count( $matches ) > 1 ) ) {
 		return 1;
+	}
 
+	// `../` which does not occur at the end of the path is not allowed:
+	if ( false !== strpos( $file, '../' ) && '../' !== mb_substr( $file, -3, 3 ) ) {
+		return 1;
+	}
+
+	// Files not in the allowed file list are not allowed:
 	if ( ! empty( $allowed_files ) && ! in_array( $file, $allowed_files ) )
 		return 3;
 
+	// Absolute Windows drive paths are not allowed:
 	if (':' == substr( $file, 1, 1 ) )
 		return 2;
 
diff --git a/tests/phpunit/tests/functions.php b/tests/phpunit/tests/functions.php
index 7c8736ab67..6a2477ef0b 100644
--- a/tests/phpunit/tests/functions.php
+++ b/tests/phpunit/tests/functions.php
@@ -1161,4 +1161,147 @@ class Tests_Functions extends WP_UnitTestCase {
 		return $data;
 	}
 
+	/**
+	 * Test file path validation
+	 *
+	 * @ticket 42016
+	 * @dataProvider data_test_validate_file()
+	 *
+	 * @param string $file          File path.
+	 * @param array  $allowed_files List of allowed files.
+	 * @param int    $expected      Expected result.
+	 */
+	public function test_validate_file( $file, $allowed_files, $expected ) {
+		$this->assertSame( $expected, validate_file( $file, $allowed_files ) );
+	}
+
+	/**
+	 * Data provider for file validation.
+	 *
+	 * @return array {
+	 *     @type array $0... {
+	 *         @type string $0 File path.
+	 *         @type array  $1 List of allowed files.
+	 *         @type int    $2 Expected result.
+	 *     }
+	 * }
+	 */
+	public function data_test_validate_file() {
+		return array(
+
+			// Allowed files:
+			array(
+				null,
+				array(),
+				0,
+			),
+			array(
+				'',
+				array(),
+				0,
+			),
+			array(
+				' ',
+				array(),
+				0,
+			),
+			array(
+				'.',
+				array(),
+				0,
+			),
+			array(
+				'..',
+				array(),
+				0,
+			),
+			array(
+				'./',
+				array(),
+				0,
+			),
+			array(
+				'foo.ext',
+				array( 'foo.ext' ),
+				0,
+			),
+			array(
+				'dir/foo.ext',
+				array(),
+				0,
+			),
+			array(
+				'foo..ext',
+				array(),
+				0,
+			),
+			array(
+				'dir/dir/../',
+				array(),
+				0,
+			),
+
+			// Directory traversal:
+			array(
+				'../',
+				array(),
+				1,
+			),
+			array(
+				'../../',
+				array(),
+				1,
+			),
+			array(
+				'../file.ext',
+				array(),
+				1,
+			),
+			array(
+				'../dir/../',
+				array(),
+				1,
+			),
+			array(
+				'/dir/dir/../../',
+				array(),
+				1,
+			),
+			array(
+				'/dir/dir/../../',
+				array( '/dir/dir/../../' ),
+				1,
+			),
+
+			// Windows drives:
+			array(
+				'c:',
+				array(),
+				2,
+			),
+			array(
+				'C:/WINDOWS/system32',
+				array( 'C:/WINDOWS/system32' ),
+				2,
+			),
+
+			// Disallowed files:
+			array(
+				'foo.ext',
+				array( 'bar.ext' ),
+				3,
+			),
+			array(
+				'foo.ext',
+				array( '.ext' ),
+				3,
+			),
+			array(
+				'path/foo.ext',
+				array( 'foo.ext' ),
+				3,
+			),
+
+		);
+	}
 }