Changeset 53575

Timestamp:

06/24/2022 08:33:56 PM (2 years ago)

Author:

davidbaumwald

Message:

Database: Add %i placeholder support to $wpdb->prepare to escape table and column names.

WordPress does not currently provide an explicit method for escaping SQL table and column names. This leads to potential security vulnerabilities, and makes reviewing code for security unnecessarily difficult. Also, static analysis tools also flag the queries as having unescaped SQL input.

Tables and column names in queries are usually in-the-raw, since using the existing %s will straight quote the value, making the query invalid.

This change introduces a new %i placeholder in $wpdb->prepare to properly quote table and column names using backticks.

Props tellyworth, iandunn, craigfrancis, peterwilsoncc, johnbillion, apokalyptik.
Fixes #52506.

Location:

trunk

Files:

• src/wp-includes/wp-db.php (modified) (11 diffs)
• tests/phpunit/tests/db.php (modified) (2 diffs)

trunk/src/wp-includes/wp-db.php

@@ -646 +646 @@
 
     /**
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
      * Whether to use mysqli over mysql. Default false.
      *
@@ -1349 +1377 @@
 
     /**
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
      * Prepares a SQL query for safe execution.
      *
@@ -1356 +1415 @@
      * - %f (float)
      * - %s (string)
+
      *
      * All placeholders MUST be left unquoted in the query string. A corresponding argument
@@ -1381 +1441 @@
      *              by updating the function signature. The second parameter was changed
      *              from `$args` to `...$args`.
+
+
+
+
      *
      * @link https://www.php.net/sprintf Description of syntax.
@@ -1412 +1476 @@
         }
 
-        // If args were passed as an array (as in vsprintf), move them up.
-        $passed_as_array = false;
-        if ( isset( $args[0] ) && is_array( $args[0] ) && 1 === count( $args ) ) {
-            $passed_as_array = true;
-            $args            = $args[0];
-        }
-
-        foreach ( $args as $arg ) {
-            if ( ! is_scalar( $arg ) && ! is_null( $arg ) ) {
-                wp_load_translations_early();
-                _doing_it_wrong(
-                    'wpdb::prepare',
-                    sprintf(
-                        /* translators: %s: Value type. */
-                        __( 'Unsupported value type (%s).' ),
-                        gettype( $arg )
-                    ),
-                    '4.8.2'
-                );
-            }
-        }
-
         /*
          * Specify the formatting allowed in a placeholder. The following are allowed:
@@ -1454 +1496 @@
         $query = str_replace( "'%s'", '%s', $query ); // Strip any existing single quotes.
         $query = str_replace( '"%s"', '%s', $query ); // Strip any existing double quotes.
-        $query = preg_replace( '/(?<!%)%s/', "'%s'", $query ); // Quote the strings, avoiding escaped strings like %%s.
-
-        $query = preg_replace( "/(?<!%)(%($allowed_format)?f)/", '%\\2F', $query ); // Force floats to be locale-unaware.
-
-        $query = preg_replace( "/%(?:%|$|(?!($allowed_format)?[sdF]))/", '%%\\1', $query ); // Escape any unescaped percents.
-
-        // Count the number of valid placeholders in the query.
-        $placeholders = preg_match_all( "/(^|[^%]|(%%)+)%($allowed_format)?[sdF]/", $query, $matches );
+
+        $query = preg_replace( "/%(?:%|$|(?!($allowed_format)?[sdfFi]))/", '%%\\1', $query ); // Escape any unescaped percents (i.e. anything unrecognised).
+
+        // Extract placeholders from the query.
+        $split_query = preg_split( "/(^|[^%]|(?:%%)+)(%(?:$allowed_format)?[sdfFi])/", $query, -1, PREG_SPLIT_DELIM_CAPTURE );
+
+        $split_query_count = count( $split_query );
+        $placeholder_count = ( ( $split_query_count - 1 ) / 3 ); // Split always returns with 1 value before the first placeholder (even with $query = "%s"), then 3 additional values per placeholder.
+
+        // If args were passed as an array (as in vsprintf), move them up.
+        $passed_as_array = ( isset( $args[0] ) && is_array( $args[0] ) && 1 === count( $args ) );
+        if ( $passed_as_array ) {
+            $args = $args[0];
+        }
+
+        $new_query       = '';
+        $key             = 2; // keys 0 and 1 in $split_query contain values before the first placeholder.
+        $arg_id          = 0;
+        $arg_identifiers = array();
+        $arg_strings     = array();
+        while ( $key < $split_query_count ) {
+            $placeholder = $split_query[ $key ];
+
+            $format = substr( $placeholder, 1, -1 );
+            $type   = substr( $placeholder, -1 );
+
+            if ( 'f' === $type ) { // Force floats to be locale-unaware.
+                $type        = 'F';
+                $placeholder = '%' . $format . $type;
+            }
+
+            if ( 'i' === $type ) {
+                $placeholder = '`%' . $format . 's`';
+                $argnum_pos  = strpos( $format, '$' ); // Using a simple strpos() due to previous checking (e.g. $allowed_format).
+                if ( false !== $argnum_pos ) {
+                    $arg_identifiers[] = ( intval( substr( $format, 0, $argnum_pos ) ) - 1 ); // sprintf argnum starts at 1, $arg_id from 0.
+                } else {
+                    $arg_identifiers[] = $arg_id;
+                }
+            } elseif ( 'd' !== $type && 'F' !== $type ) { // i.e. ('s' === $type), where 'd' and 'F' keeps $placeholder unchanged, and we ensure string escaping is used as a safe default (e.g. even if 'x').
+                $argnum_pos = strpos( $format, '$' );
+                if ( false !== $argnum_pos ) {
+                    $arg_strings[] = ( intval( substr( $format, 0, $argnum_pos ) ) - 1 );
+                }
+                if ( true !== $this->allow_unsafe_unquoted_parameters || '' === $format ) { // Unquoted strings for backwards compatibility (dangerous).
+                    $placeholder = "'%" . $format . "s'";
+                }
+            }
+
+            $new_query .= $split_query[ $key - 2 ] . $split_query[ $key - 1 ] . $placeholder; // Glue (-2), any leading characters (-1), then the new $placeholder.
+
+            $key += 3;
+            $arg_id++;
+        }
+        $query = $new_query . $split_query[ $key - 2 ]; // Replace $query; and add remaining $query characters, or index 0 if there were no placeholders.
+
+        $dual_use = array_intersect( $arg_identifiers, $arg_strings );
+        if ( count( $dual_use ) ) {
+            wp_load_translations_early();
+            _doing_it_wrong(
+                'wpdb::prepare',
+                sprintf(
+                    /* translators: %s: A comma-separated list of arguments found to be a problem. */
+                    __( 'Arguments (%s) cannot be used for both String and Identifier escaping.' ),
+                    implode( ', ', $dual_use )
+                ),
+                '6.1.0'
+            );
+
+            return;
+        }
 
         $args_count = count( $args );
 
-        if ( $args_count !== $placeholders ) {
-            if ( 1 === $placeholders && $passed_as_array ) {
+        if ( $args_count !== $placeholder ) {
+            if ( 1 === $placeholder && $passed_as_array ) {
                 // If the passed query only expected one argument, but the wrong number of arguments were sent as an array, bail.
                 wp_load_translations_early();
@@ -1487 +1592 @@
                         /* translators: 1: Number of placeholders, 2: Number of arguments passed. */
                         __( 'The query does not contain the correct number of placeholders (%1$d) for the number of arguments passed (%2$d).' ),
-                        $placeholders,
+                        $placeholder,
                         $args_count
                     ),
@@ -1497 +1602 @@
                  * return an empty string to avoid a fatal error on PHP 8.
                  */
-                if ( $args_count < $placeholders ) {
-                    $max_numbered_placeholder = ! empty( $matches[3] ) ? max( array_map( 'intval', $matches[3] ) ) : 0;
-
+                if ( $args_count < $placeholder_count ) {
+                    $max_numbered_placeholder = 0;
+                    for ( $i = 2, $l = $split_query_count; $i < $l; $i += 3 ) {
+                        $argnum = intval( substr( $split_query[ $i ], 1 ) ); // Assume a leading number is for a numbered placeholder, e.g. '%3$s'.
+                        if ( $max_numbered_placeholder < $argnum ) {
+                            $max_numbered_placeholder = $argnum;
+                        }
+                    }
                     if ( ! $max_numbered_placeholder || $args_count < $max_numbered_placeholder ) {
                         return '';
@@ -1507 +1617 @@
         }
 
-        array_walk( $args, array( $this, 'escape_by_ref' ) );
-        $query = vsprintf( $query, $args );
+        $args_escaped = array();
+
+        foreach ( $args as $i => $value ) {
+            if ( in_array( $i, $arg_identifiers, true ) ) {
+                $args_escaped[] = $this->_escape_identifier_value( $value );
+            } elseif ( is_int( $value ) || is_float( $value ) ) {
+                $args_escaped[] = $value;
+            } else {
+                if ( ! is_scalar( $value ) && ! is_null( $value ) ) {
+                    wp_load_translations_early();
+                    _doing_it_wrong(
+                        'wpdb::prepare',
+                        sprintf(
+                            /* translators: %s: Value type. */
+                            __( 'Unsupported value type (%s).' ),
+                            gettype( $value )
+                        ),
+                        '4.8.2'
+                    );
+                    $value = ''; // Preserving old behaviour, where values are escaped as strings.
+                }
+                $args_escaped[] = $this->_real_escape( $value );
+            }
+        }
+
+        $query = vsprintf( $query, $args_escaped );
 
         return $this->add_placeholder_escape( $query );
@@ -3739 +3873 @@
 
     /**
-     * Determines if a database supports a particular feature.
+     * Determine DB or WPDB support for a particular feature.
+     *
+     * Capability sniffs for the database server and current version of wpdb.
+     *
+     * Database sniffs test based on the version of MySQL the site is using.
+     *
+     * WPDB sniffs are added as new features are introduced to allow theme and plugin
+     * developers to determine feature support. This is to account for drop-ins which may
+     * introduce feature support at a different time to WordPress.
      *
      * @since 2.7.0
      * @since 4.1.0 Added support for the 'utf8mb4' feature.
      * @since 4.6.0 Added support for the 'utf8mb4_520' feature.
+
      *
      * @see wpdb::db_version()
      *
      * @param string $db_cap The feature to check for. Accepts 'collation', 'group_concat',
-     *                       'subqueries', 'set_charset', 'utf8mb4', or 'utf8mb4_520'.
-     * @return int|false Whether the database feature is supported, false otherwise.
+     *                       'subqueries', 'set_charset', 'utf8mb4', 'utf8mb4_520',
+     *                       or 'identifier_placeholders'.
+     * @return bool True when the database feature is supported, false otherwise.
      */
     public function has_cap( $db_cap ) {
@@ -3783 +3927 @@
             case 'utf8mb4_520': // @since 4.6.0
                 return version_compare( $version, '5.6', '>=' );
+
+
         }
 

trunk/tests/phpunit/tests/db.php

@@ -493 +493 @@
         $this->assertTrue( $wpdb->has_cap( 'group_concat' ) );
         $this->assertTrue( $wpdb->has_cap( 'subqueries' ) );
+
         $this->assertTrue( $wpdb->has_cap( 'COLLATION' ) );
         $this->assertTrue( $wpdb->has_cap( 'GROUP_CONCAT' ) );
         $this->assertTrue( $wpdb->has_cap( 'SUBQUERIES' ) );
+
         $this->assertSame(
             version_compare( $wpdb->db_version(), '5.0.7', '>=' ),
@@ -1718 +1720 @@
                 "'hello' 'foo##'",
             ),
-        );
+            array(
+                'SELECT * FROM %i WHERE %i = %d;',
+                array( 'my_table', 'my_field', 321 ),
+                false,
+                'SELECT * FROM `my_table` WHERE `my_field` = 321;',
+            ),
+            array(
+                'WHERE %i = %d;',
+                array( 'evil_`_field', 321 ),
+                false,
+                'WHERE `evil_``_field` = 321;', // To quote the identifier itself, then you need to double the character, e.g. `a``b`.
+            ),
+            array(
+                'WHERE %i = %d;',
+                array( 'evil_````````_field', 321 ),
+                false,
+                'WHERE `evil_````````````````_field` = 321;',
+            ),
+            array(
+                'WHERE %i = %d;',
+                array( '``evil_field``', 321 ),
+                false,
+                'WHERE `````evil_field````` = 321;',
+            ),
+            array(
+                'WHERE %i = %d;',
+                array( 'evil\'field', 321 ),
+                false,
+                'WHERE `evil\'field` = 321;',
+            ),
+            array(
+                'WHERE %i = %d;',
+                array( 'evil_\``_field', 321 ),
+                false,
+                'WHERE `evil_\````_field` = 321;',
+            ),
+            array(
+                'WHERE %i = %d;',
+                array( 'evil_%s_field', 321 ),
+                false,
+                "WHERE `evil_{$wpdb->placeholder_escape()}s_field` = 321;",
+            ),
+            array(
+                'WHERE %i = %d;',
+                array( 'value`', 321 ),
+                false,
+                'WHERE `value``` = 321;',
+            ),
+            array(
+                'WHERE `%i = %d;',
+                array( ' AND evil_value', 321 ),
+                false,
+                'WHERE `` AND evil_value` = 321;', // Won't run (SQL parse error: "Unclosed quote").
+            ),
+            array(
+                'WHERE %i` = %d;',
+                array( 'evil_value -- ', 321 ),
+                false,
+                'WHERE `evil_value -- `` = 321;', // Won't run (SQL parse error: "Unclosed quote").
+            ),
+            array(
+                'WHERE `%i`` = %d;',
+                array( ' AND true -- ', 321 ),
+                false,
+                'WHERE `` AND true -- ``` = 321;', // Won't run (Unknown column '').
+            ),
+            array(
+                'WHERE ``%i` = %d;',
+                array( ' AND true -- ', 321 ),
+                false,
+                'WHERE ``` AND true -- `` = 321;', // Won't run (SQL parse error: "Unclosed quote").
+            ),
+            array(
+                'WHERE %2$i = %1$d;',
+                array( '1', 'two' ),
+                false,
+                'WHERE `two` = 1;',
+            ),
+            array(
+                'WHERE \'%i\' = 1 AND "%i" = 2 AND `%i` = 3 AND ``%i`` = 4 AND %15i = 5',
+                array( 'my_field1', 'my_field2', 'my_field3', 'my_field4', 'my_field5' ),
+                false,
+                'WHERE \'`my_field1`\' = 1 AND "`my_field2`" = 2 AND ``my_field3`` = 3 AND ```my_field4``` = 4 AND `      my_field5` = 5', // Does not remove any existing quotes, always adds it's own (safer).
+            ),
+            array(
+                'WHERE id = %d AND %i LIKE %2$s LIMIT 1',
+                array( 123, 'field -- ', false ),
+                true, // Incorrect usage.
+                null, // Should be rejected, otherwise the `%1$s` could use Identifier escaping, e.g. 'WHERE `field -- ` LIKE field --  LIMIT 1' (thanks @vortfu).
+            ),
+            array(
+                'WHERE %i LIKE %s LIMIT 1',
+                array( "field' -- ", "field' -- " ),
+                false,
+                "WHERE `field' -- ` LIKE 'field\' -- ' LIMIT 1", // In contrast to the above, Identifier vs String escaping is used.
+            ),
+        );
+    }
+
+    public function test_allow_unsafe_unquoted_parameters() {
+        global $wpdb;
+
+        $sql    = 'WHERE (%i = %s) OR (%10i = %10s) OR (%5$i = %6$s)';
+        $values = array( 'field_a', 'string_a', 'field_b', 'string_b', 'field_c', 'string_c' );
+
+        $default = $wpdb->allow_unsafe_unquoted_parameters;
+
+        $wpdb->allow_unsafe_unquoted_parameters = true;
+
+        // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared
+        $part = $wpdb->prepare( $sql, $values );
+        $this->assertSame( 'WHERE (`field_a` = \'string_a\') OR (`   field_b` =   string_b) OR (`field_c` = string_c)', $part ); // Unsafe, unquoted parameters.
+
+        $wpdb->allow_unsafe_unquoted_parameters = false;
+
+        // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared
+        $part = $wpdb->prepare( $sql, $values );
+        $this->assertSame( 'WHERE (`field_a` = \'string_a\') OR (`   field_b` = \'  string_b\') OR (`field_c` = \'string_c\')', $part );
+
+        $wpdb->allow_unsafe_unquoted_parameters = $default;
+
     }