harden D7 against PHP Gadget Chain Drupal7/RCE1

Initial patch which prevents the exploit from working.

This includes a variable that would allow sites to opt out of this should they actually need \DrupalCacheArray::__destruct() to persist key/value pairs into cache_form.

I think it's likely to be very rare that sites would need this opt-out, so I'd propose that we don't add the variable to default.settings.php

There's a risk that we might not notice a problem caused by a change to this functionality as it may only cause a performance degradation as there are more cache misses, as opposed to actually breaking anything.

I'm curious about whether the D7 test suite actually exercises this functionality much.

It looks like it's very common for the destructor to persist data into the "cache" bin as part of tests, and there are a couple of mentions of DrupalCacheArray's behaviour in drupal_web_test_case.php for example.

This patch is an experiment to see whether the test suite results in anything being "persisted" to a cache bin other than the default.

If it does, we should get exceptions that will be easy to see in the results.

To be clear, this is not something we'd actually commit.. and nobody should apply it to their site unless they're also trying a similar experiment to see if their custom code/tests results in writes to non-default cache bins from this destructor.

Hmm, so unless I'm missing something that suggests that the D7 test suite doesn't do anything that results in DrupalCacheArray writing to a cache bin other than the default "cache".

If we don't exclude that default cache bin, a lot of tests end up throwing errors like:

Exception: Persisting data to cache bin cache: link in /var/www/html/includes/bootstrap.inc:469
Stack trace:
#0 /var/www/html/includes/bootstrap.inc(3786): DrupalCacheArray->__destruct()
#1 /var/www/html/includes/bootstrap.inc(3802): drupal_static('theme_get_regis...', NULL, true)
#2 /var/www/html/modules/simpletest/drupal_web_test_case.php(1903): drupal_static_reset()
#3 /var/www/html/modules/simpletest/drupal_web_test_case.php(1820): DrupalWebTestCase->resetAll()
#4 /var/www/html/modules/simpletest/tests/theme.test(687): DrupalWebTestCase->setUp('theme_test')
#5 /var/www/html/modules/simpletest/drupal_web_test_case.php(563): ModuleProvidedThemeEngineTestCase->setUp()
#6 /var/www/html/scripts/run-tests.sh(429): DrupalTestCase->run(Array)
#7 /var/www/html/scripts/run-tests.sh(26): simpletest_script_run_one_test('9', 'ModuleProvidedT...')
#8 {main}FATAL ModuleProvidedThemeEngineTestCase: test runner returned a non-zero error code (2).

I've been wondering whether we'd be better to take an allow-list or block-list approach here.

The result of this experiment makes me think we're probably okay to just block cache_form specifically (with the opt-out variable in case that breaks things on specific sites) - which is what patch #2 does.

The problem with cache_form specifically is the fact that the ajax system provides a way to execute the payload. Even if an attacker can write their own data to other cache bins/tables, it doesn't matter much if they can't then do anything with the data they've written to cache.

Thanks for working on this @mcdruid!

Let's summarize this. If I understand this correctly, you can "trick" the DrupalCacheArray by manually constructing a SchemaCache or ThemeRegistry object and using an insecure deserialisation to write to a diffent cache bin - specifically the cache_form (the different I mean when comparing with what the D7 core does). Then this data/payload can be retrieved and executed by calling the system/ajax (which is how ajax forms works). Is this correct?

When looking at the D7 core code, it seems like these are the only two classes extending and using the DrupalCacheArray and when used in core, these are writing to the cache bin by default.

I have not seen any attempts to write to cache_form via DrupalCacheArray when "manually" testing this on a demo site. Writes to the cache_form were via form_set_cache() method. I know this is not a 100% proof, but I think that I agree with this:

I think it's likely to be very rare that sites would need this opt-out,

Regarding this:

I've been wondering whether we'd be better to take an allow-list or block-list approach here.

If everything I said above is correct, then I think, we should be fine with what the patch #2 does. We does not need to offer possibility to allow/block-list other cache bins, if those does not have the possibility to execute the payload afterwards, see:

The problem with cache_form specifically is the fact that the ajax system provides a way to execute the payload. Even if an attacker can write their own data to other cache bins/tables, it doesn't matter much if they can't then do anything with the data they've written to cache.

We can, probably, try to grep a bunch of contrib modules (if you have that possibility), if there are any modules extending the DrupalCacheArray and using the cache_form bin by default (so that we would not block these regular writes to that bin), but I do not think so.

There seems to be one more possible scenario, how the write to cache_form regularly via DrupalCacheArray. The ThemeRegistry class allows to define a bin on initialization. So if there is anyone using the ThemeRegistry with a different bin (for example cache_form), then this code would write to cache_form either (and the patch in #2 would block this). But I think this is very unlikely to happen.

Thanks for taking a look @poker10... yes, your summary matches my understand of what's going on.

It's a clever hack that someone came up with to abuse DrupalCacheArray's ability to persist keys/values to the db (/ other cache storage) which allows them to inject a payload that can then be executed via the ajax system in a second request.

I also don't see any evidence of core using the functionality to write to a bin other than the default "cache" but perhaps it's plausible that contrib / custom code does.

As we know, cache_form isn't really a cache so I think it's probably quite safe to assume that it's really unlikely anyone's deliberately / legitimately writing to cache_form like this.

We can provide the override/opt-out variable just in case, but I think it's so unlikely to be used that we should not clutter up default.settings.php with it; it could be documented in a CR (and here).

So with all that confirmed, do you think this needs anything else before RTBC / commit?

Yeah, we can keep the opt-out variable just in case, so I think the patch #2 looks good. Agree that we do not need to add it to the default.settings.php.

I cannot think of anything else right now, except the change record, so I think this could be RTBC. Thanks!

Thanks for the review.

I drafted a CR at [#3381030].

I'll try to get this committed today so that there's plenty of time for it to be tested in dev before the next D7 release.

Thank you!

Automatically closed - issue fixed for 2 weeks with no activity.