Req #70638 [Opn->Csd]: php's unserialize() function deserialization parser issue

Edit report at https://bugs.php.net/bug.php?id=70638&edit=1 ID: 70638 User updated by: taoguangchen at icloud dot com Reported by: taoguangchen at icloud dot com Summary: php's unserialize() function deserialization parser issue -Status: Open +Status: Closed Type: Feature/Change Request Package: *General Issues Operating System: * PHP Version: Irrelevant Block user comment: N Private report: N New Comment: This bug has been fixed: http://git.php.net/?p=php-src.git;a=commit;h=6f241f5fad3a26810c96d5b634bfbceaeac10176 Previous Comments: ------------------------------------------------------------------------ [2015-10-05 08:12:07] 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 07:20:36] 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 03:49:20] 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; ``` ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=70638&edit=1