Unserializes, inheritance and allows_classes

php.internals

Marcos Passos

7 years ago
Hi internals, Today I stumbled upon a limitation when implementing the unserialize method of a serializable class which depends on an abstraction also serializable. Currently, there is no way to unserialize an object specifying a parent class in the allowed_classes option: class SerializableBase implements \Serializable {
> } > class SerializableChild extends SerializableBase { > } > class Foo implements \Serializable { > private $dependency; > public function __construct(SerializableBase $dependency) { > $this->dependency = $dependency; > } > public function serialize() : string { > return \serialize($this->dependency); > } > public function unserialize($data) : void { > $this->dependency = \unserialize($data, ['allowed_classes' => > SerializableBase::class]); > } > }
Is this an intentional limitation? - Marcos

Marco Pivetta

7 years ago
On Fri, Jan 18, 2019 at 12:49 AM Marcos Passos <marcospassos.com@gmail.com> wrote:
> Hi internals, > > Today I stumbled upon a limitation when implementing the unserialize method > of a serializable class which depends on an abstraction also serializable. > Currently, there is no way to unserialize an object specifying a parent > class in the allowed_classes option: > > class SerializableBase implements \Serializable { > > } > > class SerializableChild extends SerializableBase { > > } > > class Foo implements \Serializable { > > private $dependency; > > public function __construct(SerializableBase $dependency) { > > $this->dependency = $dependency; > > } > > public function serialize() : string { > > return \serialize($this->dependency); > > } > > public function unserialize($data) : void { > > $this->dependency = \unserialize($data, ['allowed_classes' => > > SerializableBase::class]); > > } > > } > > > Is this an intentional limitation? > >
Seems expected to me: `allowed_classes` is a whitelist, not a complex filter/ruleset. Also: nothing denies an attacker from defining a subtype to your class, then passing a malicious instance to your application. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

Marcos Passos

7 years ago
Hi Marco, Also: nothing denies an attacker from defining a subtype to your class,
> then passing a malicious instance to your application.
Fact, but it also reveals a fragility in the solution in the sense that one has to opt between flexible design or security. Em qui, 17 de jan de 2019 às 22:24, Marco Pivetta <ocramius@gmail.com> escreveu:

Marco Pivetta

7 years ago
On Fri, Jan 18, 2019 at 1:50 AM Marcos Passos <marcospassos.com@gmail.com> wrote:
> Hi Marco, > > Also: nothing denies an attacker from defining a subtype to your class, >> then passing a malicious instance to your application. > > > Fact, but it also reveals a fragility in the solution in the sense that > one has to opt between flexible design or security. > > Em qui, 17 de jan de 2019 às 22:24, Marco Pivetta <ocramius@gmail.com> > escreveu: > >> >> On Fri, Jan 18, 2019 at 12:49 AM Marcos Passos < >> marcospassos.com@gmail.com> wrote: >> >>> Hi internals, >>> >>> Today I stumbled upon a limitation when implementing the unserialize >>> method >>> of a serializable class which depends on an abstraction also >>> serializable. >>> Currently, there is no way to unserialize an object specifying a parent >>> class in the allowed_classes option: >>> >>> class SerializableBase implements \Serializable { >>> > } >>> > class SerializableChild extends SerializableBase { >>> > } >>> > class Foo implements \Serializable { >>> > private $dependency; >>> > public function __construct(SerializableBase $dependency) { >>> > $this->dependency = $dependency; >>> > } >>> > public function serialize() : string { >>> > return \serialize($this->dependency); >>> > } >>> > public function unserialize($data) : void { >>> > $this->dependency = \unserialize($data, ['allowed_classes' => >>> > SerializableBase::class]); >>> > } >>> > } >>> >>> >>> Is this an intentional limitation? >>> >>> >> Seems expected to me: `allowed_classes` is a whitelist, not a complex >> filter/ruleset. >> >> Also: nothing denies an attacker from defining a subtype to your class, >> then passing a malicious instance to your application. >> >> Marco Pivetta >> >> http://twitter.com/Ocramius >> >> http://ocramius.github.com/ >> >> >
Security is not a choice. The design is not fragile, it is strict and correct. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

Marcos Passos

7 years ago
How would you fix this example, then? Perhaps we should add more examples in the docs. Em qui, 17 de jan de 2019 às 22:53, Marco Pivetta <ocramius@gmail.com> escreveu:

Marco Pivetta

7 years ago
On Fri, Jan 18, 2019 at 2:00 AM Marcos Passos <marcospassos.com@gmail.com> wrote:
> How would you fix this example, then? Perhaps we should add more examples > in the docs. >
You'd include all expected child classes in the list of `allowed_classes`. As for the docs, feel free to go ahead and edit them directly :+1: Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

Marcos Passos

7 years ago
> > How would you fix this example, then? Perhaps we should add more examples > in the docs. >
But it closes for extension preventing the serialization of instances not whitelisted. It may work for @internal or package private classes, but not for public classes. Em qui, 17 de jan de 2019 às 23:03, Marco Pivetta <ocramius@gmail.com> escreveu:

Marco Pivetta

7 years ago
On Fri, Jan 18, 2019 at 2:13 AM Marcos Passos <marcospassos.com@gmail.com> wrote:
> How would you fix this example, then? Perhaps we should add more examples >> in the docs. >> > > But it closes for extension preventing the serialization of instances not > whitelisted. It may work for @internal or package private classes, but not > for public classes. >
That is precisely what this is designed for. A subclass is a different type, with a world of possible broken things in it: do not consider the subclass to be the same as a parent class: it's a logical mistake. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

Rowan Collins

7 years ago
On Fri, 18 Jan 2019 at 01:15, Marco Pivetta <ocramius@gmail.com> wrote:
> On Fri, Jan 18, 2019 at 2:13 AM Marcos Passos <marcospassos.com@gmail.com> > wrote: > > But it closes for extension preventing the serialization of instances not > > whitelisted. It may work for @internal or package private classes, but > not > > for public classes. > > > > That is precisely what this is designed for. > > A subclass is a different type, with a world of possible broken things in > it: do not consider the subclass to be the same as a parent class: it's a > logical mistake. >
I think it depends on the scenario, and what risks you're trying to protect against. If you are using the whitelist in the core of a CMS, to stop plugins accidentally introducing problematic behaviour, sub-classes should absolutely be blocked; but if you're using it in a closed application to stop users manipulating the *serialized data*, there is no way for a sub-class to be created unless you create it yourself, so limiting to any class in a given hierarchy would be reasonable. However, you can always implement more complex logic by using custom serialization and deserializing with a factory which creates whichever sub-class is appropriate. Although fairly powerful, serialize() / deserialize() are always going to be a bit of a "lowest common denominator", rather than the right tool for every job. Regards,
-- Rowan Collins [IMSoP]