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
 [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

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

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-2018 The PHP Group
All rights reserved.
Last updated: Sat Sep 22 01:01:26 2018 UTC