Two weeks ago there was discussion on our post detailing a vulnerability in the plugin Gallery – Video Gallery over the escaping method being used to fix a SQL injection vulnerability in the plugin. While the changes made look to have fixed the issue, they were less than ideal. Part of the issue was that instead of using a prepared statement to prevent the possibility of SQL injection the function esc_sql() was used. That is despite documentation for that that function making a pretty clear suggestion that it is not recommend in most situations:

In 99% of cases, you can use $wpdb->prepare() instead, and that is the recommended method. This function is only for use in those rare cases where you can’t easily use $wpdb->prepare().

The documentation then notes that:

Note: Be careful to use this function correctly. It will only escape values to be used in strings in the query.

That discussion reminded us of an attempt to fix a SQL injection vulnerability in another plugin, Simple Personal Message, where that warning was not heeded and it mattered. What made that situation troubling was that plugin had been removed from the Plugin Directory due to the vulnerability and was restored without actually being fixed, due to esc_sql() being used where it wasn’t appropriate (after we reported that the change didn’t fix the issue it was removed again).

The problem was that the value that permitted SQL injection is run through esc_sql() then placed in to query where it is not in quotes:

$id = esc_sql(esc_attr($_GET['message'])); 
 
$message = $wpdb->get_results("SELECT * FROM $table WHERE id = $id");

Using a prepared statement isn’t much more difficult than using esc_sql() for that code and it avoids the problem that comes with using esc_sql(). The prepared statement replacement for the code could be done like this:

$message = $wpdb->get_results( $wpdb->prepare( "SELECT * FROM $table WHERE id = %d", $_GET['message']) );

With that the SQL query and the user input value are separated, so the code understands the user input is only intended to contain a value and not additional SQL code, which would permit SQL injection to occur.

Need Help Fixing a Vulnerability in Your Plugin?