Make WordPress Core

Opened 14 years ago

Closed 11 years ago

Last modified 11 years ago

#12416 closed defect (bug) (fixed)

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

Reported by: denis-de-bernardy's profile Denis-de-Bernardy Owned by: ryan's profile 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:

http://core.trac.wordpress.org/ticket/9015#comment:136

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 14 years ago.
untested
12416.2.diff (13.2 KB) - added by ryan 14 years ago.
Refreshed patch
12416sprintf.diff (1.4 KB) - added by jamescollins 14 years ago.
Fixes the usage of %s in $wpdb->prepare() calls
12416.3.diff (2.7 KB) - added by nacin 14 years ago.
Expect unslashed in meta api

Download all attachments as: .zip

Change History (39)

#1 @Denis-de-Bernardy
14 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
14 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
14 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
14 years ago

See also #11956 while we're at it.

#5 @Denis-de-Bernardy
14 years ago

set_theme_mod() has an unsanitized option name

#6 @Denis-de-Bernardy
14 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
14 years ago

  • Priority changed from normal to high

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

@Denis-de-Bernardy
14 years ago

untested

#8 @Denis-de-Bernardy
14 years ago

  • Keywords needs-testing added

#10 @ryan
14 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
14 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
14 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
14 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.

@ryan
14 years ago

Refreshed patch

#14 @ryan
14 years ago

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

#15 @ryan
14 years ago

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

#16 @miqrogroove
14 years ago

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

#17 @nacin
14 years ago

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

#18 @nacin
14 years ago

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

#19 @nacin
14 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
14 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].

@jamescollins
14 years ago

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

#21 @dd32
14 years ago

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

@nacin
14 years ago

Expect unslashed in meta api

#22 @nacin
14 years ago

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

#23 @nacin
14 years ago

  • Keywords has-patch added

Patch added to include meta API in this.

#24 @ryan
14 years ago

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

#25 @ryan
14 years ago

  • Milestone changed from 3.0 to 3.1

We'll do meta in 3.1.

#26 @hakre
13 years ago

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

#27 @nacin
13 years ago

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

#28 @nacin
13 years ago

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

#29 @ryan
13 years ago

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

#30 @Denis-de-Bernardy
13 years ago

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

#31 @nbachiyski
13 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
13 years ago

  • Cc nbachiyski added

#33 @wonderboymusic
11 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
11 years ago

Yes, we can pick this up there.

#35 @nacin
11 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.