php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #72610 unserialize() read-after-free when property_table is reallocated
Submitted: 2016-07-17 18:12 UTC Modified: 2017-01-16 11:37 UTC
Votes:1
Avg. Score:1.0 ± 0.0
Reproduced:1 of 1 (100.0%)
Same Version:1 (100.0%)
Same OS:1 (100.0%)
From: tandre at ifwe dot co Assigned: dmitry (profile)
Status: Closed Package: *General Issues
PHP Version: 7.0.8 OS: All
Private report: No CVE-ID: None
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: tandre at ifwe dot co
New email:
PHP Version: OS:

 

 [2016-07-17 18:12 UTC] tandre at ifwe dot co
Description:
------------
This affects all versions of phpPHP 7.0.0 to PHP 7.1-alpha3

Running the linked 3v4l test script in php 7 will result in the error "Notice: unserialize(): Error at offset 100 of 102 bytes in /in/1SsOJ on line 28"

Versions from php 7.0.0 to php 7.0.2 are affected slightly differently.

Additionally, Running the test script with the bash command `USE_ZEND_ALLOC=0 valgrind php that_file.php` will reveal multiple invalid memory reads of already freed data. See https://pastee.org/tjzp8

I'm not sure if this falls under security, change the bug type if it doesn't. (If new objects are allocated, I assume they may overlap with the invalid pointers)

Cause:
See ext/standard/var_unserialize.re
The problem is that the var_entries struct contains pointers to `zval`s in the object property_table if that property is dynamic (e.g. no declaration in the class such as `public $a`). 
When the property_table is expanded by realloc(), those pointers usually become invalid.

Possible fixes (not sure if these will work)

- keep a list of copies of those values (instead of pointers) in var_entries (list of `zval` instead of `zval*`) in struct var_entries, and temporarily increment refcount of underlying objects/arrays?
- Defer calls to __wakeup() until after all properties were set up, and perform those calls in the same order they originally would have. This may cause different behavior when unserializing.

https://bugs.php.net/bug.php?id=69295 may or may not be affected by the fix to this bug

Test script:
---------------
https://3v4l.org/DkcB5

Expected result:
----------------
The program runs without reading free()d/realloc()ed memory. It has the below output:

a:2:{i:0;O:3:"Obj":1:{s:1:"a";O:8:"stdClass":1:{s:4:"test";s:3:"foo";}}i:1;O:3:"Obj":1:{s:1:"a";r:3;}}
Called __unserialize
array(2) {
  [0]=>
  object(Obj)#4 (1) {
    ["a"]=>
    object(stdClass)#5 (1) {
      ["test"]=>
      string(3) "foo"
    }
  }
  [1]=>
  object(Obj)#6 (1) {
    ["a"]=>
    object(stdClass)#5 (1) {
      ["test"]=>
      string(3) "foo"
    }
  }
}

Actual result:
--------------
unserialize performs invalid memory reads, then returns false

a:2:{i:0;O:3:"Obj":1:{s:1:"a";O:8:"stdClass":1:{s:4:"test";s:3:"foo";}}i:1;O:3:"Obj":1:{s:1:"a";r:3;}}

Notice: unserialize(): Error at offset 100 of 102 bytes in /in/DkcB5 on line 20
Called __unserialize
Notice: Trying to get property of non-object in /in/DkcB5 on line 24
Fail 0 b0

Patches

Pull Requests

Pull requests:

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2016-07-17 20:30 UTC] stas@php.net
-Summary: unserialize() +Summary: unserialize() read-after-free when property_table is reallocated
 [2016-07-18 00:43 UTC] stas@php.net
-Type: Security +Type: Bug -Assigned To: +Assigned To: dmitry
 [2016-07-18 00:43 UTC] stas@php.net
Doesn't look like security issue, requires a lot of specialized code.
 [2016-07-18 02:18 UTC] tandre at ifwe dot co
It seems like hhvm handles __wakeup in the same way as the second option I mentioned:

https://github.com/facebook/hhvm/blob/2d5f00afbb033aec0cbc51cbe3a897af79cdcb28/hphp/runtime/base/variable-unserializer.cpp#L333
https://github.com/facebook/hhvm/blob/2d5f00afbb033aec0cbc51cbe3a897af79cdcb28/hphp/runtime/base/variable-unserializer.cpp#L965

igbinary7 had a similar issue, and a (possibly incomplete) fix for the similar issue was https://github.com/igbinary/igbinary7/pull/17/files

(The NG engine might require slightly different adjustments)
 [2016-07-18 17:55 UTC] tandre at ifwe dot co
I created a PR to fix this issue: https://github.com/php/php-src/pull/2004 . Comments and any test cases that you think I should add are welcome.

Also, never mind, https://bugs.php.net/bug.php?id=69295 should be unaffected by the fix, it won't call __wakeup.
 [2017-01-16 11:37 UTC] nikic@php.net
-Status: Assigned +Status: Closed
 [2017-01-16 11:37 UTC] nikic@php.net
This has been fixed by https://github.com/php/php-src/commit/0426b916df396a23e5c34514e4f2f0627efdcdf0. The change is in 5.6+, even though the property table issue only affects PHP 7, because this also prevents a wide range of other __wakeup() based attacks.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Thu Dec 05 22:01:29 2024 UTC