WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#12402 new defect (bug)

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

Reported by: Denis-de-Bernardy Owned by: ryan
Milestone: Future Release 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 4 years ago.
12402.2.diff (3.2 KB) - added by ryan 4 years ago.
Introduce addslashes_deep() and standardize on it.

Download all attachments as: .zip

Change History (16)

Denis-de-Bernardy4 years ago

comment:2 follow-up: miqrogroove4 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.

comment:3 in reply to: ↑ 2 ; follow-up: Denis-de-Bernardy4 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().

comment:4 in reply to: ↑ 3 miqrogroove4 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.

comment:5 Denis-de-Bernardy4 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);
}

comment:6 Denis-de-Bernardy4 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...

comment:8 westi4 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 :)

comment:9 Denis-de-Bernardy4 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.

comment:10 ryan4 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?

ryan4 years ago

Introduce addslashes_deep() and standardize on it.

comment:11 miqrogroove4 years ago

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

comment:12 ryan4 years ago

  • Milestone changed from 3.0 to 3.1

comment:13 hakre4 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

comment:14 nacin3 years ago

  • Milestone changed from Awaiting Triage to Future Release
Note: See TracTickets for help on using tickets.