WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 months 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)

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

Download all attachments as: .zip

Change History (45)

@hakre4 years ago

Add C next to a and O

comment:2 @westi4 years ago

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

@hakre4 years ago

Improved patched, added S as well.

comment:3 @hakre4 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.

comment:4 @hakre4 years ago

  • Summary changed from Serialzed option values broken for classes with Serializeable interface to Serialzed option values broken for classes and strings

comment:5 @hakre4 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

comment:6 @hakre4 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 4 years ago by hakre (previous) (diff)

@hakre4 years ago

fix for d serialized values as well, see #9930

@hakre4 years ago

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

comment:7 @nacin4 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.

comment:8 @dd324 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.

comment:9 @dd324 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.

comment:10 @westi4 years ago

  • Keywords 3.3-early added; early removed

@wonderboymusic3 years ago

comment:11 @wonderboymusic3 years ago

  • Milestone changed from Future Release to 3.6

refreshed patch against trunk due to "(Stripping trailing CRs from patch.)" caused by previous

comment:12 @markjaquith2 years ago

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

comment:13 @SergeyBiryukov2 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

comment:14 @wonderboymusic2 years ago

  • Milestone changed from Future Release to 3.7

comment:15 follow-up: @nacin2 years ago

  • Milestone changed from 3.7 to Awaiting Review

Any changes here need sign-off by the security team before continuing.

comment:16 follow-up: @hakre2 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 2 years ago by hakre (previous) (diff)

comment:17 @hakre2 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

comment:18 in reply to: ↑ 16 @rmccue2 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.

comment:19 @hakre2 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 2 years ago by hakre (previous) (diff)

comment:20 @nacin2 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.

comment:21 @hakre2 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 2 years ago by hakre (previous) (diff)

comment:23 @dd3222 months ago

#26118 was marked as a duplicate.

comment:24 @nacin20 months ago

  • Component changed from General to Options and Meta

comment:25 @SergeyBiryukov7 months ago

#31317 was marked as a duplicate.

comment:26 @jamesgol7 months ago

If this is dead in the water, would a patch to make is_serializable a pluggable function be considered?

comment:27 @slackbot7 months ago

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

comment:28 @SergeyBiryukov7 months ago

  • Summary changed from Serialzed option values broken for classes with Serializeable interface to Serialized option values broken for classes with Serializable interface

comment:29 @sc0ttkclark7 months 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?

@channeleaton6 months ago

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

comment:30 @channeleaton6 months 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 6 months ago by channeleaton (previous) (diff)

comment:31 @slackbot6 months ago

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

comment:32 @slackbot5 months ago

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

comment:33 @samuelsidler5 months ago

  • Keywords 4.3-early added

comment:34 @obenland4 months ago

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

comment:35 in reply to: ↑ 15 ; follow-up: @nacin4 months 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.*

comment:36 in reply to: ↑ 35 @channeleaton4 months 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 4 months ago by channeleaton (previous) (diff)

comment:37 @nacin3 months 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.)

comment:38 @nacin3 months ago

In 32631:

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

comment:39 @nacin3 months ago

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