WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 5 years ago

#12402 closed defect (bug) (fixed)

make addslashes_gpc() use addslashes() fix to use real_escape, rather than addslashes

Reported by: Denis-de-Bernardy Owned by:
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.0
Component: Security Keywords: has-patch needs-testing
Focuses: Cc:

Description

If memory serves, the main problems with making wpdb->escape() point to mysql_real_escape_string() were two:

  1. mysql_real_escape_string() doesn't work on all platforms
  2. mysql_real_escape_string() doesn't play well with unslashing

Point 1 is covered in wpdb->_real_escape().

The legacy fix to point 2, for some strange reason, led to disabling mysql_real_escape_string() on platforms that could actually use it, instead of using add_magic_quotes() where calls to addslashes()/stripslashes() could occur.

r12961 partially fixes point 2. But only partially.

At the risk of re-opening the can of worms, the attached patch finishes fixing point 2...:

  • It changes addslashes_gpc() and esc_sql() so that they use add_magic_quotes() instead.
  • This frees wpdb->escape(), which can then be a true alias for wpdb->_real_escape().

I've scanned trunk for occurrences of wpdb->escape() used on arrays -- there were none.

Attachments (2)

12402.diff (1.8 KB) - added by Denis-de-Bernardy 10 years ago.
12402.2.diff (3.2 KB) - added by ryan 10 years ago.
Introduce addslashes_deep() and standardize on it.

Download all attachments as: .zip

Change History (18)

#2 follow-up: @miqrogroove
10 years ago

I'll give this +1 if the patch makes esc_sql() deprecated or aliases weak_escape instead of magic_quotes. Otherwise, that can of worms is looking rather large.

#3 in reply to: ↑ 2 ; follow-up: @Denis-de-Bernardy
10 years ago

Replying to miqrogroove:

I'll give this +1 if the patch makes esc_sql() deprecated or aliases weak_escape instead of magic_quotes. Otherwise, that can of worms is looking rather large.

I think that, in the long term esc_sql() should call wpdb->escape(). But for the sake of doing incremental fixes, however, I felt it was better to just keep esc_sql()'s behavior unchanged for now (which is exactly what the patch does).

We'll look into making esc_sql() call _real_escape() once this gets checked in. I think it'll largely be a matter of replacing most calls to esc_sql() into calls to add_magic_quotes().

#4 in reply to: ↑ 3 @miqrogroove
10 years ago

Replying to Denis-de-Bernardy:

keep esc_sql()'s behavior unchanged for now (which is exactly what the patch does).

Take another look at add_magic_quotes() then. It is not equivalent to what you're trying to do here.

#5 @Denis-de-Bernardy
10 years ago

err... I might be reading this wrong, but, in trunk at the time of writing this:

function add_magic_quotes( $array ) {
	foreach ( (array) $array as $k => $v ) {
		if ( is_array( $v ) ) {
			$array[$k] = add_magic_quotes( $v );
		} else {
			$array[$k] = addslashes( $v );
		}
	}
	return $array;
}


	function escape( $data ) {
		if ( is_array( $data ) ) {
			foreach ( (array) $data as $k => $v ) {
				if ( is_array( $v ) )
					$data[$k] = $this->escape( $v );
				else
					$data[$k] = $this->_weak_escape( $v );
			}
		} else {
			$data = $this->_weak_escape( $data );
		}

		return $data;
	}

both are poorly written versions of the same thing, i.e.:

function addslashes_deep($data) {
    return is_array($data) ? array_map('addslashes_deep', $data) : addslashes($data);
}

#6 @Denis-de-Bernardy
10 years ago

actually... nevermind... you're right. add_magic_quotes() only accepts an array, while esc_sql accepts anything.

I'd like to see some dev feedback before updating the patch, though...

#8 @westi
10 years ago

  • Cc westi added

If my memory serves me correctly isn't one of the big issues with mysql_real_escape_string that it just doesn't work well for anything remotely non-ascii/UTF8 like Shift-JIS or Big5.

It is only a distant memory but I thought it worth adding to the discussion so someone could correct me if I was wrong :)

#9 @Denis-de-Bernardy
10 years ago

My understanding is the real issue was that we'd stripslash() $_POST and $_GET, and then slash them using mysql_real_escape_string(). but by doing so, we add more slashes than addslashes would, so doing the same over again for any reason would return garbage.

#10 @ryan
10 years ago

addslashes_gpc() should definitely be using addslashes() and not esc_sql(). Can we get that part into 3.0 and punt the rest?

@ryan
10 years ago

Introduce addslashes_deep() and standardize on it.

#11 @miqrogroove
10 years ago

You guys are playing with fire here. I would punt it.

#12 @ryan
10 years ago

  • Milestone changed from 3.0 to 3.1

#13 @hakre
10 years ago

Just to make bold what miqrogroove just wrote: Blindly using "real_escape" does not help if there exists no function to revert it on the same data to use decoded values on various places. Currently stripslashes() is used to revert addslashes(). That won't work for real_escape() conceptually and the whole codebase must be changed from strispashes() -> real_unescape() of which later is missing (!).

Related: #14169

#14 @nacin
10 years ago

  • Milestone changed from Awaiting Triage to Future Release

#15 @ryan
6 years ago

  • Owner ryan deleted
  • Status changed from new to assigned

#16 @johnbillion
5 years ago

  • Milestone changed from Future Release to 3.6
  • Resolution set to fixed
  • Status changed from assigned to closed

This was effectively dealt with in r23555 and r23591.

Note: See TracTickets for help on using tickets.