Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I've been working on a simple login script. The registration part is still to be coded, but I'm done the login part. Are there any security holes in my code? I'm using SHA-512 right now but I think I'll change it to mcrypt later.

Class User
{
    private $db;

    public function __construct($db)
    {
        $this->db = $db;
    }

    public function Login($user_email, $user_pass, $remember = false)
    {
        $user_pass = sha1($user_pass);
        $query = $this->db->prepare("SELECT id, password FROM users WHERE email = :email");
        $query->bindValue(':email', $user_email);
        $query->execute();

        if($query->rowCount() > 0) {
            $user = $query->Fetch(PDO::FETCH_OBJ);
            if($user->password == $user_pass) {
                $_SESSION['loggd_id'] = $user->id;
                if($remember == true) {
                    setcookie('lggd_sess', hash('sha512', uniqid()), 84600);
                    return true;
                }
                return true;
            } else {
                return 'Incorrect Email/Password Combination.';
            }
        } else {
            return 'Incorrect Email/Password.';
        }

    }
}
share|improve this question
1  
->rowCount() on SELECT statements don't work reliably across PDO backends. You can just use ->fetch() and check whether the result is false or not. – Jack Aug 15 '14 at 16:36

I'm using SHA-512 right now but I think I'll change it to mcrypt later.

Neither of those are the proper tool for the job. Mcrypt is meant for symmetric encryption (AES, TwoFish, DES, etc.), not password hashing.

If you're running PHP 5.5+, use password_hash() and password_verify()

Otherwise, https://github.com/ircmaxell/password_compat is the way to go.

EDIT: Example code:

<?php
Class User
{
    private $db;

    public function __construct($db)
    {
        $this->db = $db;
    }

public function Login($user_email, $user_pass, $remember = false)
{
        $query = $this->db->prepare("SELECT id, password FROM users WHERE email = :email");
        $query->bindValue(':email', $user_email);
        $query->execute();

        if($query->rowCount() > 0) {
            $user = $query->Fetch(PDO::FETCH_OBJ);
            if (password_verify($user_pass, $user->password)) {
                $_SESSION['loggd_id'] = $user->id;
                if($remember == true) {
                    setcookie('lggd_sess', hash('sha512', mcrypt_create_iv(64, MCRYPT_DEV_URANDOM)), 84600);
                    return true;
                }
                return true;
            } else {
                return 'Incorrect Email/Password Combination.';
            }
        } else {
            return 'Incorrect Email/Password.';
        }
    }
}

When you insert a row in the DB, you want to use password_hash($user_password)

share|improve this answer
1  
+1 for password_hash() / password_verify(). It's too bad we can't all be on 5.5 yet though... – Schism Aug 14 '14 at 14:09
1  
That's what password_compat is for. If you run PHP < 5.5.0, it polyfills it. – Scott Arciszewski Aug 14 '14 at 14:14

Ok, let's go through this line by line, and talk about it:

Class User
{
    private $db;

    public function __construct($db)
    {
        $this->db = $db;
    }

    public function Login($user_email, $user_pass, $remember = false)
    {
        $user_pass = sha1($user_pass);

OK, let's stop right here. First off, sha1 is not acceptable for any cryptographic usage.

Second, neither is any other "normal" hash function. You need to salt and stretch the password hash so that it's difficult to brute force. You really should be using bcrypt to hash the passwords.

How do you use BCrypt?.

In modern versions of PHP, use password_hash (built in with 5.5+, and I maintain a compatibility library for 5.3.7+. In that case, the line would be removed, since the test would happen later.

        $query = $this->db->prepare("SELECT id, password FROM users WHERE email = :email");
        $query->bindValue(':email', $user_email);
        $query->execute();

You're using prepared statements! EXCELLENT.

        if($query->rowCount() > 0) {
            $user = $query->Fetch(PDO::FETCH_OBJ);
            if($user->password == $user_pass) {

With password_hash, this line would become if (password_verify($user_pass, $user->password)) {. Simple, easy and secure.

                $_SESSION['loggd_id'] = $user->id;
                if($remember == true) {
                    setcookie('lggd_sess', hash('sha512', uniqid()), 84600);

uniqid should never be used for security. It's predictable, and gives very little protection against brute forcing.

Instead, you want to use a random token that's generated from /dev/urandom or a similar strong source. I maintain 2 libraries that abstract this process RandomLib and RandomCompat.

Using RandomLib, the hash would be replaced with $factory = new RandomLib\Factory; $generator = $factory->getMediumStrengthGenerator(); $token = $generator->generateString(32);. Which would generate a 32 character token using a-zA-Z0-9.

Additionally, how is the remember me token supposed to work if you're not actually storing the value of the token anywhere except in the cookie...???

                    return true;
                }
                return true;
            } else {
                return 'Incorrect Email/Password Combination.';
            }
        } else {
            return 'Incorrect Email/Password.';

There's no practical need to distinguish between an email that you couldn't find, and a password being incorrect. The security it adds is 0 (since you'll likely have registration checks that will leak the existence of an email anyway). So for usability purposes, I'd recommend specifying which field was incorrect...

        }

    }
}
share|improve this answer
  • It appears you're storing the password in your database without salting your hash. Adding a user-by-user salt will improve the security of your code.

  • This code block has a bunch of extra stuff:

    if($remember == true) {
        setcookie('lggd_sess', hash('sha512', uniqid()), 84600);
        return true;
    }
    return true;
    

    In particular, the return statement is duplicated. You can just do this instead:

    if ($remember) {
        setcookie('lggd_sess', hash('sha512', uniqid()), 84600);
    }
    return true;
    
  • Just gonna point out that the manual page for uniqid() explicitly states:

    Warning This function does not create random nor unpredictable strings. This function must not be used for security purposes. Use a cryptographically secure random function/generator and cryptographically secure hash functions to create unpredictable secure IDs.

share|improve this answer
    
Have a question, does that matter to use a strong hash in CSRF protections or in remember me cookies? Because why would a attacker dcrypt it? – TrueDevE Aug 5 '14 at 19:03
2  
The hash function is meaningless. The important thing is random entropy to remove the ability for an attacker to guess a valid session id (or remember me token). If they can guess one, it's game over (session hijacking, etc). So the problem isn't hash, but uniqid... – ircmaxell Aug 14 '14 at 14:21

You should read up on using salts, your approach is too simplistic.

Your use of returns seems too complex though, can I counter-propose something like

public function Login($user_email, $user_pass, $remember = false)
{
    $user_pass = sha1($user_pass);
    $query = $this->db->prepare("SELECT id, password FROM users WHERE email = :email");
    $query->bindValue(':email', $user_email);
    $query->execute();

    if($query->rowCount() > 0) {
        $user = $query->Fetch(PDO::FETCH_OBJ);
        if($user->password == $user_pass) {
            $_SESSION['loggd_id'] = $user->id;
            if($remember == true) {
                setcookie('lggd_sess', hash('sha512', uniqid()), 84600);
            }
            return true;
        }
    }
    return 'Incorrect Email/Password Combination.';
}
share|improve this answer
    
Yea....this one is better – TrueDevE Aug 5 '14 at 18:58
    
I - personally - hate when websites tell me that either my e-mail or my password is wrong. It should be perfectly doable to detect which is missing and return accordingly. – SeinopSys Aug 5 '14 at 19:59
1  
@DJDavid98 adding that feature allows for username harvesting, and is frowned upon in general (OWASP etc.) – konijn Aug 5 '14 at 20:03
2  
@konijn that's true, and I would follow that advice. But usernames can generally also be harvested from the registration page (there, they have to tell you that the username already exists). – tim Aug 5 '14 at 20:07
    
@tim Hilarious, I never considered that. – konijn Aug 5 '14 at 20:10

Only thing I could think of adding right now would be,

if (isset($user_pass)) {
    $user_pass = htmlspecialchars($user_pass);
    $user_pass = preg_replace('/\s+/', '', $user_pass);
}
if (isset($user_email)) {
    $user_email = htmlspecialchars($user_email );
    $user_email = preg_replace('/\s+/', '', $user_email);
}

Even though you prepare your statement and bind your variable, it doesn't hurt to escape any characters that may break your query.

Also, consider removing all unnecessary white space?

share|improve this answer
    
Yup actually those have been added now in my code just have query about session and cookie. I'm bit doubt about it. – TrueDevE Aug 5 '14 at 18:52
1  
-1. Escape on output (display), not on input. – ircmaxell Aug 14 '14 at 13:57

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.