Slashing consistency

[markjaquith]

In order to make SQL injection bugs easier to find, I propose that we standardize the way WP functions expect their data. Specifically, I propose that all WP functions expect unslashed data. This allows them to do their slashing within the function, before making SQL queries.

Expecting slashed data is dangerous. It's no problem if the data comes from GPC/S, but when data is manipulated or queried from the database, it can become unslashed. Passing unslashed data to functions that expect slashed data leads to SQL injection bugs.

On the other hand, passing slashed data to functions that expect unslashed data only leads to double-slashing... an annoyance, rather than a security hole. And if we implement this at the start of the 2.4 development cycle, we can eliminate any WP core instances of doubleslashing as well as give plugin authors 4 months of prep time to update plugins that use affected functions.

Instead of:

1. Get data passed to function
2. ?
3. Make SQL query

We can have a consistent flow of:

1. Get data passed to function
2. Escape it.
3. Make SQL query

This will make SQL injection bugs much more blatantly obvious. Right now, they are hard to track down, because Step 2 is an unknown.

[matt]

I like this, escaping should be done as close to the query as possible.

[intoxination]

This is a great idea to help prevent injections. I always thought that $wpdb->query should do the add/remove of slashes.

Of course this would mean plugin authors have to update their plugins so that data doesn't get double slashed or ran through stripslashes twice.

[nbachiyski]

+1

We can also make a prepared statement-like/printf-like method of wpdb, which can handle escaping internally and get rid of the few lines, before every query, spent in escaping.

[markjaquith]

This is a great idea to help prevent injections. I always thought that $wpdb->query should do the add/remove of slashes.

It can't. SQL Injection attacks are legitimate SQL queries (or else they wouldn't work). How do you tell a SQL Injection attack from a normal SQL query when both are legitimate queries?

I'm just proposing that the escaping we do be as close to the query as possible, so that failure to escape is more readily identifiable.

We can also make a prepared statement-like/printf-like method of wpdb, which can handle escaping internally and get rid of the few lines, before every query, spent in escaping.

This crossed my mind too.

Something like:

$result = $wpdb->get_results(
	$wpdb->prepare("SELECT something FROM $wpdb->tablename WHERE foo = '%s' LIMIT %d", $unslashed_value, $unslashed_uninted_limit)
);

We couldn't force people to use it, but it could really clean up our core code and help with SQL Injection squashing.

Heck, we could implement that now, for 2.3, for functions that already expect unslashed data (which I think is the majority, thankfully).

New ticket for that topic: #4553

[ryan]

Prepare is good by me. I think most functions expect slashed data though. update_option() and add_option() are two exceptions that expect unslashed data.

[pishmishy]

I'm not sure that this ticket is worth keeping open when #4553 appears to be addressing this issue. Resolve as duplicate?

[darkdragon]

Replying to pishmishy:

I'm not sure that this ticket is worth keeping open when #4553 appears to be addressing this issue. Resolve as duplicate?

Agreed. I think the prepared method work has just about solved a number of previous issues.