php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #69425 Use After Free in unserialize()
Submitted: 2015-04-11 01:55 UTC Modified: 2017-01-16 13:29 UTC
From: taoguangchen at icloud dot com Assigned: nikic
Status: Closed Package: *General Issues
PHP Version: 5.4.39 OS:
Private report: No CVE-ID:
 [2015-04-11 01:55 UTC] taoguangchen at icloud dot com
Description:
------------
I has reported a similar bug in BUG#68976 and BUG#69133, but the fix is not perfect, you can still use a similar use-after-free attack.

var_unserializer.c
```
static inline int process_nested_data(UNSERIALIZE_PARAMETER, HashTable *ht, long elements, int objprops)
{
...
				if (zend_hash_index_find(ht, Z_LVAL_P(key), (void **)&old_data)==SUCCESS) {
					var_push_dtor(var_hash, old_data);
				}
				zend_hash_index_update(ht, Z_LVAL_P(key), &data, sizeof(data), NULL);
				break;
...

static inline int object_common2(UNSERIALIZE_PARAMETER, long elements)
{
...

	if (!process_nested_data(UNSERIALIZE_PASSTHRU, Z_OBJPROP_PP(rval), elements, 1)) {
		return 0;
	}

	if (Z_OBJCE_PP(rval) != PHP_IC_ENTRY &&
		zend_hash_exists(&Z_OBJCE_PP(rval)->function_table, "__wakeup", sizeof("__wakeup"))) {
		INIT_PZVAL(&fname);
		ZVAL_STRINGL(&fname, "__wakeup", sizeof("__wakeup") - 1, 0);
		BG(serialize_lock)++;
		call_user_function_ex(CG(function_table), rval, &fname, &retval_ptr, 0, 0, 1, NULL TSRMLS_CC);
		BG(serialize_lock)--;
	}
...
	INIT_PZVAL(*rval);

	array_init_size(*rval, elements);

	if (!process_nested_data(UNSERIALIZE_PASSTHRU, Z_ARRVAL_PP(rval), elements, 0)) {
		return 0;
	}
```

php_date.c
```
static int php_date_interval_initialize_from_hash(zval **return_value, php_interval_obj **intobj, HashTable *myht TSRMLS_DC)
{
	(*intobj)->diff = timelib_rel_time_ctor();

#define PHP_DATE_INTERVAL_READ_PROPERTY(element, member, itype, def) \
	do { \
		zval **z_arg = NULL; \
		if (zend_hash_find(myht, element, strlen(element) + 1, (void**) &z_arg) == SUCCESS) { \
			convert_to_long(*z_arg); \
			(*intobj)->diff->member = (itype)Z_LVAL_PP(z_arg); \
		} else { \
			(*intobj)->diff->member = (itype)def; \
		} \
	} while (0);

#define PHP_DATE_INTERVAL_READ_PROPERTY_I64(element, member) \
	do { \
		zval **z_arg = NULL; \
		if (zend_hash_find(myht, element, strlen(element) + 1, (void**) &z_arg) == SUCCESS) { \
			convert_to_string(*z_arg); \
			DATE_A64I((*intobj)->diff->member, Z_STRVAL_PP(z_arg)); \
		} else { \
			(*intobj)->diff->member = -1LL; \
		} \
	} while (0);
```

The convert_to_long()\convert_to_string() leads to the zval is freed from memory, and the unserialize() allow to use R: or r: to set references. So attacker can set references to the zval *rval and freed it, then zend_hash_index_find/zend_hash_index_update will use that already freed memory.

The following code should crash PHP:

via DateInterval::__wakeup():
```
$data = unserialize('a:2:{i:0;O:12:"DateInterval":1:{s:1:"y";R:1;}i:1;i:2;}');
```

via __wakeup():
```
class test
{
	var $ryat;
	
	function __wakeup()
	{
		$this->ryat = 1;
	}
}

$data = unserialize('a:2:{i:0;O:4:"test":1:{s:4:"ryat";R:1;}i:1;i:2;}');
```


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-04-28 06:30 UTC] taoguangchen at icloud dot com
Another UaF in unserialize() with DateInterval::__wakeup()/__wakeup()

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) /* {{{ */
{
	mpz_ptr gmpnum;
	const unsigned char *p, *max;
	zval zv, *zv_ptr = &zv;
	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);

	p = buf;
	max = buf + buf_len;

	INIT_ZVAL(zv);
	if (!php_var_unserialize(&zv_ptr, &p, max, &unserialize_data TSRMLS_CC)
		|| Z_TYPE_P(zv_ptr) != IS_STRING
		|| convert_to_gmp(gmpnum, zv_ptr, 10 TSRMLS_CC) == FAILURE
	) {
		zend_throw_exception(NULL, "Could not unserialize number", 0 TSRMLS_CC);
		goto exit;
	}
	zval_dtor(&zv);

	INIT_ZVAL(zv);
	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 *)
		);
	}
```

unserialize() allow to use R: or r: to set references. so attacker can set references to the zval *object and freed it, then zend_std_get_properties will use that already freed memory. it is also possible to type-onfusion attack.

the following code should crash php:

via DateInterval::__wakeup()
```
$inner = 's:5:"12345";a:1:{i:0;O:12:"DateInterval":1:{s:1:"y";R:1;}}';
unserialize('C:3:"GMP":'.strlen($inner).':{'.$inner.'}');
```

via __wakeup()
```
class test
{
	var $ryat;
	
	function __wakeup()
	{
		$this->ryat = 'AAAAAAAAAAAAAAAAAA';
	}
}

$inner = 's:5:"12345";a:1:{i:0;O:4:"test":1:{s:4:"ryat";R:1;}}';
unserialize('C:3:"GMP":'.strlen($inner).':{'.$inner.'}');
```
 [2015-04-28 06:33 UTC] stas@php.net
This appears to be a duplicate of bug #69425, could you explain what is the difference between them?
 [2015-04-28 06:33 UTC] stas@php.net
-Status: Open +Status: Feedback
 [2015-04-28 06:34 UTC] stas@php.net
-Status: Feedback +Status: Open
 [2015-04-28 06:34 UTC] stas@php.net
Oops, ignore me, it's the same one, I clicked wrong link :)
 [2015-05-13 12:58 UTC] taoguangchen at icloud dot com
the fix (test on 5.6):

diff --git a/5.6.7/ext/gmp/gmp.c b/5.6.7-fixed/ext/gmp/gmp.c
index ba59acb..65d8d8e 100644
--- a/5.6.7/ext/gmp/gmp.c
+++ b/5.6.7-fixed/ext/gmp/gmp.c
@@ -649,7 +649,7 @@ static int gmp_unserialize(zval **object, zend_class_entry *ce, const unsigned c

 	INIT_ZVAL(zv);
 	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;
diff --git a/5.6.7/ext/standard/var_unserializer.c b/5.6.7-fixed/ext/standard/var_unserializer.c
index f322ef1..9bf8747 100644
--- a/5.6.7/ext/standard/var_unserializer.c
+++ b/5.6.7-fixed/ext/standard/var_unserializer.c
@@ -293,10 +293,11 @@ static inline size_t parse_uiv(const unsigned char *p)
 #define UNSERIALIZE_PARAMETER zval **rval, const unsigned char **p, const unsigned char *max, php_unserialize_data_t *var_hash TSRMLS_DC
 #define UNSERIALIZE_PASSTHRU rval, p, max, var_hash TSRMLS_CC

-static inline int process_nested_data(UNSERIALIZE_PARAMETER, HashTable *ht, long elements, int objprops)
+static inline int process_nested_data(UNSERIALIZE_PARAMETER, long elements, int objprops)
 {
 	while (elements-- > 0) {
 		zval *key, *data, **old_data;
+		HashTable *ht;

 		ALLOC_INIT_ZVAL(key);

@@ -320,6 +321,12 @@ static inline int process_nested_data(UNSERIALIZE_PARAMETER, HashTable *ht, long
 			zval_ptr_dtor(&data);
 			return 0;
 		}
+
+		ht = HASH_OF(*rval);
+
+		if (!ht) {
+			return 0;
+		}

 		if (!objprops) {
 			switch (Z_TYPE_P(key)) {
@@ -427,7 +434,7 @@ static inline int object_common2(UNSERIALIZE_PARAMETER, long elements)
 		return 0;
 	}

-	if (!process_nested_data(UNSERIALIZE_PASSTHRU, Z_OBJPROP_PP(rval), elements, 1)) {
+	if (!process_nested_data(UNSERIALIZE_PASSTHRU, elements, 1)) {
 		return 0;
 	}

@@ -822,7 +829,7 @@ yy34:

 	array_init_size(*rval, elements);

-	if (!process_nested_data(UNSERIALIZE_PASSTHRU, Z_ARRVAL_PP(rval), elements, 0)) {
+	if (!process_nested_data(UNSERIALIZE_PASSTHRU, elements, 0)) {
 		return 0;
 	}
 [2015-05-20 12:30 UTC] taoguangchen at icloud dot com
The similar bugs exists in SPL ArrayObject/SPLObjectStorage unserialization:

spl_array.c/spl_observer.c:
```
	if (!intern->std.properties) {
		rebuild_object_properties(&intern->std);
	}
```

unserialize() allow to use R: or r: to set references. so attacker can set references to the *intern and freed it, then rebuild_object_properties will use that already freed memory.

the following code should crash php:
```
$inner = 'x:i:0;O:12:"DateInterval":1:{s:1:"y";R:1;};m:a:0:{}';
$exploit = 'C:11:"ArrayObject":'.strlen($inner).':{'.$inner.'}';
// $inner = 'x:i:1;O:12:"DateInterval":1:{s:1:"y";R:1;};m:a:0:{}';
// $exploit = 'C:11:"ArrayObject":'.strlen($inner).':{'.$inner.'}';
unserialize($exploit);
```
 [2015-08-04 23:26 UTC] taoguangchen at icloud dot com
I noticed that the latest commits in github, disable convert_to_* on DateInterval::__wakeup(), so this patch can fix part of this bug.
 [2015-09-01 06:29 UTC] stas@php.net
-Status: Open +Status: Feedback
 [2015-09-01 06:29 UTC] stas@php.net
There's a lot of code on this bug, is any of it still relevant for latest version? It's kind of hard to track when there are so many issues in one ticket.
 [2015-09-01 07:29 UTC] taoguangchen at icloud dot com
-Status: Feedback +Status: Open
 [2015-09-01 07:29 UTC] taoguangchen at icloud dot com
these codes had a lot of changes, when the latest update is released, i will check them again.
 [2015-09-02 11:27 UTC] taoguangchen at icloud dot com
these bugs can be triggered via __wakeup() in latest of php.

the fix for ArrayObject:

```
SPL_METHOD(Array, unserialize)
{
-	spl_array_object *intern = (spl_array_object*)zend_object_store_get_object(getThis() TSRMLS_CC);

+	zval *object = getThis();
+	spl_array_object *intern = (spl_array_object*)zend_object_store_get_object(object TSRMLS_CC);
...
+	if (Z_TYPE_P(object) != IS_OBJECT) {
+		return;
+	}
	
	if (!intern->std.properties) {
		rebuild_object_properties(&intern->std);
	}

```
 [2015-09-02 11:31 UTC] taoguangchen at icloud dot com
previous patch for GMP and process_nested_data is still valid.
 [2015-09-02 12:12 UTC] taoguangchen at icloud dot com
oh, the fix for arrayobject has some errors, i will check it.
 [2015-09-02 12:27 UTC] taoguangchen at icloud dot com
update fix:

```
SPL_METHOD(Array, unserialize)
{
-	spl_array_object *intern = (spl_array_object*)zend_object_store_get_object(getThis() TSRMLS_CC);

+	zval *object = getThis();
+	spl_array_object *intern = (spl_array_object*)zend_object_store_get_object(object TSRMLS_CC);

...

-		if (!php_var_unserialize(&intern->array, &p, s + buf_len, &var_hash TSRMLS_CC)) {
+		if (!php_var_unserialize(&intern->array, &p, s + buf_len, &var_hash TSRMLS_CC) || Z_TYPE_P(object) != IS_OBJECT) {

...

-	if (!php_var_unserialize(&pmembers, &p, s + buf_len, &var_hash TSRMLS_CC) || Z_TYPE_P(pmembers) != IS_ARRAY) {
+	if (!php_var_unserialize(&pmembers, &p, s + buf_len, &var_hash TSRMLS_CC) || Z_TYPE_P(pmembers) != IS_ARRAY || Z_TYPE_P(object) != IS_OBJECT) {
```
 [2015-09-28 20:00 UTC] stas@php.net
-Status: Open +Status: Feedback
 [2015-09-28 20:00 UTC] stas@php.net
I don't think it is a correct fix - the method should never be called on non-object. We can not check on every step in every object that the method is called on an object, this would make developing methods impossible. The check should happen somewhere else.
 [2015-09-28 20:02 UTC] stas@php.net
-Status: Feedback +Status: Open
 [2015-09-28 22:20 UTC] taoguangchen at icloud dot com
This type conversion only occurs in during deserialization, i don't think it need to check every step or anywhere.
 [2015-09-29 11:27 UTC] taoguangchen at icloud dot com
update better fix for ArrayObject's deserialization bug:

diff --git a/php-5.6.13/spl_array.c b/php-5.6.13-fixed/spl_array.c
index 2c65210..08751c3 100644
--- a/php-5.6.13/spl_array.c
+++ b/php-5.6.13-fixed/spl_array.c
@@ -1735,7 +1735,10 @@ SPL_METHOD(Array, serialize)
  */
 SPL_METHOD(Array, unserialize)
 {
-	spl_array_object *intern = (spl_array_object*)zend_object_store_get_object(getThis() TSRMLS_CC);
+	zval *object = getThis();
+	zval tmp = *object;
+	zval_copy_ctor(&tmp);
+	spl_array_object *intern = (spl_array_object*)zend_object_store_get_object(&tmp TSRMLS_CC);
 
 	char *buf;
 	int buf_len;
@@ -1746,15 +1749,18 @@ SPL_METHOD(Array, unserialize)
 	long flags;
 
 	if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &buf, &buf_len) == FAILURE) {
+		zval_dtor(&tmp);
 		return;
 	}
 
 	if (buf_len == 0) {
+		zval_dtor(&tmp);
 		return;
 	}
 
 	aht = spl_array_get_hash_table(intern, 0 TSRMLS_CC);
 	if (aht->nApplyCount > 0) {
+		zval_dtor(&tmp);
 		zend_error(E_WARNING, "Modification of ArrayObject during sorting is prohibited");
 		return;
 	}
@@ -1827,6 +1833,7 @@ SPL_METHOD(Array, unserialize)
 	/* done reading $serialized */
 
 	PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
+	zval_dtor(&tmp);
 	if (pflags) {
 		zval_ptr_dtor(&pflags);
 	}
@@ -1834,6 +1841,7 @@ SPL_METHOD(Array, unserialize)
 
 outexcept:
 	PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
+	zval_dtor(&tmp);
 	if (pflags) {
 		zval_ptr_dtor(&pflags);
 	}
 [2015-09-29 14:05 UTC] taoguangchen at icloud dot com
update fix for first bug:

diff --git a/php-5.6.13/var_unserializer.c b/php-5.6.13-fixed/var_unserializer.c
index 00b2f06..1f37f20 100644
--- a/php-5.6.13/var_unserializer.c
+++ b/php-5.6.13-fixed/var_unserializer.c
@@ -326,6 +326,16 @@ static inline int process_nested_data(UNSERIALIZE_PARAMETER, HashTable *ht, long
             var_push_dtor_no_addref(var_hash, &data);
 			return 0;
 		}
+		
+		if (Z_TYPE_PP(rval) != IS_ARRAY && Z_TYPE_PP(rval) != IS_OBJECT) {
+			var_push_dtor(var_hash, &data);
+        	var_push_dtor_no_addref(var_hash, &key);
+			if (elements && *(*p-1) != ';' && *(*p-1) != '}') {
+				(*p)--;
+				return 0;
+			}
+			continue;
+		}
 
 		if (!objprops) {
 			switch (Z_TYPE_P(key)) {
@@ -435,7 +445,8 @@ static inline int object_common2(UNSERIALIZE_PARAMETER, long elements)
 		return 0;
 	}
 
-	if (Z_OBJCE_PP(rval) != PHP_IC_ENTRY &&
+	if (Z_TYPE_PP(rval) == IS_OBJECT &&
+		Z_OBJCE_PP(rval) != PHP_IC_ENTRY &&
 		zend_hash_exists(&Z_OBJCE_PP(rval)->function_table, "__wakeup", sizeof("__wakeup"))) {
 		INIT_PZVAL(&fname);
 		ZVAL_STRINGL(&fname, "__wakeup", sizeof("__wakeup") - 1, 0);
 [2016-07-02 01:06 UTC] taoguangchen at icloud dot com
this patch can fix all these bugs:
```
	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-10-24 03:27 UTC] stas@php.net
As noted elsewhere, the fix above is not good since it breaks serializing references.
 [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 14:01:44 2017 UTC