Secure unserialize by restricting allowed classes when using PHP 7 #21090
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stloyd
suggested changes
Dec 29, 2016
| $options = null; | ||
| if (PHP_VERSION_ID >= 70000) { | ||
| $options = array('allowed_classes' => false); | ||
| } | ||
| return unserialize($this->nodes); |
There was a problem hiding this comment.
I removed that bit for now because $this-nodes contains Node-classes. I will have to check whether I can restrict allowed_classes here.
xabbuh
reviewed
Dec 29, 2016
| if (PHP_VERSION_ID >= 70000) { | ||
| $options = array('allowed_classes' => array('Twig_Profiler_Profile')); | ||
| } | ||
| $this->profile = unserialize($this->data['profile'], $options); |
There was a problem hiding this comment.
To avoid problems with PHP 5.x this could be changed to:
if (PHP_VERSION_ID >= 70000) {
$this->profile = unserialize($this->data['profile'], array(
'allowed_classes' => array('Twig_Profiler_Profile'),
));
} else {
$this->profile = unserialize($this->data['profile']);
}There was a problem hiding this comment.
Yes, but do we want this? I was thinking about something like this: https://github.com/dbrumann/polyfill-unserialize
There was a problem hiding this comment.
I confirm we prefer doing this than adding a dependency yes :)
There was a problem hiding this comment.
Ok, then I will change the occurrences instead. Thanks for the feedback.
|
👍 |
|
👍 Status: Reviewed |
|
Thank you @dbrumann. |
fabpot
added a commit
that referenced
this pull request
Feb 12, 2017
…sing PHP 7 (dbrumann) This PR was merged into the 3.3-dev branch. Discussion ---------- Secure unserialize by restricting allowed classes when using PHP 7 | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | --- | License | MIT | Doc PR | --- While playing around with Symfony in a PHP 7.1 application I noticed a warning in how EnvParameterResoure uses unserialize. Since PHP 7.0 introduced the options argument which allows to restrict which classes can be unserialized for better security, it might make sense to use it here. As far as I can tell this is no BC break, it only provides an additional safety mechanism. Commits ------- b420181 Conditionally add options to unserialize in PHP 7.0+.
Closed
stof
reviewed
Jun 9, 2017
| @@ -185,6 +185,10 @@ public function serialize() | |||
| */ | |||
| public function unserialize($serialized) | |||
| { | |||
| list($this->message, $this->messageTemplate, $this->messageParameters, $this->messagePluralization, $this->cause) = unserialize($serialized); | |||
| if (PHP_VERSION_ID >= 70000) { | |||
| list($this->message, $this->messageTemplate, $this->messageParameters, $this->messagePluralization, $this->cause) = unserialize($serialized, array('allowed_classes' => false)); | |||
There was a problem hiding this comment.
this looks wrong to me. The cause can be an object (and actually, it is an object in many cases in Symfony)
Gribnif
pushed a commit
to Gribnif/symfony
that referenced
this pull request
Jun 9, 2017
…e it breaks BC. Refers to symfony#21090 and symfony#23109
fabpot
added a commit
that referenced
this pull request
Jun 13, 2017
…outing/Route.php (Dan Wilga) This PR was submitted for the master branch but it was merged into the 3.3 branch instead (closes #23121). Discussion ---------- [Routing] Revert the change in [#b42018] with respect to Routing/Route.php | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21090 #23109 | License | MIT | Doc PR | ...because it breaks BC with third-party code which, for instance, might use a subclass of CompiledRoute within the options portion of the Route. Refers to #21090 and #23109 Commits ------- f09893b [Routing] Revert the change in [#b42018] with respect to Routing/Route.php
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While playing around with Symfony in a PHP 7.1 application I noticed a warning in how EnvParameterResoure uses unserialize. Since PHP 7.0 introduced the options argument which allows to restrict which classes can be unserialized for better security, it might make sense to use it here. As far as I can tell this is no BC break, it only provides an additional safety mechanism.