Changeset 42011

Timestamp:

10/24/2017 11:14:33 PM (7 years ago)

Author:

johnbillion

Message:

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

Location:

trunk

Files:

• src/wp-admin/includes/file.php (modified) (1 diff)
• src/wp-includes/functions.php (modified) (1 diff)
• tests/phpunit/tests/functions.php (modified) (1 diff)

trunk/src/wp-admin/includes/file.php

@@ -664 +664 @@
  * @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 );
 

trunk/src/wp-includes/functions.php

@@ -4253 +4253 @@
  * @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;

trunk/tests/phpunit/tests/functions.php

@@ -1162 +1162 @@
     }
 
+    /**
+     * 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,
+            ),
+
+        );
+    }
 }