Field sql storage has an unrestricted unserialize() call

Created on 7 February 2025, 7 months ago

Problem/Motivation

This was originally logged as a private issue to the security team, but was cleared to be moved to the public queue

The field sql storage (like the cache layer) has an unrestricted unserialize() call.
In https://www.drupal.org/project/drupal/issues/2232427 📌 Allow field types to control how properties are mapped to and from storage Needs work the question arose if can lead to another instance of SA-CORE-2019-003 which is about api-injected evil data (but i'm not familiar with the internal details of that one).

Some research:
- Core uses serialized storage of objects in section storage (see https://www.drupal.org/project/drupal/issues/3484469 📌 Do not store sections as PHP objects Active ).
- Custom code may use that too (i'm doing it a lot)
- The unserialize() is in \Drupal\Core\Entity\Sql\SqlContentEntityStorage::mapFromStorageRecords
- Mitigation: As i grok it, an attack can only happen if evil data passes through the (core / custom) FieldItem class and is written to DB.

More eyes needed:
- Some more eyes please cross-check the claim "an attack can only happen if evil data passes through the (core / custom) FieldItem class and is written to DB." as that is fundamental to all the following.

Steps to reproduce

Proposed resolution

- add a warning to the 'serialize' property for field item storage in FieldItemInterface::schema, sth like "If 'serialize' is used, the FieldItem class is responsible to prevent storage of evil classes, via ::setValue or custom TypedData class"
- add a allowedClasses whitelist to 'serialize' and deprecate usage without it.
- add comments to all unresticted serialize() calls and possibly add a code style rule to guard that.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Background information

List of people initially involved and should receive credit here:
bradjones1
bramdriesen
mcdruid
benjifisher
greggles
geek-merlin

🐛 Bug report
Status

Active

Version

11.0 🔥

Component

entity system

Created by

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Live updates comments and jobs are added and updated live.
  • Security

    It is used for security vulnerabilities which do not need a security advisory. For example, security issues in projects which do not have security advisory coverage, or forward-porting a change already disclosed in a security advisory. See Drupal’s security advisory policy for details. Be careful publicly disclosing security vulnerabilities! Use the “Report a security vulnerability” link in the project page’s sidebar. See how to report a security issue for details.

Sign in to follow issues

Comments & Activities

  • Issue created by @BramDriesen
  • 🇧🇪Belgium BramDriesen Belgium 🇧🇪

    Adding Wim as well 😊

  • 🇺🇸United States benjifisher Boston area
  • 🇺🇸United States benjifisher Boston area
  • 🇺🇸United States benjifisher Boston area

    The title of this issue refers to "Field sql storage", and the proposed resolution mentions Drupal\Core\Field\FieldItemInterface::schema(). But 📌 Allow field types to control how properties are mapped to and from storage Needs work refers to \Drupal\Core\Entity\Sql\SqlContentEntityStorage, and I think that is where the actual calls to unserialize() are.

  • 🇮🇳India kalpanajaiswal

    I'm still relatively new to contributing to core, but I’ve been trying to understand this issue and the potential risks around unserialize() and serialized field storage.
    Please feel free to correct or improve them:

    1. Add a Warning in FieldItem Schema
    Since 'serialize' => TRUE can be risky, we could document something like the following in FieldItemInterface::schema():

    When using 'serialize' => TRUE, the FieldItem class must ensure only safe values (such as arrays or scalars) are stored. Arbitrary PHP objects should not be serialized.

    2. Use allowed_classes in unserialize()
    Wherever unserialize() is used (for example, in SqlContentEntityStorage::mapFromStorageRecords()), it might be safer to include:

    
    unserialize($data, ['allowed_classes' => false]);
    

    Alternatively, we could use a limited allowlist of known-safe classes to prevent PHP object injection vulnerabilities.

    3. Deprecate Unrestricted unserialize() Usage
    We could consider marking all usages of unserialize() without the allowed_classes parameter as @deprecated, and encourage the use of a safer wrapper function or utility.

    4. Add a PHPCS Rule
    A PHPCS rule could help identify unsafe uses of unserialize() during development—similar to how we detect unsafe render arrays.

    5. Audit Use of 'serialize' => TRUE in Core and Contrib
    It might be worth reviewing all field types in core and contrib that use 'serialize' => TRUE, to ensure they are not accepting object instances via setValue() or unsafe API data.

Production build 0.71.5 2024