Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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));
    }
}
share|improve this question
add comment

2 Answers

up vote 2 down vote accepted

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.

share|improve this answer
    
Ah, damn! I was going to add a check to see if the session already existed, must've forgotten. Thanks for the points. :) –  user1813383 Nov 12 '12 at 21:32
add comment

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.

share|improve this answer
    
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 –  user1813383 Nov 12 '12 at 21:31
    
Don't use MD5, use Blowfish. –  user555 Nov 12 '12 at 21:35
    
@user555 so that's all it requires and I'll be (more or less) protected from attacks? That simple? o.o –  user1813383 Nov 12 '12 at 21:40
    
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. –  user555 Nov 12 '12 at 21:47
    
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. –  user1813383 Nov 12 '12 at 22:07
show 1 more comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.