[wp-hackers] Making WP more secure the evolutionary way

[Florian Thiel]

On Thu, Jan 22, 2009 at 11:32 AM, DD32 <wordpress at dd32.id.au> wrote:
> 2009/1/22 Florian Thiel <flo.thiel+wphackers at googlemail.com>:
>>> Also, It seems to be that you're suggesting in your patch that using
>>> raw SQL (even though its prepared) is a bad idea?  Or am i reading it
>>> wrong? :)
>>
>> Yes, that's my point, I'm probably not making that clear enough. The
>> whole idea is centralising access to the database. When you have 5
>> places in the system where the db is accessed, it's really easy to fix
>> things once you notice there is a security vulnerability. You're
>> fixing it in one place and all callers profit from that. Experience
>> shows that if you have critical things like db access all over the
>> place you will forget updating one or two places when applying a
>> security fix. When you look at the last XSS incidents with WP, there's
>> the exact pattern. The problem was fixed by applying another magic
>> escaping method to the vulnerable spot. This probably leaves other
>> places vulnerable because escaping is not done in a consistent way
>> everywhere it's used. Obviously, you still need to find all the places
>> where db access (or client output) happens. But you only have to do
>> that one time.
>
> Thats the thing, I dont see how this is different from current. All
> queries which use user-data go through the wpdb::prepare() function,
> which sanitizes the data for the query.. wpdb->prepare("SELECT a, b, c
> FROM table WHERE a = %s", ' '\ UNION...'); would result in the -exact-
> variable passed in being quoted -correctly-, Of course, since
> wpdb::query() uses addslashes() instead of mysql_real_escape_string()
> There may be chars which make it through that.. But thats beside the
> point, Do you see where my confusion lies?

Using prepare everywhere is definitely a step in the right direction.
The problem is that it does not help against flawed SQL passed from
the caller. If you do it like it's done for INSERT and UPDATE now, you
control the construction of SQL statements in one place and can avoid
constructing evil SQL. Since you have an abstract representation of
the statement (semantically richer) you have much more control over
the SQL generated. You could even do fancy things like making sure
that all columns mentioned in SELECT or WHERE really exist. We don't
have to go that far, but once it's centralized, improving these
central methods has a much larger payoff, since you only have to do it
once. That justifies spending more time on this, creating a more
secure WordPress in the end.

Florian