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
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: nikic@php.net
New email:
PHP Version: OS:

 

 [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: Thu Nov 28 19:01:28 2024 UTC