[PHP-DEV] Re: Fix for unserialise() "vulnerabilities"

[Dmitry Stogov]

On Oct 12, 2017 6:01 PM, Nikita Popov  wrote:
On Thu, Oct 12, 2017 at 4:38 PM, Dmitry Stogov 
> wrote:

Hi,


I've found, that at least half of unserialise() security problems, occurs 
because of non-symmetric serialize/unserialize assumption, regarding references 
encoded with "r".


serialize() assumes it's an object.


https://github.com/php/php-src/blob/master/ext/standard/var.c#L828


universalize() allows any value.


https://github.com/php/php-src/blob/master/ext/standard/var_unserializer.re#L677


This allows manual crafting of strings that may lead to creation of unexpected 
data structures.

I propose to fix this just by fixing the symmetry.


https://gist.github.com/dstogov/53382540bdfee7b6c7dadf142dc437ed


This will prohibit, some manually crafted strings.

Of course, this will break few "security" related tests. Especially:


> Bug #70284 (Use after free vulnerability in unserialize() with GMP) 
> [ext/gmp/tests/bug70284.phpt]
> Bug #70211 (php 7 ZEND_HASH_IF_FULL_DO_RESIZE use after free) 
> [ext/soap/tests/bug70211.phpt]
> Bug #70172 - Use After Free Vulnerability in unserialize() 
> [ext/standard/tests/serialize/bug70172.phpt]
> Bug #70963 (Unserialize shows UNKNOW in result) 
> [ext/standard/tests/serialize/bug70963.phpt]
> Memleaks if unserialize return a self-referenced array/object 
> [ext/standard/tests/serialize/unserialize_mem_leak.phpt]
> Bug #72433: Use After Free Vulnerability in PHP's GC algorithm and 
> unserialize [ext/standard/tests/strings/bug72433.phpt]

Any objections? (this is for master only of course)

Hi,

I don't think this will really fix any vulnerabilities, because the core issue 
are R references, not r references. If this prevents a vulnerability using r, 
you can usually replicate something similar using R instead.

However, I still agree that it makes sense to restrict this. Especially because 
unserialize() currently allows creating structures that are just impossible in 
plain PHP, such as cyclic arrays without use of references (GLOBALS 
notwithstanding).

The check looks too strict to me though. Shouldn't it first DEREF the value 
before performing the OBJECT check? (E.g. for something like 
"a:3:{i:0;O:8:"stdClass":0:{}i:1;R:2;i:2;r:2;}", in which case r:2 will be a 
REF to OBJECT).

Thanks, for catching. You are right. I'll fix the patch a bit later. Just add 
DEREF.

Dmitry.


Regards,
Nikita

[PHP-DEV] Re: Fix for unserialise() "vulnerabilities"

[Nikita Popov]

On Thu, Oct 12, 2017 at 4:38 PM, Dmitry Stogov  wrote:

> Hi,
>
>
> I've found, that at least half of unserialise() security problems, occurs
> because of non-symmetric serialize/unserialize assumption, regarding
> references encoded with "r".
>
>
> serialize() assumes it's an object.
>
>
> https://github.com/php/php-src/blob/master/ext/standard/var.c#L828
>
>
> universalize() allows any value.
>
>
> https://github.com/php/php-src/blob/master/ext/standard/
> var_unserializer.re#L677
>
>
> This allows manual crafting of strings that may lead to creation of
> unexpected data structures.
>
> I propose to fix this just by fixing the symmetry.
>
>
> https://gist.github.com/dstogov/53382540bdfee7b6c7dadf142dc437ed
>
>
> This will prohibit, some manually crafted strings.
>
> Of course, this will break few "security" related tests. Especially:
>
>
> > Bug #70284 (Use after free vulnerability in unserialize() with GMP)
> [ext/gmp/tests/bug70284.phpt]
> > Bug #70211 (php 7 ZEND_HASH_IF_FULL_DO_RESIZE use after free)
> [ext/soap/tests/bug70211.phpt]
> > Bug #70172 - Use After Free Vulnerability in unserialize()
> [ext/standard/tests/serialize/bug70172.phpt]
> > Bug #70963 (Unserialize shows UNKNOW in result)
> [ext/standard/tests/serialize/bug70963.phpt]
> > Memleaks if unserialize return a self-referenced array/object
> [ext/standard/tests/serialize/unserialize_mem_leak.phpt]
> > Bug #72433: Use After Free Vulnerability in PHP's GC algorithm and
> unserialize [ext/standard/tests/strings/bug72433.phpt]
>
> Any objections? (this is for master only of course)
>

Hi,

I don't think this will really fix any vulnerabilities, because the core
issue are R references, not r references. If this prevents a vulnerability
using r, you can usually replicate something similar using R instead.

However, I still agree that it makes sense to restrict this. Especially
because unserialize() currently allows creating structures that are just
impossible in plain PHP, such as cyclic arrays without use of references
(GLOBALS notwithstanding).

The check looks too strict to me though. Shouldn't it first DEREF the value
before performing the OBJECT check? (E.g. for something like
"a:3:{i:0;O:8:"stdClass":0:{}i:1;R:2;i:2;r:2;}", in which case r:2 will be
a REF to OBJECT).

Regards,
Nikita