WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#11509 closed defect (bug) (fixed)

sanitize_user_object() throws fatal error on user property objects

Reported by: filosofo Owned by: ryan
Milestone: 2.9.1 Priority: normal
Severity: normal Version: 2.9
Component: Users Keywords: sanitize_user_object has-patch
Focuses: Cc:

Description

Suppose a plugin has serialized an object and saved it as user-meta data. (I'm not saying this is a good idea, but other people are doing it).

get_user_to_edit() gets the user data as an object with that now-unserialized-object as a property of the user object. Then it passes the user object to sanitize_user_object().

sanitize_user_object() currently loops through each property of the user object and if the property is not an array, it attempts to sanitize it. When the property is an object, you get a fatal error trying to cast the object to a string.

My patch, rather than excluding arrays, only attempts to sanitize things that are strings or numbers.

You're probably thinking, "if checking for objects is the problem, why not just call is_object()?" The problem is that if the serialized object is of an object type that is no longer defined, is_object() returns false. This could happen in the case in which a plugin saves the serialized object as user meta data, and then the plugin is deactivated. Then you end up with something that's a __PHP_Incomplete_Class, a non-object object.

Attachments (2)

sanitize_user_field-check.11509.diff (618 bytes) - added by filosofo 5 years ago.
11509.2.patch (810 bytes) - added by hakre 5 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 @filosofo5 years ago

This is one cause of the white screen of death for some people on their 2.9 profile pages.

comment:2 @filosofo5 years ago

  • Keywords has-patch added

comment:3 @hakre5 years ago

  • Milestone changed from 3.0 to 2.9.1

That would need two checks, the array variant of the user might have such objects as well, hasn't it? (Line 650 ca.)

Addtionally the function "sanitize_user_object" has the problem that it does not return a user - if object - by reference as it is common practise in wordpress. But that's only a sidenote.

the function sanitize_user_object accepts by definition object related values only. this might be a documentation problem. if not, this should be properly reflected in function sanitize_user_object. sanitize_user_object is misleading as well because according to it's code and docblock, it accepts a user array as well.

sanitize_user_field renders useless in the raw context. It seems pretty useless that it is implemented then.

line 692 in sanitize_user_field() looks like the counterpart that you fix with your patch. it should be updated as well.

docblock of function sanitize_user_field is pretty redundant to the code, the @uses apply_filters() notes are pretty useless. better formatted php code does help a lot more than double code, one time in comments with pseudo code and second time in the function with real code.

for the line 692 topic I've created a second patch. I would not recommend this for 3.0 because as filosofo pointed out this actually breaks things.

comment:4 in reply to: ↑ description ; follow-up: @hakre5 years ago

Replying to filosofo:

You're probably thinking, "if checking for objects is the problem, why not just call is_object()?" The problem is that if the serialized object is of an object type that is no longer defined, is_object() returns false.

is_object() does not unserialize a value. it will return false. it will only return true on real (not serialized) objects.

comment:5 in reply to: ↑ 4 ; follow-up: @filosofo5 years ago

Replying to hakre:

is_object() does not unserialize a value. it will return false. it will only return true on real (not serialized) objects.

But in the case I mentioned the "object" is unserialized as an __PHP_Incomplete_Class object. It's not just a string of serialized data.

comment:6 @filosofo5 years ago

hakre, your patch has a typo: "retirm"

@hakre5 years ago

comment:7 in reply to: ↑ 5 @hakre5 years ago

Replying to filosofo:

Replying to hakre:

is_object() does not unserialize a value. it will return false. it will only return true on real (not serialized) objects.

But in the case I mentioned the "object" is unserialized as an __PHP_Incomplete_Class object. It's not just a string of serialized data.

Ah okay, I was not aware of that. I guess that's a nice case which shows where usage makes sense with user options. I suggest to base64_encode serialized values to protect their integrity within wordpress.

Replying to filosofo:

hakre, your patch has a typo: "retirm"

Thanks for reporting, fixed.

comment:8 @Denis-de-Bernardy5 years ago

see also #9640 and related tickets

comment:9 @ryan5 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [12511]) Sanitize only string and numeric fields in the user object. Props filosofo hakre. fixes #11509 for trunk

comment:10 @ryan5 years ago

[12512] for 2.9.

Note: See TracTickets for help on using tickets.