PHP user account login with salt

Question

I'm creating a user account login with PHP and Mysql which will be used by school teachers. To login there are 3 items required : school, username and password. This will allow for 2 of "MRSMITH" in two different schools.

The database is:

schools.login_name is the school part of the login
admin_users.username this is the teacher's username
admin_users.pass_hash(&salt) is the password and its salt



// salt / hash creation - returned SALT & HASH are inserted into user MySQL table 
function hash_password($password)
    {
    $return_array=array();

    $salt = mcrypt_create_iv(32, MCRYPT_DEV_RANDOM);
    $hexsalt = bin2hex($salt);

    $pass_hash = hash('sha256', $password . $hexsalt);

    $return_array['SALT'] = $hexsalt;
    $return_array['HASH'] = $pass_hash;
    return $return_array;
    }

(Note, the salt is unique and specific to each user if is is not clear from the above.)

/*
<input type='text' name='school_login_id'>
<input type='text' name='admin_login_id'>
<input type='password' name='admin_password'>

login form above, inouts from $_POST are checked for SQL inject before being passed to the function below to validate login

*/

function validate_login_details($school_login_id, $admin_login_id, $admin_password)
    {
    global $schools_table, $admin_users_table;
    $return_result = false; // invalid unless proven otherwise

    // find user and their pass hash and salt
    $query = "select S.school_id, pass_hash, pass_salt 
        from $schools_table as S join $admin_users_table as A using(school_id)
        where S.login_name = '$school_login_id' and A.username ='$admin_login_id'";

    log_this($query, "loginQueries.log"); // temp debug

    $login_res = query_db($query); // wrapper function for MYSQL_QUERY - yes, i will stop using this i promise!
    if (mysql_num_rows($login_res) == 1) 
        {
        $db_pass_hash = mysql_result($login_res, 0, "pass_hash");
        $db_pass_salt = mysql_result($login_res, 0, "pass_salt");


        // re-create hash from supplied password and compare to d/b
        $hash_from_entered_pass = hash('sha256', $admin_password . $db_pass_salt);
        if ($hash_from_entered_pass == $db_pass_hash) 
            {
            $return_result = true;
            }
        }
    else
        {
        log_this("mysql_num_rows(\$login_res) not 1 row", "loginfails.log"); // temp debug
        }

    return $return_result;
    }

Before data is passed to validate_login_details() it is checked against these regexps:

School and user login ID:

    if (preg_match("/[^-a-z0-9_]/i", $text) )
        {
            $error_count++;
        }

Password:

$match_list = "\x{20}-\x{5f}\x{61}-\x{7e}"; // basic ascii chars excluding backtick
$match_list .= "\x{a1}-\x{ff}"; // extended latin 1 chars excluding control chars
$match_list .= "\x{20ac}\x{201c}\x{201d}"; // euro symbol & left/right double quotation mark (from Word)
$match_list .= "\x{2018}\x{2019}"; // left/right single quotation mark (from word)

if (preg_match("/[^$match_list]/u", $text) )
    {
    $error_count++;
    }

Is this code correct and secure? I have seen many more complex has generation functions, but given that there are no credit cards or personal details involved, is this robust enough?

Answer 1

Security

Is this code correct and secure?

Probably not.

EDIT: I just read your comments about the input being checked elsewhere (which might invalidate my answer somewhat, depending on how you check the user input). You should really check user input right before database interaction. Otherwise it will become unmaintainable really quick (you will always ask yourself if you already checked it, and you will inevitably forget to do it in some places, as you falsely believe you already checked them).

SQL Injection

You are missing the part where you actually retrieve user input, but the code you did post looks pretty open to SQL injection (which in general means that an attacker can for example bypass a login, can steal data from the database, and possibly write to a file/read from a file, change data, etc).

You should really stop using mysql immediately, and you should start using prepared statements (either with mysqli or PDO).

Concrete Attacks

For your concrete query, it might not look like an attacker can do all that much with this injection (the data is not displayed, and the login actually cannot be bypassed). But that is actually not true, as blind SQL injection is possible if the attacker knows a valid school and login id (blind sql injection means that you ask true/false questions, eg "is the first character of the first user in ascii smaller than 100? yes? is it also smaller than 60?", and so on; this does generate quite a bot of requests, but getting the database user or admin is certainly doable in short amount of time). You should be able to test this by injecting $school_login_id = valid id and $admin_login_id = valid id ' and 1=1 # (you might need a valid password as well) and then doing the same thing again with 1=2, which should give you a different result (meaning you can post yes/no questions to the database).

If the database user is allowed to write to a file, that would also be a possible attack vector.

Also, it should be possible to login as any user if the attacker knows the password of a specific user (by database id). An injection could look like this: $school_login_id = desired school name, $admin_login_id = desired user id' or 1=1 limit 1,1 # with the password not of the desired user, but of the first user in the database (this becomes a lot easier if you expose database ids to endusers).

To summarize: If you do not use prepared statements (or at least something) to defend against SQL injections, there will always be a way to exploit it.

Hashing, Comparison, Misc

given that there are no credit cards or personal details involved, is this robust enough?

For password hashing, bcrypt is generally recommended over simple one-round sha256 hash. Weak hashing might actually be ok for your security requirements, but as bcrypt is actually easier to use in PHP (it manages salts for you), and is more secure, why not use it?

Also, for hash comparisons, you should not use ==, but a timing safe function such as hash_equals to avoid timing attacks.

Also, currently it looks as if your query logs might be in a public directory. If so, that is a really bad idea (exposes database names, etc; invalid logins might be mistypes, also exposes valid logins).

Misc

• be consistent with your names. Eg $school_login_id vs S.login_name (is it an integer, as id implies, or a string as name implies?).
• your indentation is off, which makes it really hard to see what else belongs to what if.
• I would get rid of $return_result. Just return false at the end, and in the middle return $hash_from_entered_pass == $db_pass_hash directly.
• global variables generally hint towards bad design.