Opened 13 years ago
Closed 9 years ago
#17375 closed defect (bug) (wontfix)
Serialized option values broken for classes with Serializable interface
Reported by: | hakre | Owned by: | markjaquith |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | 2.0.5 |
Component: | Options, Meta APIs | Keywords: | close |
Focuses: | Cc: |
Description
Wordpress has a feature build in to store arrays and objects into option values.
This is done by transparently serializing on storing and unserialize on retrieving.
The implementation is broken.
Wordpress is unable to unserialize serialized option values if they contain a serialized representation of classes implementing the PHP serializeable interface (C instead of O).
This is because the is_serialized() does not recorgnize that type of value.
Attachments (6)
Change History (45)
#2
@
13 years ago
- Milestone changed from Awaiting Review to 3.2
- Owner set to markjaquith
- Status changed from new to reviewing
#3
@
13 years ago
I've reviewed the code of yesterday now.
It lacked the second missing serialized identifier: S.
It has been added, as well for the is_serialized_string because it represents a serialized string.
#4
@
13 years ago
- Summary changed from Serialzed option values broken for classes with Serializeable interface to Serialzed option values broken for classes and strings
#5
@
13 years ago
- Summary changed from Serialzed option values broken for classes and strings to Serialzed option values broken for classes and strings on unserialize for C and S
#6
@
13 years ago
#7
@
13 years ago
#9930 is pretty messy. Should probably concentrate work on float detection there, given the existing functional tests there.
What's 'S'? Can't find a reference to it anywhere.
#8
@
13 years ago
went on a hunt for the S modifier.
Lead me to this changeset in php: phprev 225029 which suggests it was for PHP6(5.3?) future compatibility, which then lead to phprev 232476, which finally gives us a security report about the 'S' modifer.
With PHP 5.2.1 the new S: data type was added to unserialize(). It is meant as compatibility layer for exchange of serialized data with future PHP 6. The data type itself is similar to the normal s: string data type with the exception that simple escaped bytes are supported. The following string is an example.
With PHP6 never being released, AFAIK, there are no cases where the S data type should be created by PHP, looking through PHP 5.3's branch's var.c seems to validate that.
#9
@
13 years ago
- Keywords early added
- Milestone changed from 3.2 to Future Release
I'm not comfortable with changing any regex in this function this close to a release, not for something which is called as much as this is.
After a quick chat in #wordpress-dev, I'm punting this to the next release where we can do this and #9930 at the same time.
#11
@
12 years ago
- Milestone changed from Future Release to 3.6
refreshed patch against trunk due to "(Stripping trailing CRs from patch.)" caused by previous
#12
@
11 years ago
- Keywords early added; 3.3-early removed
- Milestone changed from 3.6 to Future Release
#13
@
11 years ago
- Summary changed from Serialzed option values broken for classes and strings on unserialize for C and S to Serialized option values broken for classes and strings on unserialize for C and S
#15
follow-up:
↓ 35
@
11 years ago
- Milestone changed from 3.7 to Awaiting Review
Any changes here need sign-off by the security team before continuing.
#16
follow-up:
↓ 18
@
11 years ago
@nacin: Have you asked the team to sign-off this and if not can you please do so? Or does the ticket owner need to ask that team to sign that off? Anyway just thinking now you mean security@wordpress.org
as the Security Team so I mailed them.
The core part of this issue (C - http://www.php.net/class.serializable) is as of PHP 5.1.0.
#17
@
11 years ago
- Summary changed from Serialized option values broken for classes and strings on unserialize for C and S to Serialzed option values broken for classes with Serializeable interface
#18
in reply to:
↑ 16
@
11 years ago
Replying to hakre:
@nacin: Have you asked the team to sign-off this and if not can you please do so? Or does the ticket owner need to ask that team to sign that off? Anyway just thinking now you mean
security@wordpress.org
as the Security Team so I mailed them.
I believe by that nacin means that they're discussing it at the moment and will get back to the ticket as soon as they've decided.
#19
@
11 years ago
Yes something is in traction with that function. Just seeing incomming changes,
Improve clarity and speed of [25320] (merged as [25325]). Merges [25338] to 3.6.
However this seems a bit misleading again as PHP core allows to add data after the serialized structure. Also it now seems that in the heat of whatever this Charlie Foxtrot is part of, long-time reported flaws on the routine are not referenced any longer, ticket is completely missing.
#20
@
11 years ago
Okay hakre, that's enough. At this point, you've emailed security@…. If you want to curse and rant, you can do so via email.
#21
@
11 years ago
@Nacin as if it was the tone. Looks like my nose wasn't so bad. And as you're member of that list, why don't you respond there?
#22
@
11 years ago
#26
@
10 years ago
If this is dead in the water, would a patch to make is_serializable a pluggable function be considered?
This ticket was mentioned in Slack in #core by sc0ttkclark. View the logs.
10 years ago
#28
@
10 years ago
- Summary changed from Serialzed option values broken for classes with Serializeable interface to Serialized option values broken for classes with Serializable interface
#29
@
10 years ago
- Keywords dev-feedback added; early removed
- Version changed from 3.1 to 2.0.5
Let's backtrack this ticket back to where it started to keep it simple.
Let's focus on adding 'C' as a supported type and the security implications that may involve.
#31317 has a good and simple patch for this and we can easily add a unit test for it.
Assuming the test is done, what then holds this ticket back from moving forward?
#30
@
10 years ago
I hope the patch I added will satisfy the requirements. It only focuses on adding the "C" type key and includes a unit test as suggested by @helen in Slack on Feb 12.
A class was added in tests/phpunit/includes/functions.php
in order to implement Serializable
. I assume this is the correct location.
This ticket was mentioned in Slack in #core by channeleaton. View the logs.
10 years ago
This ticket was mentioned in Slack in #core by channeleaton. View the logs.
10 years ago
#35
in reply to:
↑ 15
;
follow-up:
↓ 36
@
9 years ago
- Keywords close added; has-patch dev-feedback removed
Replying to nacin:
Any changes here need sign-off by the security team before continuing.
I am almost positive we cannot make this change without directly adding an arbitrary code execution vulnerability.
*DO NOT COMMIT UNDER ANY CIRCUMSTANCES.*
#36
in reply to:
↑ 35
@
9 years ago
Replying to nacin:
I am almost positive we cannot make this change without directly adding an arbitrary code execution vulnerability.
I'm just trying to understand what's going on with the change. Is it the regex that creates the vulnerability? If not, we're basically just adding another key by which is_serialized()
will return true. If normal objects are already processed as true through this function, is the vulnerability not already present?
#37
@
9 years ago
I made this comment while preparing for a talk (see https://poststatus.com/the-trojan-emoji/), so please pardon the quick drive-by originally.
Here's some overall details on PHP object injection: https://www.owasp.org/index.php/PHP_Object_Injection.
I'll try to explain the vulnerability here succinctly:
- If the user can arbitrarily pass a string to
unserialize()
, then it can be an object injection vulnerability which could lead to any number of other issues, such as arbitrary code execution. This is explained in the above link. - Right now, if the user tries to insert a serialized string (with the hope that it is unserialized), we serialize it a second time. See
maybe_serialize()
callingis_serialized()
. - Right now, someone can insert the string
C:16:"Serialized_Class":50:{a:3:{i:0;s:3:"one";i:1;s:3:"two";i:2;s:5:"three";}}
. It will not passis_serialized()
, which means it will be treated as a simple scalar that can be inserted into the DB. - If we add this patch to a future WordPress version, then any existing
C:
strings will suddenly be unserializable. See the sameis_serialized()
inmaybe_unserialize()
. Thus, we would be turning a harmless string (on original insert) into a sleeping exploit waiting to be unserialized (on a future select).
Thus, is_serialized()
is frozen in time. (There could be worse things in life.)
Add C next to a and O