0

I used this hash function for a while (got it from the internet). The thing is it used to work immediately but now it is complaining about a paramter. The following is the code:

function generateHash($plainText, $salt = null)
{

    if ($salt === null)
    {
        $salt = substr(md5(uniqid(rand(), true)), 0, SALT_LENGTH);
    }
    else
    {
        $salt = substr($salt, 0, SALT_LENGTH);
    }
    return $salt . sha1($salt . $plainText);
}

So I would use this code in the method call:

validateUserInput($userid, $pass);

and validateUserInput is:

function validateUserInput($username, $password)
{
    //$username = mysql_real_escape_string($username);
    //$password = mysql_real_escape_string($password);

    if(!$username || !$password)
    {
        //$errors['credentials'] = 'Missing Credentials!';
        //$_SESSION['errors_array'] = $errors;
        //echo $errors['credentials'];
        header("LOCATION:XXXXXXX.php");
    }

    $local_salt = generateHash($password);
    //echo $local_salt;
    $groupid;

    if($username != null && $password !=null)
    {   
        connectToServer();
        $result = mysql_query("SELECT * FROM users WHERE hashkey = '{$local_salt}'");

        while($row_access = mysql_fetch_array($result))
        {
            $groupid = $row_access['groupid'];
        }
        if(!isset($result))
        {
            $errors['not_found_user'] = 'No Users Found with Provided Credentials!';
            //$_SESSION['errors_array'] = $errors;
            $userfound = 0;
            $_SESSION['user_available'] = $userfound;
        }elseif(isset($result)){
            $_SESSION['user_logged'] = array('username' => $username, 'password' => $password, 'salt' => $local_salt, 'groupid' => $groupid);
            $userfound = 1;
            //echo "stored";
            $_SESSION['user_available'] = $userfound;
        }       
    }
}

finally the error is:

Warning: substr() expects parameter 3 to be long, string given in /home/XXXX.php on line 64

This is pointing to the function generateHash()

0

3 Answers 3

6

The error itself tells you everything. The constant SALT_LENGTH is not a long. I suspect it's not defined at all, so PHP converts the bare string to a string ("SALT_LENGTH") and passes that to substr(), which complains.

That being said... This code is dangerously wrong:

  1. if(!isset($result)): Really? This condition will always be false because $result will always be set (unless you run into a problem with mysql_query(), but that doesn't tell you anything about the valididty of the login). And since mysql_query() never returns null, no logins will ever be rejected.

  2. This query:

    SELECT * FROM users WHERE hashkey = '{$local_salt}'
    

    Is invalid. $local_salt = generateHash($password);. From the generateHash function, if a salt is not given, one will be randomly created for you. Thus, every call to generateHash will generate a new hash, which means it can't be compared to anything in the database.

On the basis of the two (very) egregious mistakes above, I would throw away this piece of code for good.


The correct way to check for a valid hash when a salt is used is something like:

$_SESSION['user_logged'] = null;

// fetch hashed pw from db, where username is the submitted username
$result = mysqli_query("SELECT hash FROM users WHERE username = '{$username}'");

if ($result->num_rows != 0) 
{
     $row = $result->fetch_assoc();    
     $hash = $row['hash'];

     $salt = substr($hash, 0, SALT_LENGTH); // extract salt 

     if (generateHash($password, $salt) == $hash) 
     {
         // login successful. 
         $_SESSION['user_logged'] = $username; // don't store passwords here
     }
}

// if $_SESSION['user_logged'] is not set, the login failed
if (!isset($_SESSION['user_logged'])) 
{
    // you *don't* want to tell people which one (login or pw) is invalid
    echo 'Invalid login or password';
}

Note: It's very important that the SALT_LENGTH is at most 32, or this won't work because of the way generateHash() is implemented.

6
  • I appreciate the comments. Thanks and will work on it. Nonetheless, your comment is not related to my question which is why would a piece of code that always worked stopped working!
    – sys_debug
    Oct 3, 2011 at 6:11
  • @sys_debug Read the first paragraph. I am guessing it was working before because you had SALT_LENGTH defined in another file, which is now missing for whatever reason. Regardless, I am pointing out that even if it was "working" before, it wasn't doing the right thing. Oct 3, 2011 at 6:16
  • aha thanks and really appreciate the help. Changing it now. Making sure the pitfalls u outlined above are avoided :)
    – sys_debug
    Oct 3, 2011 at 6:23
  • ok before thinking about your implementation i did the following: (please inform me if it may work or not)
    – sys_debug
    Oct 3, 2011 at 6:42
  • let us continue this discussion in chat Oct 3, 2011 at 6:43
2

Clearly SALT_LENGTH is not an integer. Find where it's defined and correct this.

1
  • yes I know but it used to work all the time! I tried defining it by taking the strlen(salt) but didn't work because the salt is null when it enters the function and is generated in the function.
    – sys_debug
    Oct 3, 2011 at 6:06
1

Instead of making a new hashing function each time you write an application , you should utilize the one provided to you by php: crypt() ( i would recommend to go with any blowfish or sha256+ hash ).

And when selecting information from database, you should select it by username and then, with php, check if hash fits the password.

2
  • I already started doing this now...half way through it...and as u said, I am now checking username, then verifying the password hashed
    – sys_debug
    Oct 3, 2011 at 6:24
  • @sys_debug , oh , and look into PDO and prepared statements. Firstly because mysql_* function are ancient and soon gonna be deprecated, and secondly - if used correctly , prepared statements make it almost impossible to have an SQL injection in your code. Lets put it this way: you need a special kind of talent to make create an sql vulnerability, when using prepared statements.
    – tereško
    Oct 3, 2011 at 6:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.