PHP Object Injection Vulnerability in SessionCookie.php

[sarciszewski]

https://github.com/slimphp/Slim/blob/master/Slim/Middleware/SessionCookie.php#L127

Generally, it's a bad idea to blindly unserialize() user-controllable input.

https://www.owasp.org/index.php/PHP_Object_Injection

EDIT - for people who don't want to read the whole thread:

The SessionCookie class is not used by default, you have to actually write your application to use it. So this means that the unserialize() -> RCE possibility is only for the select few apps that explicitly use this feature. The default is the native session driver, which is of course not vulnerable.

[4nd]

Could be worth changing this class to store data in JSON format?

[sarciszewski]

Why are you storing session state in the hand of the client in the first place?

I'd hate to be self-referential, but see similar work I've done for Kohana and CodeIgniter: https://scott.arciszewski.me/research/view/php-framework-timing-attacks-object-injection

They had a similar problem, except they were using hash functions (poorly) to detect tampering. It's still a bad idea. Use server-side storage:

• Files (default) are okay.
• Memached is faster.
• Databases are a good choice if you have thin web servers and a big database.

Cookies are doom.

[4nd]

Totally agree, but some people may need them for some reason. They're good for long-term client-side configuration storage, no need for objects though, a key/value pair should be enough. Definitely not for use with secure data.

[sarciszewski]

Totally agree, but some people may need them for some reason.

Okay. Who needs them and what are they doing that requires such a poorly designed feature? PHP supports file storage out of the box.

• Using Memcache or a database allows you to scale webservers horizontally.
• Replacing the session save path with a samba share allows load-balancing (though is not fault-tolerant; if the share goes offline you lose everything).

People are going to use sessions for secure data. ['user_id' => 92, 'is_admin' => false] Hmm, gee, I wonder how I could exploit that?

[mattsah]

@sarciszewski

How do you feel about storing such data in a cookie as an encrypted JSON Web Token?

[sarciszewski]

NO

#1035 - I do not trust the framework authors to implement encryption properly after reading Slim\Http\Util

[codeguy]

@sarciszewski But how do you really feel? I agree earlier versions should not persist session data client side. This will be changed in 3.x. You can also review the latest encryption strategies in \Slim\Crypt in the develop branch.

[sarciszewski]

https://github.com/slimphp/Slim/blob/develop/Slim/Crypt.php#L128

http://php.net/manual/en/function.hash-equals.php

[codeguy]

@sarciszewski Please review the develop branch \Slim\Crypt class and let me know if you have any feedback. I want it peer reviewed as much as possible. It's already had a lot of eyes on it, but one more won't hurt. Thanks!

[joepie91]

@4nd

Totally agree, but some people may need them for some reason.

Like what reasons, for example?

They're good for long-term client-side configuration storage, no need for objects though, a key/value pair should be enough.

That's what non-session cookies and local storage are for. This does not belong in a session, end of story.

[mattsah]

@sarciszewski

I wasn't necessarily referring to the context of slim. https://github.com/firebase/php-jwt/blob/master/Authentication/JWT.php uses PHP's built in hmac and openssl stuff. Why wouldn't one just use a third party library? It becomes as simple as, pass it data to JWT encode, pass it a key for encryption.

Not sure I follow your concern. You seem to suggest they use someone else's php encryption library to solve that case. Wouldn't that be fine for JWT?

[joepie91]

@mattsah Why are you trying to store session data client-side to begin with?

EDIT: Sorry, mistook you for @codeguy - you are not a contributor, I guess?

[mattsah]

@joepie91

As I believe was already covered, it does make things more scalable. That is, I think I'd agree with the principle as described in REST, and the client should contain the necessary state to succeed in a given request. Sessions blur this, and if the session is removed, the client state is affected irreversibly.

This is true for memcache as well. It scales horizontally, but the client application state is still dependent on the state of the server... clear memcache, and the client is suddenly logged out.

EDIT: Even if you mistook me, I agree that "session data" such as a login name can and should be stored client side. It's part of the client state. I agree, this means we need a means to do so securely, but the answer to that is not "move the client state to the server."

[joepie91]

I don't really see how this makes it more "scalable". You may as well run a Redis cluster to keep track of sessions, if you're at the scale where you need multiple backend servers - statefulness doesn't change anything here.

And frankly, if you're running at that kind of scale, you don't need to rely on the default framework configuration, and can easily roll your own session handler - they are pluggable, after all. Hell, even if you can't, there's plenty other solutions like sticky sessions.

Right now, Slim is shipping in an insecure default for the majority of users, in order to solve an effectively non-existent problem that is an edge case at best. People are not going to read that caveat about secure storage, and cookie encryption is almost certainly going to cause issues later down the line. Not to mention the size limits that are going to bite people.

TL;DR: It makes absolutely no sense to implement this in a framework like this.