Corrupted memory in destructor with weak references

[mvorisek]

Description

In the code below, var_dump(array_keys($analysingMap->valueWithOwnerCountByIndex)) called twice prints different results even if the value is not changed between the calls.

I did my best to create a minimal repro. It seems the issue is present when weak references and destructor is used.

When opcache and/or GC is disabled, the issue is still present. It seems there is some allocation/refcounting issue in destructors.

repro: https://3v4l.org/h7ZER

<?php

class WeakMap2
{
    private array $weakRefs = [];
    private array $values = [];

    public function offsetSet($object, $value) : void
    {
        $id = spl_object_id($object);

        $this->weakRefs[$id] = \WeakReference::create($object);
        $this->values[$id]   = $value;
    }
}

class WeakAnalysingMapRepro
{
    /* private */ public array $valueWithOwnerCountByIndex = [];

    private WeakMap2 $ownerDestructorHandlers;

    public function __construct()
    {
        $this->ownerDestructorHandlers = new WeakMap2();

        $this->addKeyOwner(new \DateTime());
    }

    protected function addKeyOwner(object $owner)
    {
        $handler = new class($this) {
            private \WeakReference $weakAnalysingMap;

            public function __construct(WeakAnalysingMapRepro $analysingMap)
            {
                $this->weakAnalysingMap = \WeakReference::create($analysingMap);
            }

            public function __destruct()
            {
                $analysingMap = $this->weakAnalysingMap->get();

                var_dump(array_keys($analysingMap->valueWithOwnerCountByIndex));
                \Closure::bind(static function () use ($analysingMap) {
                    var_dump(array_keys($analysingMap->valueWithOwnerCountByIndex));
                }, null, WeakAnalysingMapRepro::class)();
            }

            public function addReference($index): void
            {
                $analysingMap = $this->weakAnalysingMap->get();
                $analysingMap->valueWithOwnerCountByIndex[$index] = true;
            }
        };

        $this->ownerDestructorHandlers->offsetSet($owner, $handler);

        $handler->addReference(10);
    }
}

$map = new WeakAnalysingMapRepro();
unset($map);

echo 'DONE';

Resulted in this output:

array(1) {
  [0]=>
  int(10)
}
array(1) {
  [0]=>
  string(12) "analysingMap"
}
DONE

But I expected this output instead:

array(1) {
  [0]=>
  int(10)
}
array(1) {
  [0]=>
  int(10)
}
DONE

PHP Version

any (tested 7.4, 8.1, 8.3, master)

Operating System

any (tested Windows, Linux)

[nielsdos]

Reduced further:

<?php

class WeakAnalysingMapRepro
{
    public array $valueWithOwnerCountByIndex = [];

    private array $ownerDestructorHandlers = [];

    public function __construct()
    {
        $handler = new class($this) {
            private \WeakReference $weakAnalysingMap;

            public function __construct(WeakAnalysingMapRepro $analysingMap)
            {
                $this->weakAnalysingMap = \WeakReference::create($analysingMap);
            }

            public function __destruct()
            {
                var_dump($this->weakAnalysingMap->get());
            }

            public function addReference($index): void
            {
                $analysingMap = $this->weakAnalysingMap->get();
                $analysingMap->valueWithOwnerCountByIndex[$index] = true;
            }
        };

        $this->ownerDestructorHandlers[] = $handler;

        $handler->addReference(10);
    }
}

new WeakAnalysingMapRepro();

Results in a reliably reproducible UAF with ASAN.

EDIT: it seems the destruction of the outer object already started, resulting in a failure when var_dumping because it tries to print out freed arrays.

[nielsdos]

Inside zend_object_std_dtor the weakrefs are notified after the destruction of properties already took place, moving that upwards seems to fix this issue, although I'd have to think if there's a better way.
This would mean that OP's code was never intended to work, i.e. the weak reference getting would return NULL. Well it doesn't actually work under ASAN now either, where it crashes due to a UAF...

[mvorisek]

This would mean that OP's code was never intended to work, i.e. the weak reference getting would return NULL.

Sure, I highly reduced the code... the test in #13613 seems correct. Thank you for looking into this issue so blazingly quickly!

One question - can this be classified as security issue, it the fix be landed to lower branches than 8.2?

[nielsdos]

I don't think it qualifies as a security issue as it doesn't seem to fit the attack model from https://github.com/php/policies/blob/main/security-classification.rst

[mvorisek]

Isn't UAF - which can hold any data - a security issue? In theory, UAF can leak sensitive data or be used to corrupt some persistent data like file or database.

Also, according to the linked document only high and medium security issues are usually assigned a CVE and IMO this issue could have CVE report.

[bwoebi]

@mvorisek Whether something is a security issue depends on the remote exploitability. This isn't exploitable without carefully crafted PHP code, i.e. nothing remote attackers can control.

[mvorisek]

I was able to workaround around this issue by creating a property in the partly-destructed object and weakly referencing it - the property seems to be guaranteed to be freed - https://3v4l.org/nUMJD.

It seems it is not possible to fix this issue (from user code) without modifying the weakly referenced object (as we cannot tell if the weakly referenced object started the destruction process or not).

About the security classification, based on the linked document, it can:

• cross security boundaries
• access data that is not intended to be accessible
• disabling the system completely (UAF crash)

Of couse, we can disagree on this, but is there anything againts not landing the fix into at least PHP 8.1, can we ask the release managers for their approval?

[nielsdos]

About the security classification, based on the linked document, it can: (...)

I'll give a bit more explanation about the policy.

First, I agree that a UAF is bad: it can indeed cause data corruption or access data structures that should've already been destroyed.
The security policy does assume a different context though, it assumes the context of a remote attacker. This is different from the situation in the OP where a particularly-constructed code sample causes a crash. This is the reason that this case isn't viewed as a security bug. It would be a different situation though when, for example, a function causes a UAF based on some input: function input is generally considered untrusted.

is there anything againts not landing the fix into at least PHP 8.1, can we ask the release managers for their approval?

You'll have to ping and convince the RMs to do so. I mean, I get it. This is an annoying bug that affects you and PHP 8.1 will still be around for a while, whilst not getting a fix. But that's just the policy. The question also is: if this gets an exception, for an extra 8.1 release, what other bugs should get an exception as well?

[mvorisek]

Hi , @ramsey and @patrickallaert, PHP 8.1 RMs, would you approve this to be landed/fixed in PHP 8.1?

The fix is simple - 39b8d5c871.

As analysed in #13612 (comment) the issue has no easy workaround and in theory, it might cause unwated data to be stored and/or sensitive data leaked.

[ramsey]

I agree with @nielsdos's assessment that this isn't a security bug, so we shouldn't update 8.1 because it opens the door to questions about other types of bugs to merge into 8.1.