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.

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
    
->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 at 16:36

6 Answers 6

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 at 19:03
1  
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 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 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. –  DJDavid98 Aug 5 at 19:59
    
@DJDavid98 adding that feature allows for username harvesting, and is frowned upon in general (OWASP etc.) –  konijn Aug 5 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 at 20:07
    
@tim Hilarious, I never considered that. –  konijn Aug 5 at 20:10

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 for password_hash() / password_verify(). It's too bad we can't all be on 5.5 yet though... –  Schism Aug 14 at 14:09
    
That's what password_compat is for. If you run PHP < 5.5.0, it polyfills it. –  Scott Aug 14 at 14:14

There are a few things I would like to point out:

  • I don't think authentication is the responsibilty of the User class. You should consider moving it to an Autheticate class or something.

  • As already suggested, definitly add some salt! Or even better, move to mcrypt like you already mentioned. Just make sure you do this before adding users to the db, or your passwords will become worthless!

  • I'm a big fan of never nesting if's, and avoid the use of else. This can often be accomplished with early returns. I believe it makes the code a lot more readable. If you really have to nest, you are probably doing something wrong (single responsibility?). Exceptions do exist off course.

  • What exactly is the use of that 'remember' cookie? You are just storing some random value in there... I suppose it is for auto login, but how are you ever going to validate it?

  • On construct of your user you provide a $db variable, but in fact you could provide whatever value you want there, which would brake your code later. Consider using strong typing.

This is what my code would look like:

class User {
  // the data object
}

class UserMapper implements DataObjectMapperInterface{
  // takes care of the storage.

  public function __construct(DatabaseInterface $db) { ... }

  public function getByEmail($email) { ... }
}

class Authentication {
   public function login($email, $pass, $remember) {
      $mapper = new UserMapper();
      $user = $mapper->getByEmail($email);

      if (! $user || $user->password !== $this->_getPasswordHash($pass) ) {
          return false; // let your controller or view decide what message to return
      }     

      $_SESSION['loggd_id'] = $user->id;

      if($remember) {
          // as said, not sure what you are doing here...
          setcookie('lggd_sess', hash('sha512', uniqid()), 84600);
      }

      return true;
   }

   protected function _getPasswordHash($pass) {
      // you'll be needing this when creating new users, so better to keep
      // the logic in one place
      return sha1('somesaltthatcouldcomefromanywhere' + $pass);
   }
}
share|improve this answer
    
I actually forgot to write down the remember line which saves the hash in the DB. And about your code, i liked it. It's clear and easy to understand :-) –  TrueDevE Aug 5 at 19:37
1  
-1 for antiquated password hashing scheme. Seriously? Sha1 + static salt???? Use password_hash and be done with it. –  ircmaxell Aug 14 at 13:59
    
@ircmaxell true, but password_hash is only available since 5.5 and I did mention he should move to mcrypt, which is basically what the password_hash function uses currently. –  PeterVR Aug 14 at 22:16
1  
@PeterVR no, he most certainly should not migrate to mcrypt. password hash uses crypt() under the hood. Not mcrypt. They are VERY different things (one is two-way-encryption, and the other is one-way-password-hashing). See this answer. And yes, while password_hash is 5.5+, there is a compatibility library linked in the official php.net documentation that works with 5.3.7+. Either way, suggesting sha1, salted or not (or peppered as in your case), is simply irresponsible in this day in age... –  ircmaxell Aug 15 at 16:20
    
I should also point out that your current implementation of _getPasswordHash() returns mostly the same hash, i.e. sha1(0). –  Jack Aug 16 at 0:21

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 at 18:52
1  
-1. Escape on output (display), not on input. –  ircmaxell Aug 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.