The problem

What's wrong with this code snippet?



session_create(); /* ... */ $username = $_POST['username']; $password = $_POST['password']; $_SESSION['username'] = $username; if (scrypt($saved_password, $saved_hash) == scrypt($password, $saved_hash)) { $perm = load_permissions_from_db($username); $_SESSION['permissions'] = $perm; redirect_to_home(); } else { session_destroy(); }

The problem is that scrypt() might allocate memory and trigger PHP's OOM handler:[1]



PHP Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 133958362 bytes)

This is the really bad part: PHP will stop execution, but still write out the current $_SESSION data when this happens.



In the above example, this would leave a PHP session sitting around with a 'username' property, but no 'permissions' property. If that were a list of negative permissions, or an empty list got interpretted as full control, that user would have access beyond his normal rights.



Even worse, an attacker could have a session with 'username' set to whatever she wished, even if the password was never correct.



If there is anything earlier in the program that the user could control to allocate memory, she could gradually increase the memory footprint until the scrypt() command hit the OOM condition.



Remediation

I was hoping to be able to make some kind of "critical section" by disabling the write handler for the session before doing something dangerous and turning it on only after completion. However, on the PHP version I tested against (and according to the documentation) that all has to be set before you begin a session. (It's possible that suhosin is screwing with my attempts to do this.)



In theory even something as simple as $_SESSION['foo'] = bar could trigger the OOM.



You can make your own critical section handler by setting $_SESSION['valid'] = false before updating any session variables, and setting it back to true when you are done. Then on every session_start() you destroy any session that is invalid.



You should also think hard about where a hostile user could cause your PHP app to use a lot of memory. Suhosin does do a good job limiting a lot of $_GET, $_POST, and $_SERVER variables. The more likely culprit would be your processing of some kind of data under user control. If you are, say, analyzing XML under the user's control, they could insert a billion laughs to cause you to run out of memory, and then carefully reduce the size of the input until they get your app to crash in just the right place.



This is also a reminder to practice good session hygiene: session IDs should only be created by the system; when a user logs out, or attempts to log in, her old session should be completely destroyed and her cookie expired.



History

I asked the PHP team about this and was told:



So there is no bug per se here, security related or normal bug.

Increase or disable the memory limit to solve this problem.

Which seems to miss the point but there you go.



Notes