Ticket #4545 (closed task: duplicate)

Opened 1 year ago

Last modified 1 year ago

Slashing consistency

Reported by: markjaquith Assigned to: anonymous
Priority: normal Milestone:
Component: Security Version: 2.3
Severity: normal Keywords:
Cc:

Description

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.

Change History

06/27/07 10:48:29 changed by matt

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

06/27/07 12:44:02 changed by 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.

06/27/07 17:25:23 changed by 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.

06/27/07 21:10:45 changed by 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

06/27/07 22:46:52 changed by 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.

(follow-up: ↓ 7 ) 11/01/07 15:48:07 changed by pishmishy

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

(in reply to: ↑ 6 ) 12/28/07 21:02:46 changed by 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.

01/06/08 17:24:39 changed by pishmishy

  • status changed from new to closed.
  • resolution set to duplicate.
  • milestone deleted.