WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 4 days ago

#17375 reviewing defect (bug)

Serialized option values broken for classes with Serializable interface

Reported by: hakre Owned by: markjaquith
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 2.0.5
Component: Options, Meta APIs Keywords: has-patch dev-feedback 4.3-early
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 2 years ago.
17375.5.patch (1.6 KB) - added by channeleaton 12 days ago.
Only adds checks for "C"key. Includes unit test.

Download all attachments as: .zip

Change History (39)

@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

@wonderboymusic2 years ago

comment:11 @wonderboymusic2 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 @markjaquith21 months ago

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

comment:13 @SergeyBiryukov21 months 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 @wonderboymusic20 months ago

  • Milestone changed from Future Release to 3.7

comment:15 @nacin19 months 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: @hakre19 months 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 19 months ago by hakre (previous) (diff)

comment:17 @hakre19 months 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 @rmccue19 months 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 @hakre19 months 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 19 months ago by hakre (previous) (diff)

comment:20 @nacin19 months 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 @hakre19 months 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 19 months ago by hakre (previous) (diff)

comment:23 @dd3216 months ago

#26118 was marked as a duplicate.

comment:24 @nacin14 months ago

  • Component changed from General to Options and Meta

comment:25 @SergeyBiryukov6 weeks ago

#31317 was marked as a duplicate.

comment:26 @jamesgol6 weeks ago

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

comment:27 @slackbot6 weeks ago

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

comment:28 @SergeyBiryukov6 weeks 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 @sc0ttkclark6 weeks 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?

@channeleaton12 days ago

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

comment:30 @channeleaton12 days 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 12 days ago by channeleaton (previous) (diff)

comment:31 @slackbot12 days ago

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

comment:32 @slackbot4 days ago

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

comment:33 @samuelsidler4 days ago

  • Keywords 4.3-early added
Note: See TracTickets for help on using tickets.