WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#24023 closed defect (bug) (fixed)

3.5/wp-includes/functions.php : missing break statement

Reported by: tivnet Owned by: helen
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.5.1
Component: Inline Docs Keywords: needs-patch
Focuses: Cc:

Description

Line 263:

switch ( $token ) {
case 's' :
if ( '"' !== $data[$length-2] )
return false;
case 'a' :

Need "break" after "return false", because return happens only if condition met. Otherwise - falls to case 'a'

Attachments (1)

24023.test.php (1.2 KB) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 @dd322 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

This is by design, The check there applies to 's' only, the following check applies to 's', 'a' and, 'O'

comment:2 follow-up: @nacin2 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Let's comment this with // Fall through so others know it is deliberate.

comment:3 follow-up: @tivnet2 years ago

The entire method can be changed to attempt to unserialize with catching the issued E_NOTICE.

comment:4 in reply to: ↑ 3 ; follow-ups: @nacin2 years ago

Replying to tivnet:

The entire method can be changed to attempt to unserialize with catching the issued E_NOTICE.

This method is written for speed. It's faster to reach into the string than attempt to unserialize. It's also why we use string functions rather than starting with regular expressions.

You can't really catch an E_NOTICE, not without a custom error handler, which in turn is even slower.

comment:5 in reply to: ↑ 4 @tivnet2 years ago

Replying to nacin:
Just wrote a little test. Do not need to catch E_NOTICE.

error_reporting(E_ALL);

var_dump(_is_serialized('a'));
var_dump(_is_serialized(serialize('a')));
var_dump(_is_serialized(false));
var_dump(_is_serialized(serialize(false)));
var_dump(_is_serialized(true));
var_dump(_is_serialized(serialize(true)));
var_dump(_is_serialized(array(1,'2',3, true)));
var_dump(_is_serialized(serialize(array(1,'2',3, true))));
var_dump(_is_serialized(new stdClass()));
var_dump(_is_serialized(serialize(new stdClass())));

function _is_serialized($data) {
	if(!is_string($data)){ // this is not required
		return false;
	}
	if ($data === "b:0;") {
		return true;
	}
	return (@unserialize($data) !== false);
}

@SergeyBiryukov2 years ago

comment:6 in reply to: ↑ 4 ; follow-up: @SergeyBiryukov2 years ago

  • Milestone set to Awaiting Review
  • Severity changed from trivial to normal

Replying to nacin:

This method is written for speed. It's faster to reach into the string than attempt to unserialize.

Performed a quick test with 1,000,000 iterations of is_serialized() and _is_serialized() from comment:5: 24023.test.php.

Turned out that _is_serialized() is actually 20% faster:

is_serialized(): 52,366 seconds
_is_serialized(): 40,198 seconds

Removing the is_string() check (marked as "this is not required") makes both results almost equal though:

is_serialized(): 52,154 seconds
_is_serialized(): 51,392 seconds

Switching to unserialize() would fix #18007 as well.

comment:7 @tivnet2 years ago

That's right Sergey, I added is_string to speed up.
Even if there was no speed improvement, the code would be tiny and clean.
Thank you!

Version 0, edited 2 years ago by tivnet (next)

comment:8 in reply to: ↑ 6 @tivnet2 years ago

Replying to SergeyBiryukov:

In 3.6.1, additional code is added to this function, making it even more "tricky".

Reconsider my tiny function instead?

comment:9 @SergeyBiryukov2 years ago

As noted in ticket:17375:15, any change to is_serialized() needs to be reviewed by the security team.

comment:10 in reply to: ↑ 2 ; follow-up: @dd322 years ago

Replying to nacin:

Let's comment this with // Fall through so others know it is deliberate.

That's the only valid part of this ticket.

For security purposes, we cannot attempt to run unserialize() on untrusted data, so lets add a comment and move on.
For a explanation of why we have is_serialized(), and why it doesn't run on untrusted data, the 3.6.1 changelog is the most recent thing I can point to:

  • Remote Code Execution: Block unsafe PHP de-serialization that could occur in limited situations and setups, which can lead to remote code execution. Reported by Tom Van Goethem. CVE-2013-4338.

Any change along the lines suggested in this ticket will undo the fixes put in place for that, as well as potentially making it easier than before to exploit.

comment:11 in reply to: ↑ 10 @tivnet2 years ago

Replying to dd32:

Replying to nacin:

Let's comment this with // Fall through so others know it is deliberate.

That's the only valid part of this ticket.

For security purposes, we cannot attempt to run unserialize() on untrusted data, so lets add a comment and move on.

Understood. There is a __wakeup thing which I did not consider.
Thanks. Let's move on.

comment:12 @helen2 years ago

  • Component changed from General to Inline Docs
  • Milestone changed from Awaiting Review to 3.7

comment:13 @helen2 years ago

  • Owner set to helen
  • Resolution set to fixed
  • Status changed from reopened to closed

In 25371:

Indicate that the fall-through in is_serialized() is deliberate. fixes #24023.

Note: See TracTickets for help on using tickets.