benedmunds / CodeIgniter-Ion-Auth

Simple and Lightweight Auth System for CodeIgniter
http://benedmunds.com/ion_auth/
MIT License
2.34k stars 1.14k forks source link

hash_password_db doesn't do what it says #226

Closed matalina closed 12 years ago

matalina commented 12 years ago

/ * This function takes a password and validates it * against an entry in the users table. * @return void \ @author Mathew / public function hash_password_db($id, $password, $use_sha1_override=FALSE) {

The function doesn't do what it says it does if you are using sha1 encryption. It returns the hashed password instead of true or false like the bcrypt part of the function does.

I have corrected it in my part and when I get time I can correct it but I don't know when that will be so I'm posting it if someone else has the time to check it out.

benedmunds commented 12 years ago

This works fine. For me, are you just saying that both types of encryption should return the same way?

matalina commented 12 years ago

Yes. The description says it compares, it's not doing that for sha1 encryption. If you switch encryption methods your script breaks.

SHA1 returns the encrypted password that you sent but does not compare. Bcrypt returns true or false returning results of the comparison. It says it compares. It should work the same way for both methods of encryption.

benedmunds commented 12 years ago

What do you mean by "If you switch encryption methods your script breaks"? It works for me both ways, you just have to update your password in the database.

matalina commented 12 years ago

I started using Bcrypt but the uri string was giving me fits so I went back to sha1 (this was before I found out there was an updated to the token codes for activation/lost password etc)

Basically my script that broke because the function doesn't do what it says checks to see if the default password is still set for a first time user:

// Has the user changed their password?
    $user_id = $this->session->userdata('user_id');
    if($this->ion_auth->hash_password_db($user_id,'password') && $this->uri->segment(1) != 'profile') {
      $this->session->set_flashdata('warning','<div class="alert-box warning">You must change your password.</div>');
      redirect('profile/information');
    }

With bcrypt the function worked correctly because the bcrypt portion returned true or false. With sha1 it returns the password that was to be decoded which results in an always true statement so any page move will continually tell the user to change their password even if they have a billion times.

I think you are thinking of hash_password NOT hash_password_db

Below is the hash_password and my modified hash_password_db functions

/**
     * Hashes the password to be stored in the database.
     *
     * @return void
     * @author Mathew
     **/
    public function hash_password($password, $salt=false)
    {
        if (empty($password))
        {
            return FALSE;
        }

        //bcrypt
        if ($this->hash_method == 'bcrypt')
        {

            if ($this->random_rounds)
            {
                $rand = rand($this->min_rounds,$this->max_rounds);
                $rounds = array('rounds' => $rand);

            }
            else
            {
                $rounds = array('rounds' => $this->default_rounds);
            }

            $CI=& get_instance();
            $CI->load->library('bcrypt',$rounds);
            return $CI->bcrypt->hash($password);
        }

        if ($this->store_salt && $salt)
        {
            return  sha1($password . $salt);
        }
        else
        {
            $salt = $this->salt();
            return  $salt . substr(sha1($salt . $password), 0, -$this->salt_length);
        }
    }

    /**
     * This function takes a password and validates it
     * against an entry in the users table.
     *
     * @return void
     * @author Mathew
     **/
    public function hash_password_db($id, $password)
    {
        if (empty($id) || empty($password))
        {
            return FALSE;
        }

        $this->trigger_events('extra_where');

        $query = $this->db->select('password, salt')
                          ->where('id', $id)
                          ->limit(1)
                          ->get($this->tables['users']);

        $hash_password_db = $query->row();

        if ($query->num_rows() !== 1)
        {
            return FALSE;
        }

        // bcrypt
         if ($this->hash_method == 'bcrypt')
        {
            $CI=& get_instance();
            $CI->load->library('bcrypt',null);

            if ($CI->bcrypt->verify($password,$hash_password_db->password))
            {
             return TRUE;
            }
             return FALSE;
        }

        if ($this->store_salt)
        {
            $db_password = sha1($password . $hash_password_db->salt);
        }
        else
        {
            $salt = substr($hash_password_db->password, 0, $this->salt_length);

            $db_password =  $salt . substr(sha1($salt . $password), 0, -$this->salt_length);
        }

    if($db_password == $hash_password_db->password) {
      return TRUE;
    }
    else {
      return FALSE;
    }
    }

This is my modified hash_password_db

The original is:

/**
     * This function takes a password and validates it
     * against an entry in the users table.
     *
     * @return void
     * @author Mathew
     **/
    public function hash_password_db($id, $password)
    {
        if (empty($id) || empty($password))
        {
            return FALSE;
        }

        $this->trigger_events('extra_where');

        $query = $this->db->select('password, salt')
                          ->where('id', $id)
                          ->limit(1)
                          ->get($this->tables['users']);

        $hash_password_db = $query->row();

        if ($query->num_rows() !== 1)
        {
            return FALSE;
        }

        // bcrypt
         if ($this->hash_method == 'bcrypt')
        {
            $CI=& get_instance();
            $CI->load->library('bcrypt',null);

            if ($CI->bcrypt->verify($password,$hash_password_db->password))
            {
             return TRUE;
            }
             return FALSE;
        }

        if ($this->store_salt)
        {
            return sha1($password . $hash_password_db->salt);
        }
        else
        {
            $salt = substr($hash_password_db->password, 0, $this->salt_length);

            return $salt . substr(sha1($salt . $password), 0, -$this->salt_length);
        }
    }
benedmunds commented 12 years ago

OK thanks. Now I think we are finally on the same page. I'll work up a fix for this and update this issue when it's ready.

For now you can compare it against the hashed value of the password.

-Ben Edmunds

On Friday, May 18, 2012 at 1:16 PM, Alicia wrote:

I started using Bcrypt but the uri string was giving me fits so I went back to sha1 (this was before I found out there was an updated to the token codes for activation/lost password etc)

Basically my script that broke because the function doesn't do what it says checks to see if the default password is still set for a first time user:

// Has the user changed their password?
 $user_id = $this->session->userdata('user_id');
 if($this->ion_auth->hash_password_db($user_id,'password') && $this->uri->segment(1) != 'profile') {
 $this->session->set_flashdata('warning','<div class="alert-box warning">You must change your password.</div>');
 redirect('profile/information');
 }

With bcrypt the function worked correctly because the bcrypt portion returned true or false. With sha1 it returns the password that was to be decoded which results in an always true statement so any page move will continually tell the user to change their password even if they have a billion times.

I think you are thinking of hash_password NOT hash_password_db

/**
 * Hashes the password to be stored in the database.
 *
 * @return void
 * @author Mathew
 **/
 public function hash_password($password, $salt=false)
 {
 if (empty($password))
 {
 return FALSE;
 }

 //bcrypt
 if ($this->hash_method == 'bcrypt')
 {

 if ($this->random_rounds)
 {
 $rand = rand($this->min_rounds,$this->max_rounds);
 $rounds = array('rounds' => $rand);

 }
 else
 {
 $rounds = array('rounds' => $this->default_rounds);
 }

 $CI=& get_instance();
 $CI->load->library('bcrypt',$rounds);
 return $CI->bcrypt->hash($password);
 }

 if ($this->store_salt && $salt)
 {
 return sha1($password . $salt);
 }
 else
 {
 $salt = $this->salt();
 return $salt . substr(sha1($salt . $password), 0, -$this->salt_length);
 }
 }

 /**
 * This function takes a password and validates it
 * against an entry in the users table.
 *
 * @return void
 * @author Mathew
 **/
 public function hash_password_db($id, $password)
 {
 if (empty($id) || empty($password))
 {
 return FALSE;
 }

 $this->trigger_events('extra_where');

 $query = $this->db->select('password, salt')
 ->where('id', $id)
 ->limit(1)
 ->get($this->tables['users']);

 $hash_password_db = $query->row();

 if ($query->num_rows() !== 1)
 {
 return FALSE;
 }

 // bcrypt
 if ($this->hash_method == 'bcrypt')
 {
 $CI=& get_instance();
 $CI->load->library('bcrypt',null);

 if ($CI->bcrypt->verify($password,$hash_password_db->password))
 {
 return TRUE;
 }
 return FALSE;
 }

 if ($this->store_salt)
 {
 $db_password = sha1($password . $hash_password_db->salt);
 }
 else
 {
 $salt = substr($hash_password_db->password, 0, $this->salt_length);

 $db_password = $salt . substr(sha1($salt . $password), 0, -$this->salt_length);
 }

 if($db_password == $hash_password_db->password) {
 return TRUE;
 }
 else {
 return FALSE;
 }
 }

This is my modified hash_password_db

The original is:

/**
 * This function takes a password and validates it
 * against an entry in the users table.
 *
 * @return void
 * @author Mathew
 **/
 public function hash_password_db($id, $password)
 {
 if (empty($id) || empty($password))
 {
 return FALSE;
 }

 $this->trigger_events('extra_where');

 $query = $this->db->select('password, salt')
 ->where('id', $id)
 ->limit(1)
 ->get($this->tables['users']);

 $hash_password_db = $query->row();

 if ($query->num_rows() !== 1)
 {
 return FALSE;
 }

 // bcrypt
 if ($this->hash_method == 'bcrypt')
 {
 $CI=& get_instance();
 $CI->load->library('bcrypt',null);

 if ($CI->bcrypt->verify($password,$hash_password_db->password))
 {
 return TRUE;
 }
 return FALSE;
 }

 if ($this->store_salt)
 {
 return sha1($password . $hash_password_db->salt);
 }
 else
 {
 $salt = substr($hash_password_db->password, 0, $this->salt_length);

 return $salt . substr(sha1($salt . $password), 0, -$this->salt_length);
 }
 }

Reply to this email directly or view it on GitHub: https://github.com/benedmunds/CodeIgniter-Ion-Auth/issues/226#issuecomment-5791011

matalina commented 12 years ago

I modified my copy of the model to make the comparison.

benedmunds commented 12 years ago

If you have a working copy of this change any chance you could send a pull request? Thanks!

-Ben Edmunds

On Tuesday, May 22, 2012 at 3:11 PM, Alicia wrote:

I modified my copy of the model to make the comparison.

Alicia Wilkerson Email/Gtalk: matalina@gmail.com (mailto:matalina@gmail.com) MSN: matalina@dragonmount.com (mailto:matalina@dragonmount.com) Yahoo!: matalina_gaidin AIM: avenderosa

On Tue, May 22, 2012 at 1:54 PM, Ben Edmunds < reply@reply.github.com (mailto:reply@reply.github.com)

wrote:

OK thanks. Now I think we are finally on the same page. I'll work up a fix for this and update this issue when it's ready.

For now you can compare it against the hashed value of the password.

-Ben Edmunds

On Friday, May 18, 2012 at 1:16 PM, Alicia wrote:

I started using Bcrypt but the uri string was giving me fits so I went back to sha1 (this was before I found out there was an updated to the token codes for activation/lost password etc)

Basically my script that broke because the function doesn't do what it says checks to see if the default password is still set for a first time user:

// Has the user changed their password?
 $user_id = $this->session->userdata('user_id');
 if($this->ion_auth->hash_password_db($user_id,'password') &&
$this->uri->segment(1) != 'profile') {
 $this->session->set_flashdata('warning','<div class="alert-box
warning">You must change your password.</div>');
 redirect('profile/information');
 }

With bcrypt the function worked correctly because the bcrypt portion returned true or false. With sha1 it returns the password that was to be decoded which results in an always true statement so any page move will continually tell the user to change their password even if they have a billion times.

I think you are thinking of hash_password NOT hash_password_db

/**
 * Hashes the password to be stored in the database.
 *
 * @return void
 * @author Mathew
 **/
 public function hash_password($password, $salt=false)
 {
 if (empty($password))
 {
 return FALSE;
 }

 //bcrypt
 if ($this->hash_method == 'bcrypt')
 {

 if ($this->random_rounds)
 {
 $rand = rand($this->min_rounds,$this->max_rounds);
 $rounds = array('rounds' => $rand);

 }
 else
 {
 $rounds = array('rounds' => $this->default_rounds);
 }

 $CI=& get_instance();
 $CI->load->library('bcrypt',$rounds);
 return $CI->bcrypt->hash($password);
 }

 if ($this->store_salt && $salt)
 {
 return sha1($password . $salt);
 }
 else
 {
 $salt = $this->salt();
 return $salt . substr(sha1($salt . $password), 0, -$this->salt_length);
 }
 }

 /**
 * This function takes a password and validates it
 * against an entry in the users table.
 *
 * @return void
 * @author Mathew
 **/
 public function hash_password_db($id, $password)
 {
 if (empty($id) || empty($password))
 {
 return FALSE;
 }

 $this->trigger_events('extra_where');

 $query = $this->db->select('password, salt')
 ->where('id', $id)
 ->limit(1)
 ->get($this->tables['users']);

 $hash_password_db = $query->row();

 if ($query->num_rows() !== 1)
 {
 return FALSE;
 }

 // bcrypt
 if ($this->hash_method == 'bcrypt')
 {
 $CI=& get_instance();
 $CI->load->library('bcrypt',null);

 if ($CI->bcrypt->verify($password,$hash_password_db->password))
 {
 return TRUE;
 }
 return FALSE;
 }

 if ($this->store_salt)
 {
 $db_password = sha1($password . $hash_password_db->salt);
 }
 else
 {
 $salt = substr($hash_password_db->password, 0, $this->salt_length);

 $db_password = $salt . substr(sha1($salt . $password), 0,
-$this->salt_length);
 }

 if($db_password == $hash_password_db->password) {
 return TRUE;
 }
 else {
 return FALSE;
 }
 }

This is my modified hash_password_db

The original is:

/**
 * This function takes a password and validates it
 * against an entry in the users table.
 *
 * @return void
 * @author Mathew
 **/
 public function hash_password_db($id, $password)
 {
 if (empty($id) || empty($password))
 {
 return FALSE;
 }

 $this->trigger_events('extra_where');

 $query = $this->db->select('password, salt')
 ->where('id', $id)
 ->limit(1)
 ->get($this->tables['users']);

 $hash_password_db = $query->row();

 if ($query->num_rows() !== 1)
 {
 return FALSE;
 }

 // bcrypt
 if ($this->hash_method == 'bcrypt')
 {
 $CI=& get_instance();
 $CI->load->library('bcrypt',null);

 if ($CI->bcrypt->verify($password,$hash_password_db->password))
 {
 return TRUE;
 }
 return FALSE;
 }

 if ($this->store_salt)
 {
 return sha1($password . $hash_password_db->salt);
 }
 else
 {
 $salt = substr($hash_password_db->password, 0, $this->salt_length);

 return $salt . substr(sha1($salt . $password), 0, -$this->salt_length);
 }
 }

Reply to this email directly or view it on GitHub: https://github.com/benedmunds/CodeIgniter-Ion-Auth/issues/226#issuecomment-5791011


Reply to this email directly or view it on GitHub:

https://github.com/benedmunds/CodeIgniter-Ion-Auth/issues/226#issuecomment-5855539


Reply to this email directly or view it on GitHub: https://github.com/benedmunds/CodeIgniter-Ion-Auth/issues/226#issuecomment-5855949

matalina commented 12 years ago

I haven't had time to add it to the clean copy as I got hit with a project that is a top priority and needs to get finished. Soon as I get a moment and get the code into an editor I can send a pull request.

benedmunds commented 12 years ago

Thank you!

-Ben Edmunds

On Wednesday, May 23, 2012 at 1:55 PM, Alicia wrote:

I haven't had time to add it to the clean copy as I got hit with a project that is a top priority and needs to get finished. Soon as I get a moment and get the code into an editor I can send a pull request.


Reply to this email directly or view it on GitHub: https://github.com/benedmunds/CodeIgniter-Ion-Auth/issues/226#issuecomment-5880049

matalina commented 12 years ago

Pull request sent off.

benedmunds commented 12 years ago

Merged.