php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Request #70638 php's unserialize() function deserialization parser issue
Submitted: 2015-10-05 03:49 UTC Modified: 2016-03-17 07:49 UTC
Votes:1
Avg. Score:5.0 ± 0.0
Reproduced:0 of 0 (0.0%)
From: taoguangchen at icloud dot com Assigned:
Status: Closed Package: *General Issues
PHP Version: Irrelevant OS: *
Private report: No CVE-ID: None
 [2015-10-05 03:49 UTC] taoguangchen at icloud dot com
Description:
------------
three years ago i had reported this bug, and until today some applications still make this mistake again.

var_unserializer.re:
```
"s:" uiv ":" ["] 	{
	...
	if (*(YYCURSOR) != '"') {
		*p = YYCURSOR;
		return 0;
	}

	YYCURSOR += 2;
	*p = YYCURSOR;

	INIT_PZVAL(*rval);
	ZVAL_STRINGL(*rval, str, len, 1);
	return 1;
}

"S:" uiv ":" ["] 	{
	...
	if (*(YYCURSOR) != '"') {
		efree(str);
		*p = YYCURSOR;
		return 0;
	}

	YYCURSOR += 2;
	*p = YYCURSOR;

	INIT_PZVAL(*rval);
	ZVAL_STRINGL(*rval, str, len, 0);
	return 1;
}
```

so this should be a invalid deserialized string can be  deserialized into a valid string-type ZVAL.

ex:
```
unserialize('s:4:"ryat"x');
```

security risks:

some applications still use unserialize() handle user input data, and use blacklist check way to fix security issue.

the latest security patch of SMF (similar fix way has also been used IPB):

```
function safe_unserialize($serialized)
{
	// Must be a string and not contain null bytes.
	if (is_string($serialized) && strpos( $serialized, "\0" ) === false)
	{
		// unserialize will only accept objects declared with O rather than o and is strict on whitespace.
		// If not found, or we found something that smelled odd but wasn't actually an object, we're good.
		if (strpos($serialized, 'O:') === false || !preg_match('~(^|;|{|})O:[+\-0-9]+:"~', $serialized))
			return @unserialize($serialized);
	}
	return false;
}
```

so an attacker can bypass this fix easily:

```
safe_unserialize('a:1:{s:4:"ryat"xO:8:"stdClass":0:{}}');
```

fix:
```
+	if (*(YYCURSOR+1) != ';') {
+		*p = YYCURSOR+1;
+		return 0;
+	}

	YYCURSOR += 2;
	*p = YYCURSOR;
```



Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-10-05 07:20 UTC] stas@php.net
-Status: Open +Status: Feedback
 [2015-10-05 07:20 UTC] stas@php.net
I'm not sure how it is security issue in PHP, could you explain? I don't know what SMF is but if it has some broken code that doesn't mean it is necessarily a security issue in PHP.
 [2015-10-05 08:12 UTC] taoguangchen at icloud dot com
-Status: Feedback +Status: Open
 [2015-10-05 08:12 UTC] taoguangchen at icloud dot com
yes you can say that it is not security issue in php but i think that at least it is bad feature or bug.

some applications write like this code should play its due role, the fact is that this code is flawed, and even lead to security problems, but is it the application's bug? php serialization parser serialized a string-type data into like `s:4:"ryat";`. anywhere in documentation don't mention that like `s:4:"ryat"x` is also an valid serialized strings. obviously this is a php bug, and this bug may cause some applications to write code with security issue.
 [2015-10-05 08:14 UTC] stas@php.net
-Type: Security +Type: Feature/Change Request
 [2016-03-17 07:49 UTC] taoguangchen at icloud dot com
-Status: Open +Status: Closed
 [2016-03-17 07:49 UTC] taoguangchen at icloud dot com
This bug has been fixed: http://git.php.net/?p=php-src.git;a=commit;h=6f241f5fad3a26810c96d5b634bfbceaeac10176
 
PHP Copyright © 2001-2021 The PHP Group
All rights reserved.
Last updated: Sun Jul 25 20:01:24 2021 UTC