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.

[mattsah]

@joepie91

It makes it more scalable for the same reasons redis/memcache/session handler does in one sense, but also makes it more scalable in the sense that client state can be provided to other systems or services which may not be tied into the back end state. Sure, it's easy to say, "just tie them into the backend state," but that doesn't seem to really address the issue.

What you're saying is, X service now has to rely on some additional backend component in order to verify client state. Not only might that be completely unnecessary for some services (you're only making it necessary by mixing server/client state), it obviously increases the requisite resources to make that service usable.

When the client state is stored client side and passed in the request, any service, no matter how disconnected, has everything it needs to complete the request. It's really that simple.

[mattsah]

@joepie91

I should probably note, I'm not necessarily disagreeing with you with respect to slim. I'm more making the point on a broader architectural level.

I would agree that at that scale you're going to be doing custom stuff anyway, and this may not be necessary out of the box, and as implemented, causes all sorts of security concerns for the 90%.

[joepie91]

but also makes it more scalable in the sense that client state can be provided to other systems or services which may not be tied into the back end state.

Cookies are hostname- and port-specific, so that will never be directly possible. Data that you want to share between systems should be explicitly shared (even when going through the client) - you can use things like local storage to keep track of this.

There's a fundamental distinction between 'trusted data' and 'shared data', and the latter simply doesn't belong in session data. Non-session cookies or local storage are a much better choice for that.

[mattsah]

Cookies are hostname- and port-specific, so that will never be directly possible.

This is irrelevant with respect to some hierarchical architectures or even more advanced proxies.

There's a fundamental distinction between 'trusted data' and 'shared data', and the latter simply doesn't belong in session data.

I think you meant the former simply doesn't belong in client data. But that's an obviously absurd notion for every single application architecture aside from web applications. The only reasons it's accepted in web applications is because of arguments like the one you're making, namely that there's just no way to do it securely.

All I'm saying is, the "how do we do this securely?" is a separate question from "why/when/should someone do this?"

There are cases for the latter, which means we need solutions for the former. You seem to be saying that there are no solutions for the former, thus, we should do whatever it takes to avoid the latter. You've got your problem solving hat on backwards.

All I was initially asking in this thread, was whether or not @sarciszewski felt JWT was a relatively secure method to store session data client side (regardless of mechanism, cookie, local storage, etc). His answer was NO, but it seemed specific to separate concerns in slim, which didn't really answer the question.

[sarciszewski]

I said "NO" because you shouldn't be storing session state that the server processes and depends on, on client machines. An identifier to fetch it from the backend? Sure. Even with JWT, you're going to run head-first into the 4KB limit (which is down to 3 KB with base64 encoding, and even less with the HMAC).

You're literally cutting yourself off at the knees with this strategy.

Want to make everyone insecure because a few morons think storing session state on the client is okay? Do what you want; I look forward to seeing your spools on Full Disclosure.

[joepie91]

I think you meant the former simply doesn't belong in client data.

No, I meant it doesn't belong in session data, exactly as stated. You can keep state beyond session data, including on the client.

All I'm saying is, the "how do we do this securely?" is a separate question from "why/when/should someone do this?"

I still have to see them. Can you name one for me?

[mattsah]

@sarciszewski

I said "NO" because you shouldn't be storing session state that the server processes and depends on, on client machines.

This sounds more like a lack of proper deliniation between server state vs. client state. Based on the source you cited in OP, I'd agree. I was moreso referring to...

An identifier to fetch it from the backend? Sure.

So it sounds like you're OK with this, if implemented in a secure fashion.

Even with JWT, you're going to run head-first into the 4KB limit (which is down to 3 KB with base64 encoding, and even less with the HMAC).

In this specific case, I'd think that would depend on how much you're storing in session, which again, may be an issue of not properly dilineating.

Want to make everyone insecure because a few morons think storing session state on the client is okay?

No. But I agree that is what this particular example does.

[sarciszewski]

So it sounds like you're OK with this, if implemented in a secure fashion.

Yes, like PHPSESSID over HTTPS (assuming TLS v1.2). That's a secure implementation.

[mattsah]

@joepie91

No, I meant it doesn't belong in session data, exactly as stated.

Then (as with my previous comment to @sarciszewski), this sounds more like a deliniation issue.

I still have to see them. Can you name one for me?

I have no idea what this means in the context of the quote you quoted. Can I name one what? Are you asking if I can show you a specific example of an application running at that level of scale which benefits from client state actually being stored in the client? No. I don't know of a single one that's open source. Do they exist? Obviously, there were plenty of examples when I worked in Motorola.

[joepie91]

@mattsah Apologies, mis-quoted. Quote should have included:

There are cases for the latter, which means we need solutions for the former.

I'd like to see one of those cases.

[mattsah]

@sarciszewski

Yes, like PHPSESSID over HTTPS (assuming TLS v1.2). That's a secure implementation.

This is an avoidance of the issue, not an addressing of it. Again, no clear delineation between client state and server state. They're on one extreme (all server state is client state) and you're on the other (all client state is server state).

Specifically, I'm talking about storing a user token with information as to which user the client is authenticated (or even whether they are at all).

[mattsah]

@joepie91

I believe I responded to your statement as if that's what you were responding to. So, no, I have no publicly visible examples. They may exist, but I don't know every enterprise level web application that is open source. Maybe Bacula?

[sarciszewski]

Specifically, I'm talking about storing a user token with information as to which user the client is authenticated (or even whether they are at all).

Congratulations, you just reinvented PHP's session storage.

[mattsah]

Congratulations, you just reinvented PHP's session storage.

Sessions are stored serverside.

[sarciszewski]

Yes, the data is stored serverside, but the identifier is stored in a cookie.

[ffflabs]

I agree with @ircmaxell. We wouldn't be discussing any of this if SessionCookie middleware used json_decode to retrieve the key/value pairs of the encrypted cookie.

The cookie is encrypted to avoid someone tampering with its values, but it isn't meant to hold anything besides your logged status and user_id.

Sending this hash on each request, and having the server unencrypt it to know who you are is just as secure as sending an OAuth token to an API backend.

Sure, you can tamper with an OAuth token just as easily as you can tamper with a cookie. Hell, you might send whatever token you want and if you succeed in forging a token that amounts for another person you can impersonate them. Because that's how it works, actually.

[sarciszewski]

The cookie is encrypted to avoid someone tampering with its values

I still see a problem here. Encryption is not authentication. Since the session state is in the hands of the client, they get unlimited tries. It takes an average of 128 tries (worst case: 256) per byte to break CBC and forge the contents. Encrypt-Then-MAC or doom.

Sending this hash on each request,

Wait, is it encrypted or hashed?

@ircmaxell I see your point. If they can implement JWT competently (my history with other frameworks reinvention the session wheel has not made me optimistic about this) this issue can be considered closed.

[mattsah]

@ircmaxell

Yes. That's a good assessment, given that an actual compromise using unserialize() could result in arbitrary code execution on __wakeup() if I'm not mistaken.

[sarciszewski]

an actual compromise using unserialize() could result in arbitrary code execution on __wakeup() if I'm not mistaken.

Yeah, that's the crux of this issue.

[mattsah]

@sarciszewski just a point to note and for the slim team, JWT still requires separate encryption, initially it's just a hash with signature if I'm not mistaken. The data itself still needs to be encrypted.

[sarciszewski]

@mattsah If you use Firebase's implementation rather than rolling your own, then I won't complain. (Instead, I'll go review that.)

EDIT: Their library looks good at a glance.

[mattsah]

@sarciszewski I'm not on the slim team. If you'd like to review Firebase's implemention, go for it, but please take note of the above with respect to encryption. Firebase, so far as I know is just supplying stock JWT which gets around the unserialize()/__wakeup() issue and offers some data integrity. Encryption is still needed.

[sarciszewski]

Well, the develop branch has adopted Zend\Crypt so it should be fine. I haven't reviewed that library at all but others have.

[ffflabs]

I still see a problem here. Encryption is not authentication. Since the session state is in the hands
of the client, they get unlimited tries. It takes an average of 128 tries (worst case: 256) per byte
to break CBC | and forge the contents. Encrypt-Then-MAC or doom.

@sarciszewski so basically the problems are

• Encrypting with a weak algorithm
• Using unserialize on the decrypted cookie contents

I agree on those too, but that has no relation whatsoever with the pros or cons of using user provided data (cookies, headers, query string or POST params) to provide a stateful app.

[sarciszewski]

4096
- 64 (HMAC-SHA-256, hex-encoded)
4032
* 3/4 (base64)
3024
- 2 ('a=' for example)

3022

If you're fine only being able to store up to 3022 bytes of information in your session state of information per user ever, then by all means keep at it.

I'd recommend adopting a more modular approach. Let people choose their session backend, don't lock them into the cookie-based one. If they're fine storing it the default PHP way, it should be trivial to do so.

EDIT: Also, please don't use compression to try to get around this. https://www.isecpartners.com/blog/2012/september/details-on-the-crime-attack.aspx

[ffflabs]

But Slim does by no means force you to use SessionCookie middleware. The default implementation uses native session storage.

[sarciszewski]

The default implementation uses native session storage.

That wasn't obvious to me when I was looking at the code.

[ffflabs]

This is the part of the documentation that describes it.

[sarciszewski]

Oh, thank you. This is probably a lot less critical than I imagined, then.

[codeguy]

Just wanted to chime in here. First, I truly appreciate the scrutiny here. I really do, be it overly harsh or not. It only makes Slim better. Also, the 2.x code is not as up to date as I'd prefer. Neither is its documentation. I've been on hiatus so to speak for 1.5yrs... working on other things, writing a book, etc. But now I've returned and am moving fast on the upcoming 3.x release (the current develop branch). You can read details about the upcoming release here:

http://www.slimframework.com/2015/02/11/whats-up-with-version-3.html

It's getting better daily, and I'm trying to fix any issues as they appear. Most notably, I killed Slim's naive crypto implementation tonight and replaced it with Zend/Crypt. Anyway, thanks for the scrutiny.

As for the 2.x code, if you see anything that needs improved, fixed, etc. please send a pr against the master branch. This branch is only accepting hotfixes and security updates.

[sarciszewski]

Just wanted to chime in here. First, I truly appreciate the scrutiny here. I really do, be it overly harsh or not. It only makes Slim better.

Okay, that's basically the best response I could have asked for.

As for the 2.x code, if you see anything that needs improved, fixed, etc. please send a pr against the master branch. This branch is only accepting hotfixes and security updates.

I'll poke around. In the meantime, swapping out unserialize()/serialize() for the appropriate JSON features will stop the PHP Object Injection (which can result in remote code execution) vulnerability and you can focus on implementing JWT in develop.

EDIT: I've also edited the issue message to clarify my misunderstanding.

[ffflabs]

@codeguy I believe that there might be a problem when dealing with non-english characters (and storing a person's first or lastname in the session might be fairly common) resulting in null values. A safer solution could be

$data=$_SESSION;
if (mb_detect_encoding($data, 'UTF-8', true) !== 'UTF-8') {
    $data = utf8_encode($data);
}
$value = json_encode($data);

[ircmaxell]

utf8_encode isn't the answer to just about any question. It really doesn't do what most people think it does, and while it may prevent errors it doesn't do so in a valid way...

Suggest avoiding that issue altogether by simply mandating that storage be UTF-8 in the first place.

[sarciszewski]

@ircmaxell Would this do the trick?

    $from_type = mb_detect_encoding($data, 'UTF-8', true);
    if ($from_type !== 'UTF-8') {
        $data = mb_convert_encoding($data, 'UTF-8', $from_type);
    }

(I'm going to guess that the answer is "no", but I'm curious why it won't. Maybe I should make a SO post?)

[ircmaxell]

No. Encoding detection is incredibly unreliable at best.

[ffflabs]

I'm not aware of the inner defects of utf8_encode, but your comment sure makes me want to dig deeper on that.

However, replacing serialize with json_encode in the master branch might result in current users of Slim 2.5.x seeing their cookie persisted session returning null. Since json encoding non utf8 values doesn't throw any errors, they will be in a difficult position to debug the unexpected behavior. They might spin in circles before actually trying json_last_error().

[sarciszewski]

No. Encoding detection is incredibly unreliable at best.

Okay.

[ircmaxell]

@amenadiel utf8_encode() assumes that the input charset is LATIN-1. Which is quite often not the case.

In fact, all text is valid LATIN-1. This leads to the problem where codepoints are converted very weirdly (especially if source was UCS2-le - what MS likes to use in their OS's).

So significant information can be lost, as it transcodes the 127 LATIN-1 high-point characters to UTF-8. The reverse is even more damaging, since it tries to turn 2^21 into 2^7 (2^21 being approximately the number of possible UTF-8 codepoints).

[ffflabs]

However unreliable encoding detection may be, having your session data a bit scrambled is easier to debug than null data.

I'm really concerned on the consequences of rolling this change as is in the next minor release.

[sarciszewski]

I'm really concerned on the consequences of rolling this change as is in the next minor release.

Enjoy getting 0wned then.

Maybe wrap non-UTF8-encoded data in urlencode() and urldecode() before/after storage (respectively) if you're ever going to store it anywhere.