#Security improvements

xjm → credited star-szr → .

Crediting triage as per #19.

xjm → credited alexpott → .

xjm → credited catch → .

Adding triage credits as per #86.

I discussed this with @catch, @xjm, and @alexpott, and regrettably, we agree with Wim's earlier conclusion in #56 and again in #80 that this isn't viable for core in 8.0.

It's definitely eligible to be considered now on that basis, although we'd want to evaluate whether the approach is still valid.

Amending attribution.

I still don't think we're quite at a complete RTBC from the above, but I re-reviewed the full details of the private issue and will add my +1 to the RTBC confirming that it does indeed provide test coverage that should prevent the vulnerability from being reintroduced, despite that it is not possible to create an actual full exploit test in this situation. :)

Verified that the test-only job fails as expected:

https://git.drupalcode.org/issue/drupal-3526769/-/jobs/5685661

..but tests pass with the changes.

Here's the contents of the test tarball:

core/modules/config/tests/fixtures$ tar ztvf not_just_config.tar.gz 

-rw-rw-r-- mcdruid/mcdruid  48 2025-06-26 10:50 config.one.yml
-rw-rw-r-- mcdruid/mcdruid  48 2025-06-26 10:50 config.two.yml
-rwxrwxr-x mcdruid/mcdruid  48 2025-06-26 11:03 executable.yml
-rwxrwxr-x mcdruid/mcdruid  60 2025-06-26 10:52 script.sh

I was wondering about putting that in a comment in the test to save anyone having to examine it themselves.. but perhaps that just risks getting out of sync with the fixture file, and might not be necessary / helpful anyway.

Added a basic test that verifies a shell script cannot be smuggled into a config import tarball, and checks that files are not executable after imports (even when they are inside the tarball).

I think this needs to be a functional test, as the changes being made are in the processing of the import form on submission.

Thank you! Manually tested as well and works well. Will get this and the other in asap and make a release.

@alexpott:

I created the child issue 📌 Never bypass validation when saving a config object Active .

I think an exception throw is the best way to notify the developer..

I agree but I wonder if it could be a breaking change if some custom code relies on passing something that is currently evaluated to true?

I have created a patch to adda warning.

Not that both |raw and Markup bypass Drupal's XSS filter so the module should make sure the markup in this variable is safe: 🐛 XSS in footnote text Active

Is there any reason to truncate the hash in ImageStyle::getPathToken()?
Would using the whole hash cause problems?

A potential solution would be to add a warning for text format when user uses configuration that is known to enable self-XSS.

Could this be covered by ✨ Provide warning when trying to use dangerous HTML markup Needs work ?

This module does something similar: https://www.drupal.org/project/protect_form_flood_control →

I merged the latest 11.x

I merged the latest 11.x.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

@benjifisher that said I think it would be more consistent to do:

    // If there is a schema for this configuration object, cast all values to
    // conform to the schema.
    if (!$has_trusted_data && $this->typedConfigManager->hasConfigSchema($this->name)) {
      // Ensure that the schema wrapper has the latest data.
      $this->schemaWrapper = NULL;
      $this->data = $this->castValue(NULL, $this->data);
    }
    else {
      foreach ($this->data as $key => $value) {
        $this->validateValue($key, $value);
      }
    }

Do you want to open an issue and implement?

@benjifisher it still wouldn't work because not all config saves are made via their own API - eg. config import / module install / ConfigFormBase - you'd have an awful lot to override... and then you could not export your config because our yaml export does not support objects.

Just to be sure I understand the comments:

Drupal\Core\Config\Config::save() calls validateValue() (both directly and indirectly, via castValue()). That ensures that proper use of the config system protects against serializing anything other than nested arrays of scalar types.

  public function save($has_trusted_data = FALSE) {
// ...
    if (!$has_trusted_data) {
      if ($this->typedConfigManager->hasConfigSchema($this->name)) {
        // Ensure that the schema wrapper has the latest data.
        $this->schemaWrapper = NULL;
        $this->data = $this->castValue(NULL, $this->data);
      }
      else {
        foreach ($this->data as $key => $value) {
          $this->validateValue($key, $value);
        }
      }
    }

By "proper use", I mean that save() is not called with the optional $has_trusted_data parameter set to TRUE.

I am a little worried that some custom or contrib code might use $config_object->save(TRUE) in order to bypass this validation. Am I being too pessimistic?

Looks like it's a random fail 🐛 [random test failure] ScaffoldUpgradeTest and ScaffoldTest rely on packagist.org Active

Looks like the test fail is an unrelated PHP 8.5 test.

Rebased on 11.x and fixed tests. We had space ' ' in the list of special chars, but this is handled separately.

Wonder if something similar can be done like in EntityBase for original

Updates to IS now that we've agreed a proposed solution.

Committed and pushed 4cfe1582b8b to 11.x and e6a220a7ebd to 11.2.x and b5eb5d74aab to 10.6.x and 45ca12eacc1 to 10.5.x. Thanks!

Backported to 11.2.x and 10.5.x and 10.6.x as a very low risk security improvement given the code in \Drupal\Core\Config\StorableConfigBase::validateValue().

Config explicitly errors if you try to add an object. See \Drupal\Core\Config\StorableConfigBase::validateValue - any non scalar value we cause an exception.

Created the MR from the security issue MR.