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

[tivnet]

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'

[dd32]

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

[nacin]

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

[tivnet]

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

[nacin]

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.

[tivnet]

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);
}

[SergeyBiryukov]

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.

[tivnet]

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!

[tivnet]

Replying to SergeyBiryukov:

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

Reconsider my tiny function instead?

[SergeyBiryukov]

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

[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.
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.

[tivnet]

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.

[helen]

In 25371:

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