2
\$\begingroup\$

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:

enter image description here

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?

\$\endgroup\$
6
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ Thank you for your detailed response. In regards to injection, I've made an edit to my post to show snippets of how I check the inputs before passing onto validate_login_details() and thus MySQL - does this improve matters? I will study at the rest of your response tomorrow. Is there a strong preference in mysqli vsr PDO? \$\endgroup\$ – user602088 Mar 1 '15 at 18:46
  • \$\begingroup\$ @user602088 yes, that check does improve security (although I still don't like checking input anywhere but directly before database interaction, and I would always prefer prepared statements). But it's also quite restrictive (which might be fine, but people do have names with characters which a rejected by your regex). As for PDO vs MySQLi: I would choose PDO, as it's more flexible and offers more functionality, but you can just try them and choose the one you get along with better. Performance wise it doesn't seem to make a big difference (different benchmarks give different results). \$\endgroup\$ – tim Mar 1 '15 at 19:04
  • \$\begingroup\$ thanks Tim. do you see any issues with having the old Mysql functions in use on some scripts and PDO or MySQLi used in other scripts, as I migrate? \$\endgroup\$ – user602088 Mar 1 '15 at 19:31
  • \$\begingroup\$ @user602088 hm, good question. I think it should be fine, but I'm not actually sure. You might want to post a question at stackexchange (I think it would fit in there). \$\endgroup\$ – tim Mar 1 '15 at 19:35
  • \$\begingroup\$ Hi, I've gone to PDO now and it seems happy to work alongside mysql_* (on different scripts). I made some other changes based on your answer, if you like to see at the end of my question, thanks. \$\endgroup\$ – user602088 Mar 5 '15 at 17:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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