WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 15 months ago

Last modified 15 months 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:

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

Download all attachments as: .zip

Change History (39)

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

comment:2 nacin4 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?

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

comment:4 Denis-de-Bernardy4 years ago

See also #11956 while we're at it.

comment:5 Denis-de-Bernardy4 years ago

set_theme_mod() has an unsanitized option name

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

comment:7 miqrogroove4 years ago

  • Priority changed from normal to high

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

Denis-de-Bernardy4 years ago

untested

comment:8 Denis-de-Bernardy4 years ago

  • Keywords needs-testing added

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

comment:11 Denis-de-Bernardy4 years ago

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

comment:12 follow-up: nacin4 years ago

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

comment:13 in reply to: ↑ 12 Denis-de-Bernardy4 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.

ryan4 years ago

Refreshed patch

comment:14 ryan4 years ago

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

comment:15 ryan4 years ago

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

comment:16 miqrogroove4 years ago

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

comment:17 nacin4 years ago

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

comment:18 nacin4 years ago

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

comment:19 nacin4 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.

comment:20 jamescollins4 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].

jamescollins4 years ago

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

comment:21 dd324 years ago

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

nacin4 years ago

Expect unslashed in meta api

comment:22 nacin4 years ago

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

comment:23 nacin4 years ago

  • Keywords has-patch added

Patch added to include meta API in this.

comment:24 ryan4 years ago

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

comment:25 ryan4 years ago

  • Milestone changed from 3.0 to 3.1

We'll do meta in 3.1.

comment:26 hakre4 years ago

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

comment:27 nacin3 years ago

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

comment:28 nacin3 years ago

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

comment:29 ryan3 years ago

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

comment:30 Denis-de-Bernardy3 years ago

Version 0, edited 3 years ago by Denis-de-Bernardy (next)

comment:31 nbachiyski3 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?

comment:32 nbachiyski3 years ago

  • Cc nbachiyski added

comment:33 wonderboymusic15 months ago

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

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

comment:34 ryan15 months ago

Yes, we can pick this up there.

comment:35 nacin15 months 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.