Skip to content
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.

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

Remove session set value will convert to an array/obj value. #1

Open
weierophinney opened this issue Dec 31, 2019 · 5 comments
Open

Comments

@weierophinney
Copy link
Contributor

I saw the Session::extractSerializableValue method comment

This value should be used by set() operations to ensure that the values
within a session are serializable across any session adapter.

But most of adapters can store string after using serialize() function . And I think this is adapter layer.

And then, I think can remove it.

Code to reproduce the issue

class User extends \ArrayObject {
}

$session->set(User::class, new User(['id' => 1]))
$user = $session->get(User::class); 

Expected results

$user instanceof User; 

Actual results

$user; 
// Is array:
// ['id' => 1]

Originally posted by @Moln at zendframework/zend-expressive-session#37

@weierophinney
Copy link
Contributor Author

Hello @Moln,
I believe this was done intentionally in order to have the same (array/hashmap) representation among all adapters/backends (see https://www.php.net/manual/en/book.memcache.php#115666 for instance memcache/memcached incompatibility when storing the same object).
I believe you should use the stored session data to populate the object ($user = new User($session->get(User::class))).

kind regards


Originally posted by @pine3ree at zendframework/zend-expressive-session#37 (comment)

@weierophinney
Copy link
Contributor Author

This design decision is pretty odd and should have nothing to do with memcache or adapters as long as it serializes to a string. Of course, JSON is easier to parse for NON-PHP environments but we're talking about PHP storing it's data to a session storage. I don't see the case that any other NON-PHP application should read PHP's session storage...

The much better (and faster) way would have been to use PHP's serialize method.

Maybe @weierophinney has more insights into this?


Originally posted by @maurice2k at zendframework/zend-expressive-session#37 (comment)

@weierophinney
Copy link
Contributor Author

The much better (and faster) way would have been to use PHP's serialize method.

I disagree that it's "much better", and can argue that it's a bad choice for handling session data.

First, if you Google for "php serialize security", you'll quickly discover taht the internal serializer in PHP has had a slew of security vulnerabilities, the worst of which have led to RCE vectors. Most security experts indicate it should be avoided whenever possible.

In particular, they indicate that storing serialized objects in a session is a recipe for additional security vectors, and should be avoided.

On top of that, there are the problems of what data different session storage backends can actually store. ext-session is not the only target: we want to support cache backends (redis, memcache), and non-persistent backends such as JWT.

If we consider security of data and the breadth of options for storing session data, serialize() becomes more and more problematic. As such, we chose to go an alternate route for this component.


Originally posted by @weierophinney at zendframework/zend-expressive-session#37 (comment)

@weierophinney
Copy link
Contributor Author

Thanks for your response.

If serialize really has had any security issues these should have been fixed by now (didn't find anything with your suggested search).
The problematic function is unserialize if you use it with untrusted input that can be tampered with -- but I don't see this case with sessions. If anyone is able to modify session files on the server I have a much bigger problem. And PHP's default setting for storing session data is to use serialize (to be more precise: session_encode which internally uses serialize).

Regarding different storage backends: I have never seen a backend that is not capable of storing a string. Redis and Memcache don't care if the data you store is encoded using JSON oder PHP's serialize. So that shouldn't be a problem either or am I missing something?

The only plausible reason for me to implement something like json_decode(json_encode($value, JSON_PRESERVE_ZERO_FRACTION), true) is to totally get rid of objects so that the average developer is not able to break anything if an object with many references/recursion gets stored in the session.

It would be easy to modify this behaviour in Session but then I would also have to adapt those new Session calls in for example https://github.com/zendframework/zend-expressive-session-ext/blob/master/src/PhpSessionPersistence.php#L119

The only reasonable workaround is to serialize the object myself, set the serialized string into the session (json_decode/encode is basically a no-op then) and do an unserialize after getting it from session.


Originally posted by @maurice2k at zendframework/zend-expressive-session#37 (comment)

@weierophinney
Copy link
Contributor Author

Thanks for everyone follow and response.

In particular, they indicate that storing serialized objects in a session is a recipe for additional security vectors, and should be avoided.

Yes, now I don't think this method (Session::extractSerializableValue()) is wrong.
But I mean this can be move to adapter layer, let the adapter layer decide how to store sessions.

@weierophinney


e.g.: zendframework/zend-expressive-session-ext's PhpSessionPersistence::persistSession()

https://github.com/zendframework/zend-expressive-session-ext/blob/0aac3766fd87d069ea41e0b42f86037f58aa94a1/src/PhpSessionPersistence.php#L139

$_SESSION = $session->toArray();  // PhpSessionPersistence.php#L139

// Change to 
$_SESSION = extractSerializableValue($session->toArray());

Originally posted by @Moln at zendframework/zend-expressive-session#37 (comment)

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

No branches or pull requests

1 participant