Make WordPress Core

Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#12416 closed defect (bug) (fixed)

*_option() and *_transient() functions should expect unslashed data.

Reported by: Denis-de-Bernardy Owned by: ryan
Milestone: 3.0 Priority: high
Severity: critical Version: 3.0
Component: Security Keywords: needs-patch
Focuses: Cc:

Description (last modified by Denis-de-Bernardy)

Following up on:


It's totally irresponsible to expect plugin authors to escape whatever they send into get_option(). As things stand:

  • get_option(), delete_option(), get_site_option() assume it's slashed
  • add_option(), update_option(), add_site_option() seem to assume it's unslashed, as does get_option()
  • *_transient() seem to expect unslashed input.
  • delete_site_option() is very special: it expects slashed input if you're not on using multisite, but unslashed if you are
  • update_site_option() is equallty special: it needs slashed input if the option is not loaded already, and unslashed input if it is

the list goes on, and on... these inconsistencies, which come on top of the *_meta() functions, which expect slashed data.

it's totally nuts, insecure, and irresponsible. especially considering calls in WP such as:

  • get_option("{$size}_crop");

or functions such as:

function form_option( $option ) {
	echo esc_attr( get_option( $option ) );

function get_settings_errors( $setting = '', $sanitize = FALSE ) {
	global $wp_settings_errors;
	//... isn't it ironic that using sanitize here makes it LESS secure?
	if ( $sanitize )
		sanitize_option( $setting, get_option($setting));
	// ...

we're asking for trouble here...

Attachments (4)

12416.diff (13.5 KB) - added by Denis-de-Bernardy 6 years ago.
12416.2.diff (13.2 KB) - added by ryan 6 years ago.
Refreshed patch
12416sprintf.diff (1.4 KB) - added by jamescollins 6 years ago.
Fixes the usage of %s in $wpdb->prepare() calls
12416.3.diff (2.7 KB) - added by nacin 6 years ago.
Expect unslashed in meta api

Download all attachments as: .zip

Change History (39)

#1 @Denis-de-Bernardy
6 years ago

  • Description modified (diff)
  • Summary changed from *_option() should all expect unslashed data. to *_option(), *_transient() and *_meta() functions should all expect unslashed data.

#2 @nacin
6 years ago

I had noted some of this in #10788. I also discovered some odd multisite-dependent issues related to serialization, which were fixed there. I agree it is largely nonsensical, but how much can we do here?

#3 @Denis-de-Bernardy
6 years ago

Imo, we should expect unslashed input absolutely everywhere, even if it means introducing a few backwards compat issues in plugins from authors who know better.

We should also release WP 2.9.3 before SQL injection related hacks are all over the place. The number of potential loopholes related to this is too large for us to "wait for a worm to creep up". Especially if you consider that few plugin authors know that *_meta() expects slashed input. I take it that even fewer are aware that *_option() expects inconsistently slashed data.

#4 @Denis-de-Bernardy
6 years ago

See also #11956 while we're at it.

#5 @Denis-de-Bernardy
6 years ago

set_theme_mod() has an unsanitized option name

#6 @Denis-de-Bernardy
6 years ago

Here's one where we might be passing unslashed garbage straight into MySQL:

function maybe_add_existing_user_to_blog() {
	if ( false === strpos( $_SERVER[ 'REQUEST_URI' ], '/newbloguser/' ) )
		return false;

	$parts = explode( '/', $_SERVER[ 'REQUEST_URI' ] );
	$key = array_pop( $parts );

	if ( $key == '' )
		$key = array_pop( $parts );

	$details = get_option( "new_user_" . $key );
	add_existing_user_to_blog( $details );
	delete_option( 'new_user_' . $key );
	wp_die( sprintf(__('You have been added to this blog. Please visit the <a href="%s">homepage</a> or <a href="%s">login</a> using your username and password.'), site_url(), admin_url() ) );

#7 @miqrogroove
6 years ago

  • Priority changed from normal to high

+1 as that code appears to be unconditionally hooked by ms-default-filters.php

6 years ago


#8 @Denis-de-Bernardy
6 years ago

  • Keywords needs-testing added

#10 @ryan
6 years ago

Given how inconsistent core WP itself is about slashing, I'm inclined to go with this. There will be some back-compat grief, but it's looking like more plugins are not escaping than are escaping. We'd be servicing the more popular expectation.

#11 @Denis-de-Bernardy
6 years ago

cool. I didn't do the meta functions in my patch. want me to go through those as well?

#12 follow-up: @nacin
6 years ago

(In [13508]) Don't pass serialized values to hooks in options API. see #10788 see #12416

#13 in reply to: ↑ 12 @Denis-de-Bernardy
6 years ago

Replying to nacin:

(In [13508]) Don't pass serialized values to hooks in options API. see #10788 see #12416

ah, cool. that one got me scratching my head. :D

wpmuguru might know what that was about.

my patch needs a slight refresh because of this, too.

6 years ago

Refreshed patch

#14 @ryan
6 years ago

Did some light testing. Saved some options with quotes. Deleted all transients and let them regenerate.

#15 @ryan
6 years ago

(In [13673]) make *_option(), *_transient() functions consistently expect unslashed data. Props Denis-de-Bernardy. see #12416

#16 @miqrogroove
6 years ago

Notice: Undefined variable: _alloptions in /wp-includes/functions.php on line 518

#17 @nacin
6 years ago

(In [13677]) Fix typo in [13673]. see #12416

#18 @nacin
6 years ago

Do we plan do to this with _meta() as well?

#19 @nacin
6 years ago

(In [14074]) Revert changes to maybe_serialize() made in r13673. Do not prevent double-serialization of data. see #12930. see also #12416. xref #7347, r8100, r8372, and others.

#20 @jamescollins
6 years ago

In [13673], some $wpdb->prepare() calls were introduced that use '%s' instead of %s.

According to http://core.trac.wordpress.org/browser/trunk/wp-includes/wp-db.php#L856, these should be left unquoted.

As per http://core.trac.wordpress.org/browser/trunk/wp-includes/wp-db.php#L884), this won't actually cause problems, however I still think the instances of '%s' should be changed to %s in $wpdb->prepare() calls.

There are 3 instances of this in [13673].

6 years ago

Fixes the usage of %s in $wpdb->prepare() calls

#21 @dd32
6 years ago

(In [14211]) s/'%s'/%s/ in prepares. Props jamescollins. See r13673. See #12416

6 years ago

Expect unslashed in meta api

#22 @nacin
6 years ago

(In [14547]) Revert patch for expecting unslashed data in the metadata API. Had snuck in with [14546]. see #12416.

#23 @nacin
6 years ago

  • Keywords has-patch added

Patch added to include meta API in this.

#24 @ryan
6 years ago

We want to remove the stripslashes_deep() on the meta_values too, right?

#25 @ryan
6 years ago

  • Milestone changed from 3.0 to 3.1

We'll do meta in 3.1.

#26 @hakre
5 years ago

Any traction on this one? Is this still considered for 3.1?

#27 @nacin
5 years ago

  • Milestone changed from Awaiting Triage to 3.1
  • Severity changed from blocker to critical

#28 @nacin
5 years ago

  • Keywords needs-patch added; needs-testing has-patch removed

#29 @ryan
5 years ago

  • Keywords 3.2-early added
  • Milestone changed from 3.1 to Future Release

#30 @Denis-de-Bernardy
5 years ago

Last edited 5 years ago by Denis-de-Bernardy (previous) (diff)

#31 @nbachiyski
4 years ago

I would really love to see this committed and I would be happy to help.

Does the ticket has needs-patch label because the current patch is missing the part, which removes stripslashes for values?

#32 @nbachiyski
4 years ago

  • Cc nbachiyski added

#33 @wonderboymusic
3 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

We're ditching this in favor of #21767, correct?

#34 @ryan
3 years ago

Yes, we can pick this up there.

#35 @nacin
3 years ago

  • Keywords 3.2-early removed
  • Milestone set to 3.0
  • Resolution changed from wontfix to fixed
  • Summary changed from *_option(), *_transient() and *_meta() functions should all expect unslashed data. to *_option() and *_transient() functions should expect unslashed data.

Closing this as fixed on 3.0 for *_option() and *_transient() functions.

Note: See TracTickets for help on using tickets.