Clean up unserialize() in the config system

benjifisher → credited catch → .

benjifisher → credited greggles → .

benjifisher → credited larowlan → .

benjifisher → credited mcdruid → .

I think it is all right to allow stClass, but tests are passing with ['allowed_classes' => FALSE], so let's at least consider that.

Does this issue need a change record? If there are any contrib or custom modules that extend the DatabaseStorage class, and they need to allow other classes in unserialize(), then the good news is that they can easily override the decode() method, which is now

  public function decode($raw) {
    $data = @unserialize($raw, ['allowed_classes' => FALSE]);
    return is_array($data) ? $data : FALSE;
  }

I see there is one more @unserialize is that impacted?

This issue is scoped to the Config system, and I think there is only one:

$ grep -ri unserialize core/lib/Drupal/Core/Config
core/lib/Drupal/Core/Config/DatabaseStorage.php:   *   The unserialize() call will trigger E_NOTICE if the string cannot
core/lib/Drupal/Core/Config/DatabaseStorage.php:   *   be unserialized.
core/lib/Drupal/Core/Config/DatabaseStorage.php:    $data = @unserialize($raw);

Did I miss something or are you thinking of something outside the Config system?

No just saw this other instance and didn't know if it had the same problem.

Then I guess you mean the usage in the dblog module:

$ grep -ri @unserialize core
core/modules/dblog/src/Controller/DbLogController.php:      $variables = @unserialize($row->variables);
core/lib/Drupal/Core/Config/DatabaseStorage.php:    $data = @unserialize($raw);

The @ just tells PHP to ignore errors from serialize(): see Error Control Operators in the PHP docs. There are many other calls to serialize() in Drupal core.

In the long run, we should switch to using json_encode() and json_decode() (or the equivalent methods in Drupal\Component\Serialization\Json) whenever possible instead of serialize() and unserialize(). In the short run, it is less disruptive to specify allowed_classes.

Not 100% how else to test so am relying on tests, which are green. Locally nothing seems broken so going to go on a limb

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

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().

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?

@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.

@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?

@alexpott:

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

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