Is this session handler secure?

Question

Are there any security vulnerabilities in this code?

class Session {
 private $id = array();
 private $data = array();
 private $name = 'session';
 public function __construct() {
 session_start();
 global $db;
 $this->id = isset($_COOKIE[$this->name]) ? $_COOKIE[$this->name] : $this->create_id();
 $id = $this->id;
 $ip = $_SERVER['REMOTE_ADDR'];
 $user_agent = $_SERVER['HTTP_USER_AGENT'];
 $check = $db->query("SELECT session_data FROM sessions WHERE session_id = ? AND user_ip = ? AND user_agent = ?");
 $db->execute(array($id, $ip, $user_agent));
 if ($db->row_count() != 0) {
 $this->data = $db->fetch_column();
 $this->data = unserialize($this->data);
 } else {
 $this->id = $this->create_id();
 $id = $this->id;
 $data = serialize($this->data);
 $time = time();
 $ip = $_SERVER['REMOTE_ADDR'];
 $user_agent = $_SERVER['HTTP_USER_AGENT'];
 $current_page = $_SERVER['REQUEST_URI'];
 $insertion_data = array(
 'session_id' => $id,
 'session_data' => $data,
 'session_last_activity' => $time,
 'user_ip' => $ip,
 'user_agent' => $user_agent,
 'user_current_page' => $current_page
 );
 $db->insert('sessions', $insertion_data);
 }
 }
 public function end() {
 global $db;
 $this->garbage();
 $id = $this->id;
 $data = serialize($this->data);
 $time = time();
 $ip = $_SERVER['REMOTE_ADDR'];
 $user_agent = $_SERVER['HTTP_USER_AGENT'];
 $current_page = $_SERVER['REQUEST_URI'];
 $name = $this->name;
 // note to self: create update function in db class
 $db->query("UPDATE sessions SET session_data = ?, session_last_activity = ?, user_ip = ?, user_agent = ?, user_current_page = ? WHERE session_id = ?");
 $db->execute(array($data, $time, $ip, $user_agent, $current_page, $id));
 @setcookie($name, $id, time()+3600, '/', '');
 }
 public function create_id() {
 return strtoupper(md5(rand(-1000000, 1000000)));
 }
 public function garbage() {
 global $db;
 $expiration = time() - '2419200';
 $db->query("DELETE FROM sessions WHERE session_last_activity < ?");
 return $db->execute(array($expiration));
 }
 public function regenerate() {
 global $db;
 $old_id = $this->id;
 $new_id = $this->create_id();
 $this->id = $new_id;
 $db->query("UPDATE sessions SET session_id = ? WHERE session_id = ?");
 return $db->execute(array($new_id, $old_id));
 }
}

Answer 1

Rand() is not random

You should be aware that rand() is not as random as one might think. What if you create a session id that already exists? Consider replacing rand() or checking if the session id already exists.

Bruteforce

Your session id is too easy to bruteforce. A session id should be treated as a password from a security point of view. The longer and more complex the session id is, the harder it is to bruteforce. Use session_regenerate_id() as it will give you a fairly good alpha-numeric ID.

• user1813383: Ah, damn! I was going to add a check to see if the session already existed, must've forgotten. Thanks for the points. :)

Answer 2

Nope. Let's walk through it from a hypothetical attackers point of view.

Okay. It's sending an identification cookie. It's hex. 32 digits long. Probably an md5 hash. I wonder what's hashed? I'll throw it in oclHashcat to check. I can make a billion guesses a second, so if it's a hash of something simple, I'll find out.

<3 seconds later>

Huh. It's a number. Lemme grab another session and see if that's a number too.

Yep.

It looks like these are all 6 digit numbers. I'll grab 10 more to check.

Okay, 6 digits AND they can be positive or negative.

That's a keyspace of about 2^21. If there are 8 sessions going right now, then it costs, on average,

2^(21 - 1 - 3) requests * 1 KB per request * 0.08 $/GB
...
=1.01 cents to crack a session. Alright, I'll start on that and see what I have in the morning.

So: Add more entropy.

• user1813383: Thanks, definitely useful! Could you be so kind as to give a bit more detail on how to prevent this? I understand what's going on, but not how to stop it from occurring. :S
• user555: Don't use MD5, use Blowfish.
• user1813383: @user555 so that's all it requires and I'll be (more or less) protected from attacks? That simple? o.o
• user555: Also use a unique random salt. This will prevent rainbow attacks and make the cracking more expensive. See "Don't use MD5" under my answer.
• user1813383: I'm slightly confused. I did a little research on the subject, multiple websites said that encrypting the session ID is pointless. The MD5 is only there to make the random string 32 chars in length.