WordPress.org

Make WordPress Core

Opened 5 months ago

Last modified 4 months ago

#52506 new defect (bug)

Add escaping method for table names in SQL queries

Reported by: tellyworth Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Database Keywords: dev-feedback
Focuses: Cc:

Description

WordPress does not currently provide an explicit method for escaping SQL table names. This leads to potential security vulnerabilities, and makes reviewing code for security unnecessarily difficult.

Core tables are of course implicitly escaped when referenced in queries like SELECT * FROM $wpdb->posts. That’s fine.

When plugins create or reference custom table names, there are no API methods and no guidance as to how they should safely escape those names. Since $wpdb->prepare() surrounds %s references with ' quotes in a way that is incompatible with MySQL table name syntax, it becomes necessary to use bare PHP variables in the first parameter of prepare(), which is inherently risky. Plugins use a variety of ways of mitigating that risk, including none at all. These are examples paraphrased from real code in popular plugins:

$wpdb->query( $wpdb->prepare( "SELECT * FROM my_table_name WHERE …" ) ); // Literal string

$wpdb->query( $wpdb->prepare( "SELECT * FROM $my_table_name WHERE …" ) ); // Variable, which may or may not be assigned a literal or escaped in some way

$wpdb->query( $wpdb->prepare( "SELECT * FROM $this->table_name WHERE …" ) ); // Object property

$wpdb->query( $wpdb->prepare( "SELECT * FROM {$wpdb->prefix}my_table_name WHERE …" ) ); // Variable prefix with literal string

$wpdb->query( $wpdb->prepare( "SELECT * FROM {$this->get_table_name()} WHERE …" ) ); // Interpolated method call

$wpdb->query( $wpdb->prepare( "SELECT * FROM " . sanitize_key( $table_name ) . " WHERE …" ) ); // Attempted escaping

$wpdb->query( $wpdb->prepare( "SELECT * FROM " . preg_replace( '/\W/', '', $table_name ) . " WHERE …" ) ); // Hand-rolled escaping

$wpdb->query( $wpdb->prepare( "SELECT * FROM " . self::TABLE_NAME . " WHERE …" ) ); // Class constant

Some variations use backticks around table names. And of course there are many other examples using similar queries without prepare() statements. Note that no guidance is given to plugin developers, so every one has had to separately decide how to handle this.

Other than the literal string examples, none of these examples are verifiably secure. They might be fine, but it is impossible to be sure without reviewing the code path leading up to that point. Even in the case of a constant, it is possible that the constant might in some circumstances be initialized with data from an insecure source. In cases that make use of OO, it is possible that the table name is initialized in a parent class in a different file or module. It might also be overridden in child classes.

All of this makes it difficult for a human code reviewer to be certain that a given query is secure, even though it uses prepare() statements. Static code analysis is similarly difficult. Developers who make use of wpcs and similar tools inevitably need to exclude their queries from sniffer rules because they will otherwise cause false positive errors.

The solution would be for WordPress to provide an explicit method for safely and defensively escaping table names, as close as possible to the query itself.

I can think of several options:

An esc_sql_table_name() function, to be used like this:

$wpdb->query( $wpdb->prepare( "SELECT * FROM " . esc_sql_table_name( $my_table_name ) . " WHERE …" ) );

A special formatting character supported by prepare(), something like:

$wpdb->query( $wpdb->prepare( "SELECT * FROM %z WHERE …", $my_table_name ) );

A modification to the %s character format that does not add ' quotes when surrounded with backticks:

$wpdb->query( $wpdb->prepare( "SELECT * FROM `%s` WHERE …", $my_table_name ) );

A way to safely add sanitized table names to the $wpdb object:

$wpdb->add_table_name( 'my_table_name' );
$wpdb->query( $wpdb->prepare( "SELECT * FROM $wpdb->my_table_name WHERE …" ) );

This last option does look attractive, but unfortunately doesn’t help with the use case where the table name is necessarily variable such as in a loop, which is somewhat common in things like backup plugins:

foreach ( get_list_of_table_names() as $table_name ) {
	$wpdb->query( $wpdb->prepare( "SELECT COUNT(*) FROM $table_name WHERE …" ) );
}

I would therefore tend to favour both adding an esc_sql_table_name() function, and also supporting one of the special prepare() formatting options (which would of course make use of esc_sql_table_name() behind the scenes).

Note that there is some obvious overlap with the very similar problem of escaping column names. I think that warrants a separate ticket, but the solution is probably similar as for table names.

Change History (1)

#1 @iandunn
4 months ago

+1 to this


Developers who make use of wpcs and similar tools inevitably need to exclude their queries from sniffer rules because they will otherwise cause false positive errors.

Even that is problematic, since it opens the door for future mistakes. Take this for instance:

$reimbursements_index = Reimbursements_Dashboard\get_index_table_name();

// phpcs:ignore WordPressDotOrg.sniffs.DirectDB.UnescapedDBParameter -- this is safe
$paid_reimbursements = $wpdb->get_results( "
        SELECT blog_id, request_id, date_paid
        FROM `$reimbursements_index`
        WHERE status = 'wcb-paid'
" );

That's fine, but another developer could come along 6 months later and change it to this:

$reimbursements_index = Reimbursements_Dashboard\get_index_table_name();

// phpcs:ignore WordPressDotOrg.sniffs.DirectDB.UnescapedDBParameter -- this is safe
$paid_reimbursements = $wpdb->get_results( "
        SELECT blog_id, request_id, date_paid
        FROM `$reimbursements_index`
        WHERE
                status = 'wcb-paid' AND
                name = " . $_GET['name']
) );

adding an esc_sql_table_name() function, and also supporting one of the special prepare() formatting options

+1

To be descriptive, and avoid potential conflicts with future printf() formats, maybe something like:

$wpdb->query( $wpdb->prepare( "SELECT * FROM %table WHERE …", 'foo' ) ); becomes
"SELECT * FROM `foo` WHERE …"


A modification to the %s character format that does not add ' quotes when surrounded with backticks

My instinct is to be a bit leery of that, since it'd be a bit more complicated to implement, which could introduce the potential for esoteric bypasses. I could be convinced that I'm being too paranoid, though.

Note: See TracTickets for help on using tickets.