8000 Add "allowed_classes" argument for unserialize() by comxd · Pull Request #9563 · symfony/symfony-docs · GitHub
[go: up one dir, main page]
Skip to content

Add "allowed_classes" argument for unserialize() #9563

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add "allowed_classes" argument for unserialize() #9563

wants to merge 1 commit into from

Conversation

comxd
Copy link
Contributor
@comxd comxd commented Apr 7, 2018

No description provided.

@wouterj
Copy link
Member
wouterj commented Apr 8, 2018

I like this suggestion! 👍 Let's wait to hear what others think.

Status: reviewed

@wouterj wouterj self-requested a review April 8, 2018 16:28
@wouterj wouterj requested review from weaverryan and xabbuh April 8, 2018 16:28
weaverryan
weaverryan reviewed Apr 9, 2018
View reviewed changes
Copy link
Member
@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I don’t understand the option - can you explain? From reading, it seems like it will not allow any classes to be deserialized... but I’m sure I’m mistaken.

@comxd
Copy link
Contributor Author
comxd commented Apr 9, 2018

Hi @weaverryan. Yes, it's a required parameter from version 7.0.0 of PHP. @see http://php.net/manual/en/function.unserialize.php

@weaverryan
Copy link
Member

It’s not required, right? Just available?

@comxd
Copy link
Contributor Author
comxd commented Apr 9, 2018

Sorry, you are right. It's symply recommended for security reasons.

@weaverryan
Copy link
Member

Can you explain what it does? Why is it useful here? And what does it protect from? Sorry, I’m just not familiar - and the php.net description didn’t help much :)

@xabbuh xabbuh added this to the 2.7 milestone Apr 9, 2018
@wouterj
Copy link
Member
wouterj commented Apr 9, 2018

@weaverryan it disallows initiating classes from the serialized string. For instance, if a hacker is able to change the serialized string in the session, they are able to set the username for instance to some object. This can pose security issues.

Setting it to false gives a php error in this case.

@xabbuh
Copy link
Member
xabbuh commented Apr 9, 2018

Thank you @comxd.

xabbuh added a commit that referenced this pull request Apr 9, 2018
@xabbuh
minor #9563 Add "allowed_classes" argument for unserialize() (comxd)
6d4cef2 
This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes #9563).

Discussion
----------

Add "allowed_classes" argument for unserialize()

Commits
-------

14c5f04 Add "allowed_classes" argu
A3D2
ment for unserialize()
@xabbuh xabbuh closed this Apr 9, 2018
@weaverryan
Copy link
Member

Ah, yes yes. I understand now. I was thinking that this example WAS serializing a User object. But yes, it’s just an array - so it makes sense to restrict a class suddenly being unserialized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weaverryan weaverryan weaverryan left review comments

@wouterj wouterj wouterj approved these changes

@xabbuh xabbuh xabbuh approved these changes

Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
5 participants
@comxd @wouterj @weaverryan @xabbuh @carsonbot
0