Are there any security issues to avoid when providing a either-email-or-username-can-act-as-username login system?

Question

I am in the process of moving from a "username/password" system to one that uses email for login. I don't think that there's any horrible problem with allowing either email or username for login, and I remember seeing sites that I consider somewhat respectable doing it as well, but I'd like to be aware of any major security flaws that I may be introducing.

More specifically, here is the pertinent function (the query_row function parameterizes the sql).

function authenticate($p_user, $p_pass) {
    $user        = (string)$p_user;
    $pass        = (string)$p_pass;
    $returnValue = false;

    if ($user != '' && $pass != '') {
        // Allow login via username or email.
        $sql = "SELECT account_id, account_identity, uname, player_id 
                FROM accounts 
                JOIN account_players ON account_id=_account_id 
                JOIN players ON player_id = _player_id 
                WHERE lower(account_identity) = lower(:login) 
                      OR lower(uname) = lower(:login) 
                      AND phash = crypt(:pass, phash)";
        $returnValue = query_row($sql, array(':login'=>$user, ':pass'=>$pass));

    }
    return $returnValue;
}

Notably, I have added the WHERE lower(account_identity) = lower(:login) OR lower(uname) = lower(:login) ...etc section to allow graceful backwards compatibility for users who won't be used to using their email for the login procedure. I'm not completely sure that that OR is safe, though. Are there some ways that I should tighten the security of the php code above?

Answer 1

Well, I don't see any particular harm in doing both email/username pairs. At the end of the day, using the email as login is just like having a different username, nothing more nothing less. Just ask the users to provide a screen name, you don't want to show their email if they login with that.

As for your code, you should really do a mysql_real_escape_string or similar on $user to avoid SQL injection (unless the query_row function does that for you). You can also escape $pass although no injection is possible there, as it's going to be hashed.

As far as I know the OR statement does not pose any particular harm.

Answer 2

Well, after formatting your query, it become clear to me that your OR is unsafe.
Make it

WHERE (lower(account_identity) = lower(:login) OR lower(uname) = lower(:login)) 
      AND phash = crypt(:pass, phash)";

Note the parenthesis around OR clause.