4

these are some functions I am using for password encryption and password verification. Was wondering if this is a good way to handle it. I am using the codeigniter framework.

This is the function to 'encrypt' :

function crypt_pass( $input ){
    $salt = substr(sha1(date('r')), rand(0, 17), 22);
    $cost = 10;
    $hash = '$2y$' . $cost . '$' . $salt;

    $pw_and_salt['pw'] = crypt($input, "$hash");
    $pw_and_salt['salt'] = $salt;

    return $pw_and_salt;
}

I store both the password and the salt in my DB. Here is the login function:

function login(){

    $this->db->select('salt');
    $salt = $this->db->get_where('users', array('username' => $this->input->post('username') ) )->row();



    $where = array(
        'username' => $this->input->post('username'),
        'password' => crypt( $this->input->post('password'), '$2y$10$' . $salt->salt),
    );


    $user = $this->db->get_where('users', $where)->first_row();

    if (!$user) {
        return FALSE;
    }else{
        if(!empty($user->activation)){

            return 2;

        }else if($user && empty($user->activation)){
            $this->session->set_userdata('id',$user->id);
            $this->session->set_userdata('username',$user->username);
            $this->session->set_userdata('first_name',$user->first_name);   

            return 1;
        }
    }
}

Am I implementing this the right way? Is this enough security?

VERSION 2 : NOT STORING SALT, EXTRACTING FROM PASSWORD IN DB INSTEAD :

function login(){

    $this->db->select('password');

    $pw = $this->db->get_where('users', array('username' => $this->input->post('username') ) )->row();


    $where = array(
        'username' => $this->input->post('username'),
        'password' => crypt( $this->input->post('password'), $pw->password),
    );

    $user = $this->db->get_where('users', $where)->first_row();

    if (!$user) {

        return FALSE;

    }else{

        if(!empty($user->activation)){

            return 2;

        }else if($user && empty($user->activation)){

            $this->session->set_userdata('id',$user->id);
            $this->session->set_userdata('username',$user->username);
            $this->session->set_userdata('first_name',$user->first_name);   

            return 1;
        }
    }
}
jww
  • 97,681
  • 90
  • 411
  • 885
Jursels
  • 173
  • 1
  • 3
  • 12
  • 1
    I think this question would be better on the [code review](http://codereview.stackexchange.com/) stack. – Taylan Aydinli Dec 04 '13 at 06:04
  • Also see Openwall's [PHP password hashing framework](http://www.openwall.com/phpass/) (PHPass). Its portable and hardened against a number of common attacks on user passwords. The guy who wrote the framework (SolarDesigner) is the same guy who wrote [John The Ripper](http://www.openwall.com/john/) and sits as a judge in the [Password Hashing Competition](http://password-hashing.net/). So he knows a thing or two about attacks on passwords. – jww Oct 12 '14 at 01:51
  • This question appears to belong on another site in the Stack Exchange network. Perhaps you should try [Code Review Stack Exchange](http://codereview.stackexchange.com/). – jww Oct 12 '14 at 01:54
  • $pw_and_salt['pw'] = crypt($input, "$hash"); why $hash is in quotes? It should not be, right? – Boris Gafurov Sep 27 '20 at 17:35

1 Answers1

6

There are some points that can be improved, but first i would recommend to use PHP's new function password_hash(). This function will generate a safe salt and includes it in the resulting hash-value, so you can store it in a single database field. There exists also a compatibility pack for earlier versions.

// Hash a new password for storing in the database.
// The function automatically generates a cryptographically safe salt.
$hashToStoreInDb = password_hash($password, PASSWORD_BCRYPT);

// Check if the hash of the entered login password, matches the stored hash.
// The salt and the cost factor will be extracted from $existingHashFromDb.
$isPasswordCorrect = password_verify($password, $existingHashFromDb);

Some thoughts about your code:

  1. You generate a BCrypt hash with crypt(), so the salt will be part of the resulting hash. There is no need to store it separately.
  2. The generation of the salt can be improved, use the random source of the operating system MCRYPT_DEV_URANDOM.
  3. If you would change the cost factor to 9, the format would become invalid, because crypt expects two digits.
martinstoeckli
  • 23,430
  • 6
  • 56
  • 87
  • Hi Martin, thanks for your input. I will certainly be using your suggestions. One question on the current setup, if I am not mistaken I can only verify an entered password by hashing it in the same way I hashed the password stored in the DB, how can I do this without actaully storing and retrieving the salt? – Jursels Dec 05 '13 at 00:46
  • I added my approach for validation without a salt from DB to the original post. – Jursels Dec 05 '13 at 02:27
  • @Jursels - The salt is already included in the resulting hash-value, and the password_verify() function will extract it from there, together with the cost factor. Example: the hash-value `$2y$10$nOUIs5kJ7naTuTFkBy1veuK0kSxUFXfuaOKdOKf9xYT0KKIGSJwFa` contains the salt after the third `$`, the salt is `nOUIs5kJ7naTuTFkBy1veu`. – martinstoeckli Dec 05 '13 at 07:50