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
[1] There is no scrypt()
in the PHP library right now, but in theory what any scrypt()
does is check a password via a method that purposefully uses a lot of memory to make things harder on people brute forcing passwords. There are lots of other functions that could allocate memory, but I chose this one because the irony of using a lot of memory as a security feature can cause other problems here.
Your code should be default deny, certainly an empty list should not be interpreted as 'allow all permissions'
ReplyDeleteI agree with the problem being in the code, why do you set $_SESSION['username'] = $username; before the scrypt() call?
ReplyDeleteOf course when writing secure PHP code you should assume that execution may be interrupted at any point.
Of course when writing secure PHP code you should assume that execution may be interrupted at any point.
ReplyDeleteIs this in the PHP documentation somewhere and I missed it?
The sum of php.net's discussion of sessions and security seems to be http://us2.php.net/manual/en/session.security.php.
PHP materials is a beautiful module which is full of many wonderful and attractive codes where the developer would visit there.The beauty of the China attract many visitors across the world and many visitors have come for spending their vacation.This edition book is becoming more and more popular among the people. This boomessays review will provide some important information the PHP module.
ReplyDeleteHave you ever thought about including a little bit more than just your articles? I mean, what you say is valuable and everything. However think about if you added some great graphics or video clips to give your posts more, "pop"! Your content is excellent but with pics and clips, this site could definitely be one of the very best in its niche. Very good blog!
ReplyDeleteNice blog here! Also your website loads up fast! What host are you using? Can I get your affiliate link to your host? I wish my site loaded up as quickly as yours lol
ReplyDelete