Skip to content

security: add allowed_classes => false to SequenceGenerator::unserialize()#12492

Closed
XananasX7 wants to merge 1 commit into
doctrine:3.6.xfrom
XananasX7:security/sequence-generator-unserialize
Closed

security: add allowed_classes => false to SequenceGenerator::unserialize()#12492
XananasX7 wants to merge 1 commit into
doctrine:3.6.xfrom
XananasX7:security/sequence-generator-unserialize

Conversation

@XananasX7

Copy link
Copy Markdown

Summary

The deprecated Serializable::unserialize() method in SequenceGenerator calls unserialize($serialized) without an allowed_classes restriction.

// src/Id/SequenceGenerator.php  (before)
$this->__unserialize(unserialize($serialized));

The serialized payload is always the output of __serialize(), which returns only two scalar values:

['allocationSize' => $this->allocationSize, 'sequenceName' => $this->sequenceName]

No object classes are expected. Without allowed_classes => false, a crafted serialized string supplied to the deprecated method could instantiate arbitrary autoloaded classes during deserialization (PHP Object Injection), potentially triggering a gadget chain.

Fix

$this->__unserialize(unserialize($serialized, ['allowed_classes' => false]));

This is safe because __unserialize() only reads $data['sequenceName'] (string) and $data['allocationSize'] (int).

Notes

  • The Serializable interface and its methods are already deprecated; this is a defense-in-depth fix to harden the remaining code path until it is removed in ORM 4.
  • No behaviour change for legitimate data.
  • The modern __serialize() / __unserialize() magic methods are not affected.

…ize()

The deprecated Serializable::unserialize() method calls unserialize($serialized)
without an allowed_classes restriction. The serialized data is always a plain
associative array of two scalar values (sequenceName: string, allocationSize: int)
produced by __serialize(); no object classes are expected.

Adding allowed_classes => false prevents PHP Object Injection if a crafted
serialized payload is supplied to the deprecated unserialize() method, which
could trigger a gadget chain during deserialization.
@beberlei

Copy link
Copy Markdown
Member

Its virtually impossible that this happens in real applications as it would require user supplied entity mappings, but we can merge this regardless to avoid machines that are not able to make this conclusion from reporting this again.

@greg0ire

Copy link
Copy Markdown
Member

I'm working on a fix for the static analysis issue: #12493

@greg0ire greg0ire closed this Jun 2, 2026
@greg0ire greg0ire reopened this Jun 2, 2026
@XananasX7

Copy link
Copy Markdown
Author

Thanks @beberlei and @greg0ire! I saw PR #12493 — looks like you already have a more complete fix in the works that also addresses the static analysis warning properly. Happy to close this one in favour of that if it covers the same ground. Let me know!

@greg0ire

Copy link
Copy Markdown
Member

It does not cover the same ground at all, it fixes the build. However I think we can close this, so as not to encourage PR that fix issues that do not really exist. I'm confident tooling will get better, and if it doesn't, this looks like something that should be enforced at the linter or static analysis level.

@greg0ire greg0ire closed this Jun 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants