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.

[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.