Escape SQL field names#4
Escape SQL field names#4szepeviktor wants to merge 2 commits intowp-cli:masterfrom szepeviktor:patch-1
Conversation
as poorly written plugins use `key` as field name.
|
Maybe I am wrong. I've escaped these: $primary_keys_sql = esc_sql( implode( ',', $primary_keys ) );
$col_sql = esc_sql( $col ); I think $primary_keys_sql could be many, I am not sure about $col_sql. Should I add backticks into implode()? |
Yes, I think this would be more appropriate. Are there any potential compatibility concerns with this change? And, does it need a test? |
I think no.
How about creating a custom table with a |
Works for me 👍 |
|
We need a guy to implement it :) |
|
@schlessera Are you comfortable with this change without including tests? |
|
|
||
| $where = $this->regex ? '' : " WHERE `$col`" . $wpdb->prepare( ' LIKE %s', '%' . self::esc_like( $old ) . '%' ); | ||
| $primary_keys_sql = esc_sql( implode( ',', $primary_keys ) ); | ||
| $primary_keys_sql = '`' . esc_sql( implode( '`,`', $primary_keys ) ) . '`'; |
There was a problem hiding this comment.
I'm not 100% sure, but I think this is vulnerable to SQL injection.
From the docs for esc_sql() function:
Note: Be careful to use this function correctly. It will only escape values to be used
in strings in the query. That is, it only provides escaping for values that will be within
quotes in the SQL (as in field = '{$escaped_value}'). If your value is not going to be
within quotes, your code will still be vulnerable to SQL injection. For example, this is
vulnerable, because the escaped value is not surrounded by quotes in the SQL
query: ORDER BY {$escaped_value}. As such, this function does not escape
unquoted numeric values, field names, or SQL keywords.
There was a problem hiding this comment.
I don't think that the backticks count as quotes. Moreover, I suspect that the combination of esc_sql() with backticks is vulnerable to SQL injection and will modify the keys so that they don't work (because of the added slashes).
There was a problem hiding this comment.
We should probably get some unit tests going for this.
|
Maybe I back off from this issue as the only thing known for me is the problem. |
|
Closing this for now. We can let poorly-written plugins be poorly written. |
3 participants
as poorly written plugins use
keyas field name.