Crypt() salt generation and password encryption, well executed?

Jursels picture Jursels · Dec 4, 2013 · Viewed 7.1k times · Source

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;
        }
    }
}

Answer

martinstoeckli picture martinstoeckli · Dec 4, 2013

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.