Make WordPress Core

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's profile hakre Owned by: markjaquith's profile 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)

17375.patch (392 bytes) - added by hakre 13 years ago.
Add C next to a and O
17375.2.patch (727 bytes) - added by hakre 13 years ago.
Improved patched, added S as well.
17375.3.patch (964 bytes) - added by hakre 13 years ago.
fix for d serialized values as well, see #9930
17375.4.patch (1.0 KB) - added by hakre 13 years ago.
Plus sign is available for uiv/length specifier as well.
17375.diff (1.6 KB) - added by wonderboymusic 11 years ago.
17375.5.patch (1.6 KB) - added by channeleaton 9 years ago.
Only adds checks for "C"key. Includes unit test.

Download all attachments as: .zip

Change History (45)

@hakre
13 years ago

Add C next to a and O

#2 @westi
13 years ago

  • Milestone changed from Awaiting Review to 3.2
  • Owner set to markjaquith
  • Status changed from new to reviewing

@hakre
13 years ago

Improved patched, added S as well.

#3 @hakre
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 @hakre
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 @hakre
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 @hakre
13 years ago

Detection on serialized float values is broken as well. There already exists a ticket for it: #9930 - I've uploaded a new patch there against current trunk. Will merge it in here, too.

Related: #17445

Last edited 13 years ago by hakre (previous) (diff)

@hakre
13 years ago

fix for d serialized values as well, see #9930

@hakre
13 years ago

Plus sign is available for uiv/length specifier as well.

#7 @nacin
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 @dd32
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 @dd32
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.

#10 @westi
13 years ago

  • Keywords 3.3-early added; early removed

#11 @wonderboymusic
11 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 @markjaquith
11 years ago

  • Keywords early added; 3.3-early removed
  • Milestone changed from 3.6 to Future Release

#13 @SergeyBiryukov
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

#14 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.7

#15 follow-up: @nacin
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: @hakre
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.

Last edited 11 years ago by hakre (previous) (diff)

#17 @hakre
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 @rmccue
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 @hakre
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.

Last edited 11 years ago by hakre (previous) (diff)

#20 @nacin
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 @hakre
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?

Last edited 11 years ago by hakre (previous) (diff)

#23 @dd32
10 years ago

#26118 was marked as a duplicate.

#24 @nacin
10 years ago

  • Component changed from General to Options and Meta

#25 @SergeyBiryukov
9 years ago

#31317 was marked as a duplicate.

#26 @jamesgol
9 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.


9 years ago

#28 @SergeyBiryukov
9 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 @sc0ttkclark
9 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?

@channeleaton
9 years ago

Only adds checks for "C"key. Includes unit test.

#30 @channeleaton
9 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.

Last edited 9 years ago by channeleaton (previous) (diff)

This ticket was mentioned in Slack in #core by channeleaton. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by channeleaton. View the logs.


9 years ago

#33 @samuelsidler
9 years ago

  • Keywords 4.3-early added

#34 @obenland
9 years ago

  • Keywords 4.3-early removed
  • Milestone changed from Awaiting Review to 4.3

#35 in reply to: ↑ 15 ; follow-up: @nacin
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 @channeleaton
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?

Last edited 9 years ago by channeleaton (previous) (diff)

#37 @nacin
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() calling is_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 pass is_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 same is_serialized() in maybe_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.)

#38 @nacin
9 years ago

In 32631:

Add unit test asserting that serializable objects will never pass is_serialized(). see #17375.

#39 @nacin
9 years ago

  • Milestone 4.3 deleted
  • Resolution set to wontfix
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.