0

I have a problem with a bit of code and connat figure why it is going wrong

//Searches the database for the username
    $mysqli_username = mysqli_query($mysqli, "SELECT username FROM ajbs_users WHERE username=`".$username."`");
    // if name does not exist
    if ($mysqli_username == null)
    {
        echo "<p>Username was inccorect, or user does not exist</p>";
    }
    else //if username is correct
    {
        //finds and stores password of the user
        $mysqli_userPassword = mysqli_query($mysqli, "SELECT password FROM ajbs_users WHERE password=`".$password."`");
        if ($mysqli_userPassword != $password) //checks password matches from user
        {
            echo "<p>Password was inccorrect</p>";
        }
        else
        {
            //Login
            echo "<p>You have been logged in";
            // Re directs to other page after creating a session
        }
    }

The script keeps outputting the "Password is incorrect" statement.

Your Common Sense
  • 152,517
  • 33
  • 193
  • 313
Adam
  • 183
  • 1
  • 11
  • 3
    **PLEASE**, read the function description before use. It will take you just a few keystrokes: php.net/mysqli_query – Your Common Sense Nov 28 '13 at 12:42
  • 3
    `$mysqli_userPassword` contains a MySQL resource, **not a string**. You're comparing an result object with a string and the comparison will ALWAYS evaluate to `FALSE`. – Amal Murali Nov 28 '13 at 12:43

1 Answers1

2

There are a number of problems with your code:

  • mysqli_query() does not return rows from the database, it returns a resource from which you can fetch rows from the database. Or else it returns false if the query produces a parse error or an execution error. You need to check for these error cases.

    Or else use mysqli_report(MYSQLI_REPORT_STRICT) to configure mysqli to throw exceptions on error (if you know how to program with exceptions).

  • You are using the back-ticks around the values $username and $password. Back-ticks are for delimiting identifiers like table names and column names. For string values and date values, use single quotes. Double-quotes are used the same as single quotes by default, for strings and dates. But this can change if you set MySQL into ANSI mode, then double-quotes are used like back-ticks, for identifiers. See MySQL's different quote marks

  • You are interpolating variables directly into your SQL expressions, which is not safe unless you've been very careful to escape them. This is how SQL injection happens. Read What is SQL injection? and How can I prevent SQL injection in PHP? You should use prepared statements and add dynamic elements to the query using parameters.

    Keep in mind that if you use parameters, the quote issue disappears. You don't put any type of quotes around parameters. Using parameters is both easier and more secure than interpolating variables into strings.

And some problems with your logic or application design:

  • You checked if a username exists. Then if it does, you checked if a password exists. But in your code, the password is not necessarily used by the same username. Your code doesn't check that the same user has that password, only that any user has that password.

    What this means is that I can hack your site if I can register one username with password I know, say 'xyzzy'. Then I can log in as any other username, using password 'xyzzy'.

  • It's not a good security practice to tell the user whether the username or the password is incorrect. They should only be told that the login failed. If they're a hacker, it can be useful to them to know that they're on the right track, that is, they have chosen a valid username, they just need to guess the password. You might want to log the information for your own troubleshooting, but be more vague as you report to the user.

  • You shouldn't store a plaintext password. Store a hash of the password. Also store a random salt per user, and use that in the hash calculation. See You're Probably Storing Passwords Incorrectly and What data type to use for hashed password field and what length?

  • You can do this search with one query instead of two if you compare the username and password in one query. I search for the username in the WHERE clause, and then return the result of a boolean comparison of the stored password hash against the hash of the user's input.

So here's how I would write it, keeping in mind all the points from above.

/*
 * Search the database for the username, and report if the password matches
 */
$sql = "SELECT (password = SHA2(CONCAT(?, salt), 256)) AS password_matches
        FROM ajbs_users WHERE username = ?";
if (($stmt = $mysqli->prepare($sql)) === false) {
    trigger_error($mysqli->error, E_USER_ERROR);
}
$stmt->bind_param("ss", $password, $username);
if ($stmt->execute() === false) {
    trigger_error($stmt->error, E_USER_ERROR);
}

$stmt->bind_result($password_matches);
if ($stmt->fetch()) {
    if ($password_matches) {
        echo "<p>You have been logged in";
        // Re directs to other page after creating a session
    } else {
        error_log("Login attempted with user '$username', but password was incorrect.");
    }
} else {
    error_log("Login attempted with user '$username', but user does not exist.");
}

// Tell the user only what they need to know.
echo "<p>Login failed. Username or password was incorrect</p>";
Community
  • 1
  • 1
Bill Karwin
  • 462,430
  • 80
  • 609
  • 762
  • +1 Great answer! Is there any shortcut method to use the mysqli prepare statements, still keeping the code short? Any method that you suggest? – Kevin Nov 28 '13 at 18:00
  • 1
    @kevin, You still need to prepare() and execute() as separate steps. I prefer to use PDO, because there's no need for the bind_param() all. I can just pass the parameters as an array argument to execute(). See example #2 at PDOStatement::execute(). – Bill Karwin Nov 28 '13 at 18:04
  • 1
    @kevin surely, there is: https://github.com/colshrapnel/safemysql although data placeholders emulated, but it's as safe as native – Your Common Sense Nov 28 '13 at 18:04
  • there is no use for the while loop when no more than one row is expected – Your Common Sense Nov 28 '13 at 18:05
  • @YourCommonSense, but it does no harm, because the code will exit the loop on the first row 99.9% of the time -- and the loop handles an unexpected case, if there's more than one row in the database for a given username. The OP doesn't show his table definition so I'm not assuming there's a UNIQUE constraint. – Bill Karwin Nov 28 '13 at 18:10
  • 1
    this loop of yours is supposed to be iterated only **once** either way, according to "Redirects to other page" comment. Any operator that bloats code with no reason is harmful. – Your Common Sense Nov 28 '13 at 18:14
  • @YourCommonSense, fine, provided we can assume that the first row matched for the given username is the only row that is important to check for a matching password (I agree this is best design, but I couldn't make that assumption). I've edited the code above. – Bill Karwin Nov 28 '13 at 18:53