From e7a9fe918c352ee87063a4a1857fc86f3e840c04 Mon Sep 17 00:00:00 2001 From: davidbaumwald Date: Fri, 24 Jun 2022 20:35:14 +0000 Subject: [PATCH] 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. Built from https://develop.svn.wordpress.org/trunk@53575 git-svn-id: http://core.svn.wordpress.org/trunk@53164 1a063a9b-81f0-0310-95a4-ce76da25c4cd --- wp-includes/version.php | 2 +- wp-includes/wp-db.php | 222 +++++++++++++++++++++++++++++++++------- 2 files changed, 185 insertions(+), 39 deletions(-) diff --git a/wp-includes/version.php b/wp-includes/version.php index 2d8fa9efae..d40a2aaee1 100644 --- a/wp-includes/version.php +++ b/wp-includes/version.php @@ -16,7 +16,7 @@ * * @global string $wp_version */ -$wp_version = '6.1-alpha-53574'; +$wp_version = '6.1-alpha-53575'; /** * Holds the WordPress DB revision, increments when changes are made to the WordPress DB schema. diff --git a/wp-includes/wp-db.php b/wp-includes/wp-db.php index 44a4434319..437646e3ec 100644 --- a/wp-includes/wp-db.php +++ b/wp-includes/wp-db.php @@ -644,6 +644,34 @@ class wpdb { 'ANSI', ); + /** + * Backwards compatibility, where wpdb::prepare() has not quoted formatted/argnum placeholders. + * + * Historically this could be used for table/field names, or for some string formatting, e.g. + * $wpdb->prepare( 'WHERE `%1s` = "%1s something %1s" OR %1$s = "%-10s"', 'field_1', 'a', 'b', 'c' ); + * + * But it's risky, e.g. forgetting to add quotes, resulting in SQL Injection vulnerabilities: + * $wpdb->prepare( 'WHERE (id = %1s) OR (id = %2$s)', $_GET['id'], $_GET['id'] ); // ?id=id + * + * This feature is preserved while plugin authors update their code to use safer approaches: + * $wpdb->prepare( 'WHERE %1s = %s', $_GET['key'], $_GET['value'] ); + * $wpdb->prepare( 'WHERE %i = %s', $_GET['key'], $_GET['value'] ); + * + * While changing to false will be fine for queries not using formatted/argnum placeholders, + * any remaining cases are most likely going to result in SQL errors (good, in a way): + * $wpdb->prepare( 'WHERE %1s = "%-10s"', 'my_field', 'my_value' ); + * true = WHERE my_field = "my_value " + * false = WHERE 'my_field' = "'my_value '" + * But there may be some queries that result in an SQL Injection vulnerability: + * $wpdb->prepare( 'WHERE id = %1s', $_GET['id'] ); // ?id=id + * So there may need to be a `_doing_it_wrong()` phase, after we know everyone can use Identifier + * placeholders (%i), but before this feature is disabled or removed. + * + * @since 6.1.0 + * @var bool + */ + private $allow_unsafe_unquoted_parameters = true; + /** * Whether to use mysqli over mysql. Default false. * @@ -1347,6 +1375,37 @@ class wpdb { } } + /** + * Escapes an identifier for a MySQL database (e.g. table/field names). + * + * @since 6.1.0 + * + * @param string $identifier Identifier to escape. + * @return string Escaped Identifier + */ + public function escape_identifier( $identifier ) { + return '`' . $this->_escape_identifier_value( $identifier ) . '`'; + } + + /** + * Escapes an identifier value. + * + * Escapes an identifier value without adding the surrounding quotes. + * + * - Permitted characters in quoted identifiers include the full Unicode Basic Multilingual Plane (BMP), except U+0000 + * - To quote the identifier itself, then you need to double the character, e.g. `a``b` + * + * @link https://dev.mysql.com/doc/refman/8.0/en/identifiers.html + * @since 6.1.0 + * @access private + * + * @param string $identifier Identifier to escape. + * @return string Escaped Identifier + */ + private function _escape_identifier_value( $identifier ) { + return str_replace( '`', '``', $identifier ); + } + /** * Prepares a SQL query for safe execution. * @@ -1355,6 +1414,7 @@ class wpdb { * - %d (integer) * - %f (float) * - %s (string) + * - %i (identifier, e.g. table/field names) * * All placeholders MUST be left unquoted in the query string. A corresponding argument * MUST be passed for each placeholder. @@ -1380,6 +1440,10 @@ class wpdb { * @since 5.3.0 Formalized the existing and already documented `...$args` parameter * by updating the function signature. The second parameter was changed * from `$args` to `...$args`. + * @since 6.1.0 Added '%i' for Identifiers, e.g. table or field names. + * Check support via `wpdb::has_cap( 'identifier_placeholders' )` + * This preserves compatibility with sprinf, as the C version uses %d and $i + * as a signed integer, whereas PHP only supports %d. * * @link https://www.php.net/sprintf Description of syntax. * @@ -1411,28 +1475,6 @@ class wpdb { ); } - // 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: * @@ -1453,19 +1495,82 @@ class wpdb { */ $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( '/(?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_count ) { + if ( 1 === $placeholder_count && $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(); _doing_it_wrong( @@ -1486,7 +1591,7 @@ class wpdb { sprintf( /* 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_count, $args_count ), '4.8.3' @@ -1496,9 +1601,14 @@ class wpdb { * If we don't have enough arguments to match the placeholders, * 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 ''; } @@ -1506,8 +1616,32 @@ class wpdb { } } - 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 ); } @@ -3738,17 +3872,27 @@ class wpdb { } /** - * 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. + * @since 6.1.0 Added support for the 'identifier_placeholders' 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 ) { $version = $this->db_version(); @@ -3782,6 +3926,8 @@ class wpdb { } case 'utf8mb4_520': // @since 4.6.0 return version_compare( $version, '5.6', '>=' ); + case 'identifier_placeholders': // @since 6.1.0, wpdb::prepare() supports identifiers via '%i' - e.g. table/field names. + return true; } return false;