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

go to bug id or search bugs for

Bug #72785 allowed_classes only applies to outermost unserialize()
Submitted: 2016-08-08 16:36 UTC Modified: 2016-09-06 11:04 UTC
From: nikic@php.net Assigned: stas (profile)
Status: Closed Package: Scripting Engine problem
PHP Version: 7.0.9 OS:
Private report: No CVE-ID: None
View Developer Edit
 [2016-08-08 16:36 UTC] nikic@php.net 
Description:
------------
When unserialize() is used with the allowed_classes option, the allowed classes are only enforced in the outer-most unserialize call. Nested unserialize() calls in the same serialization context do not enforce allowed classes.

In practice this means that if allowed_classes contains any class using C-style serialization, then the restriction becomes ineffective. The test script shows this using ArrayObject.



Test script:
---------------
<?php

// Forbidden class
class A {}

$p = 'x:i:0;a:1:{i:0;O:1:"A":0:{}};m:a:0:{}';
$s = 'C:11:"ArrayObject":' . strlen($p) . ':{' . $p . '}';
var_dump(unserialize($s, ['allowed_classes' => 'ArrayObject']));

Expected result:
----------------
object(A) should be an incomplete class instead.

Actual result:
--------------
object(ArrayObject)#1 (1) {
  ["storage":"ArrayObject":private]=>
  array(1) {
    [0]=>
    object(A)#2 (0) {
    }
  }
}

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2016-08-10 06:19 UTC] stas@php.net 
Yeah that's a problem for nested unserialize's that use internal container classes... I'm not sure it's possible to fix it without adding arguments or globals (i.e. BC break).
 [2016-08-12 06:53 UTC] stas@php.net
-PHP Version: 7.0.9 +PHP Version: 5.6.24
 [2016-08-13 19:36 UTC] stas@php.net
-PHP Version: 5.6.24 +PHP Version: 7.0.9
 [2016-08-13 20:32 UTC] stas@php.net 
BTW your example code has a bug - allowed_classes value should be an array. We probably need to add notice or something on this...
 [2016-08-14 01:22 UTC] stas@php.net 
Proposed fix https://gist.github.com/360b34c65bcb11f917bbef4ba29acfd3

It requires slight BC break, so I am not sure but do not see better alternative.
 [2016-08-15 21:49 UTC] nikic@php.net 
Looks reasonable. Notes:
* The new type check needs a PHP_VAR_UNSERIALIZE_DESTROY.
* As this is PHP 7.1-only anyway, maybe we could drop php_var_unserialize_ex and make the unserialize_data the canonical source of truth about allowed classes?

Variant with these changes: https://gist.github.com/nikic/c7941621d3a083f84d05d46c1cef35bd
 [2016-09-06 01:23 UTC] stas@php.net 
The fix still doesn't seem to be complete, since we don't pass it when we parse keys. Maybe we should ban objects completely when we are parsing keys.
 [2016-09-06 02:58 UTC] stas@php.net
-Type: Security +Type: Bug
 [2016-09-06 02:58 UTC] stas@php.net 
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=747d21cfd2a7414b8d5ace203524f61eab2b8323
Log: Fix bug #72785 - allowed_classes only applies to outermost unserialize()
 [2016-09-06 02:58 UTC] stas@php.net
-Status: Open +Status: Closed
 [2016-09-06 03:36 UTC] stas@php.net
-Assigned To: +Assigned To: stas
 [2016-09-06 03:36 UTC] stas@php.net 
Ah, my last comment was wrong, it's already checked.
 [2016-09-06 11:04 UTC] nikic@php.net 
Followup fix in https://github.com/php/php-src/commit/09f7bb2082067baab56fe0f04a6531beef253d95 - I made a mistake in the previous patch when handling nesting, it did not reset allowed_classes to the old value after a nested unserialize with different allowed classes.
 [2016-10-17 10:08 UTC] bwoebi@php.net 
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=747d21cfd2a7414b8d5ace203524f61eab2b8323
Log: Fix bug #72785 - allowed_classes only applies to outermost unserialize()
 [2016-12-30 01:00 UTC] cmb@php.net 
Related To: Bug #73836
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved. 		Last updated: Mon Nov 25 00:01:33 2024 UTC