#12416 closed defect (bug) (fixed)
*_option() and *_transient() functions should expect unslashed data.
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Priority: | high | Milestone: | 3.0 |
| Component: | Security | Version: | 3.0 |
| Severity: | critical | Keywords: | needs-patch |
| Cc: | nbachiyski |
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)
Change History (39)
- Description modified (diff)
- Summary changed from *_option() should all expect unslashed data. to *_option(), *_transient() and *_meta() functions should all expect unslashed data.
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.
See also #11956 while we're at it.
set_theme_mod() has an unsanitized option name
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
miqrogroove — 3 years ago
- Priority changed from normal to high
+1 as that code appears to be unconditionally hooked by ms-default-filters.php
- Keywords needs-testing added
related: #11771
comment:10
ryan — 3 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.
cool. I didn't do the meta functions in my patch. want me to go through those as well?
comment:12
follow-up:
↓ 13
nacin — 3 years ago
comment:13
in reply to:
↑ 12
Denis-de-Bernardy — 3 years ago
comment:14
ryan — 3 years ago
Did some light testing. Saved some options with quotes. Deleted all transients and let them regenerate.
comment:15
ryan — 3 years ago
comment:16
miqrogroove — 3 years ago
Notice: Undefined variable: _alloptions in /wp-includes/functions.php on line 518
comment:18
nacin — 3 years ago
Do we plan do to this with _meta() as well?
comment:19
nacin — 3 years ago
comment:20
jamescollins — 3 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].
comment:21
dd32 — 3 years ago
comment:22
nacin — 3 years ago
comment:24
ryan — 3 years ago
We want to remove the stripslashes_deep() on the meta_values too, right?
comment:26
hakre — 3 years ago
Any traction on this one? Is this still considered for 3.1?
comment:27
nacin — 3 years ago
- Milestone changed from Awaiting Triage to 3.1
- Severity changed from blocker to critical
comment:28
nacin — 3 years ago
- Keywords needs-patch added; needs-testing has-patch removed
comment:29
ryan — 2 years ago
- Keywords 3.2-early added
- Milestone changed from 3.1 to Future Release
comment:31
nbachiyski — 23 months 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
nbachiyski — 23 months ago
- Cc nbachiyski added
comment:33
wonderboymusic — 4 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
ryan — 4 months ago
Yes, we can pick this up there.
comment:35
nacin — 4 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.

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?