0

I have been developing a login library for a website using CodeIgniter. The authentication code is as follows:

function signin($username, $password)
{
    $CI =& get_instance();
    $query_auth=$this->db->query('SELECT user_id, banned FROM user WHERE username=? AND password=SHA1(CONCAT(?,salt)) LIMIT 1', array($username, $password));

    if($query_auth->num_rows()!=1)
        return 2;
    else
    {
        if($query_init->row()->banned==1)
            return 3;
        else
        {
            $CI->load->library('session');
            $this->session->set_userdata('gauid', $query_auth->row()->user_id);
            return 1;
        }
    }
}

The return values signifying success, failure or banned. Each user has a unique salt stored in the database.

Originally i grabbed the salt from the database, combined the users inputted password and salt from the database in PHP, then queried the database again with the combined value. I thought that this would speed things up as only one trip to the database is required and there is less code. I also thought that it would be equally secure, however after reading the top reponse to this question Salting my hashes with PHP and MySQL ...

First of all, your DBMS (MySQL) does not need to have any support for cryptographic hashes. You can do all of that on the PHP side, and that's also what you should do.

...I started to wonder if there was a security problem i had neglected to spot.

Is there actually anything wrong this code?

Community
  • 1
  • 1
j82374823749
  • 847
  • 4
  • 15
  • 24
  • possible duplicate of [Secure hash and salt for PHP passwords](http://stackoverflow.com/questions/401656/secure-hash-and-salt-for-php-passwords) – rook Jul 19 '10 at 19:34
  • I don't see how that's true! The questions are completely different. – j82374823749 Jul 19 '10 at 19:44
  • Then it should be closed for being too localized. – rook Jul 19 '10 at 19:46
  • I'm sorry, i don't understand what you mean. Can you please explain? – j82374823749 Jul 19 '10 at 19:48
  • @briggins5 On average there are about 3 questions a week relating to slating passwords. If you think there is a security problem then you should read one of these posts instead of adding more redundancy. – rook Jul 19 '10 at 19:50
  • What I actually wanted to know was whether there was a particular security issue relating to executing the single SQL statement shown above. – j82374823749 Jul 19 '10 at 19:54

1 Answers1

2

Not anything wrong per se. Keep in mind any traffic carrying the unencrypted/unhashed password is suspect. So, for instance, when the server is a remote one, and not working with encryption in communicating with that server, it is yet another moment to try to intercept a password. Also, if queries are logged somewhere (either by default, or because they're slow), you have a plain password + the salt you're using sitting in those serverlogs, after all the trouble you went through NOT to store a plaintext password somewhere. If you did it privately in your own code, that wouldn't happen.

It all depends on how paranoid you like to be. There are far easier to abuse and often forgotten evils, like session-fixation.

Wrikken
  • 69,272
  • 8
  • 97
  • 136
  • Sorry, pet peeve on per se. :) – Joe Mastey Jul 19 '10 at 19:33
  • As long as you're thinking through security issues like this already, I wouldn't call the logging of all password attempts in plaintext as a minor thing. Otherwise agreed as above. – Joe Mastey Jul 19 '10 at 19:34
  • @Joseph Mastey & @The Rook: you're both right (albeit on entirely different topics, this just saves time :) ) – Wrikken Jul 19 '10 at 19:51