Storing passwords with sha1 and salt

Question

I have a simple registration script done in php and I was just curious if the way I am doing it is secure enough to store user passwords. I am generating a 32bit random salt and appending it to an sha1 hashed password.

//create new validator object
    $validator = new data_validation();
    //validate user input
    $firstName = $validator->validate_fname($firstName); //is the first name a string?
    $lastName = $validator->validate_lname($lastName); // is the last name a string?
    $username = $validator->validate_username($username); // is the username a string?
    $email = $validator->validate_email($email); //is the email in valid format?

    //make sure there isn't duplicate emails
    $valQuery = $link->query("SELECT email FROM users WHERE email = '" .$email. "'");

    if ($valQuery->num_rows == 1) {
        echo "An email is already registered with that address";
        return false;
    }

    // generate a random salt for converting passwords into sha1
    $salt = $link->real_escape_string(bin2hex(mcrypt_create_iv(32, MCRYPT_DEV_URANDOM)));
    $saltedPW =  $password . $salt;
    $hashedPW = sha1($saltedPW);

    mysqli_connect($db_host, $db_user, $db_pass) OR DIE (mysqli_error());
    // select the db
    mysqli_select_db ($link, $db_name) OR DIE ("Unable to select db".mysqli_error($db_name));

     // our sql query
    $sql = "INSERT INTO users (first_name, last_name, username, email, password, salt) VALUES ('$firstName', '$lastName', '$username', '$email', '$hashedPW', '$salt');";

    //save the updated information to the database          
    $result = mysqli_query($link, $sql) or die("Error in Query: " . mysqli_error($link));

    if (!mysqli_error($link)) 
    {
        $row = mysqli_fetch_assoc($result);
        $_SESSION['user_id'] = $row['user_id'];
        $_SESSION['loggedin'] = TRUE;
        header("Location: ../home");
    }

Also, I am using a combination of procedural and oop php. Most of it is done in procedural, but there are a few oop classes such as the validation class you see used in the above script. Will this cause any performance issues using both styles?

Solution 1

No. Stop what you're doing, read How to securely hash passwords, then read Secure hash and salt for PHP passwords:

Most importantly:

• Use scrypt when you can; bcrypt if you cannot.
• Use PBKDF2 if you cannot use either bcrypt or scrypt.

See this answer for a comparison of PBKDF2, bcrypt and scrypt.

Also refer to the often-linked article How To Safely Store A Password:

[MD5, SHA1, SHA256, SHA512, SHA-3, etc] are all general purpose hash functions, designed to calculate a digest of huge amounts of data in as short a time as possible. This means that they are fantastic for ensuring the integrity of data and utterly rubbish for storing passwords.

PHPass is probably the easiest way to do bcrypt hashing in PHP. You can also do it the hard way using the crypt function and CRYPT_BLOWFISH if you want, but be aware that there's a lot of ways to get it wrong, and the interface is fairly arcane (like how you specify salt values).

Solution 2

Switching between procedural and OO in itself will not affect performance. The overhead of loading and instantiating classes is negligable. However, the managing a non-OO codebase that grow can be a non-negligible task--especially juggling everything in global namespace.

Adding an additional field to your insert (the salt) also will not affect anything. Using the salt does not add overhead to the sha1 algorithm by tacking it to the end of the password.

I am a little confused at how you choose to generate the random salt, but it doesn't look very system intensive either.