Make WordPress Core

Opened 8 years ago

Last modified 5 years ago

#34392 new defect (bug)

calling update_option("siteurl", $newval) can overwrite it with an instance of WP_Error.

Reported by: pbogdan's profile pbogdan Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.3.1
Component: General Keywords:
Focuses: Cc:

Description

Hello,

under admittedly somewhat obscure circumstances a call to
update_option("siteurl", ...) can overwrite its value with an instance of
WP_Error class.

When being updated siteurl is subject to some additional checks in
sanitize_option() function, and the bug is due its handling of WP_Error's.

In particular the code at
https://github.com/WordPress/WordPress/blob/4.3.1/wp-includes/formatting.php#L3609-L3612:

  • overwrites the $value variable with result of the call to $wpdb->strip_invalid_text_for_column() -
  • will only create the $error variable if the WP_Error received from the call to $wpdb->strip_invalid_text_for_column() contains the optional error message

If $wpdb->strip_invalid_text_for_column() returns an instance of WP_Error
without an error message we end up with $value containing a WP_Error instance
but empty $error variable.

Then the check at
https://github.com/WordPress/WordPress/blob/4.3.1/wp-includes/formatting.php#L3720
fails due to empty $error, and the WP_Error $value propagates upwards as the
return result of sanitize_option(), to be written into the database. As most
places trust get_option("siteurl") to return a string rather than an object this
results in fatal errors as WP_Error can't be converted to a string.

In the specific case I observed, the WP_Error instance without an error message
was originating from wpdb::get_table_charset() function -
https://github.com/WordPress/WordPress/blob/4.3.1/wp-includes/wp-db.php#L2306-L2308. From
what I gather wpdb::strip_invalid_text_for_column() needs to know the database
charset for the database column to determine valid input, this will eventually
call out to wpdb::get_table_charset() and if the query in that function fails
for some reason the resulting WP_Error propagates all the way up to
sanitize_option().

As this was tracked down retrospectively I don't know what exact conditions
allowed the query in wpdb::get_table_charset() to fail while allowing any
subsequent queries to succeed, however it can be reproduced with a test case at
https://gist.github.com/pbogdan/9827a8f2358cf9e8ca71

I have not reviewed the logic for other options handled by sanitize_option()
but it's possible some of them could be affected if they use the same logic for
dealing with WP_Error's - which relies on the WP_Error having the optional error
message.

I might be bit slow to respond but let me know if there is any additional
information I can provide.

Thanks,
Piotr

Attachments (1)

34392.diff (639 bytes) - added by MikeHansenMe 8 years ago.

Download all attachments as: .zip

Change History (3)

@MikeHansenMe
8 years ago

#1 @MikeHansenMe
8 years ago

  • Keywords has-patch added

#2 @MikeHansenMe
8 years ago

  • Keywords has-patch removed

Actually that won't work either because the $value still gets assigned the error and gets set in the option.

Note: See TracTickets for help on using tickets.