PHP/MySQL login

Question

I just wanted to ask if this is a secure code (doesn't really matter if it's optimal or not)

The code

if(!$_SESSION['logged']){
 if(!$_POST['inputlogin']||!$_POST['inputpassword']){
 require 'cpanellogin.php';
 die();
 }
 else{
 $con=mysqli_connect("localhost","root","","librarydb");
 mysqli_query($con,'SET CHARACTER SET utf8');
 mysqli_query($con,'SET collation_connection = latin2_general_ci');
 $login = $_POST['inputlogin'];
 $password = $_POST['inputpassword'];
 $loginsquery = mysqli_query($con,"SELECT Konta_login, Konta_haslo FROM konta");
 while($row = mysqli_fetch_array($loginsquery))
 {
 $logins[] = $row['Konta_login'];
 $passwords[] = $row['Konta_haslo'];
 }
 $misslogin=0;
 for($i=0;$i<count($logins);$i++){
 if($login==$logins[$i]){
 if(MD5($password)==$passwords[$i]){
 $_SESSION['logged'] = $logins[$i];
 require 'cpanel.php';
 die();
 }
 else{
 $_POST['logerror'] = "Wrong password";
 require 'cpanellogin.php';
 die();
 }
 }
 else{
 $misslogin++;
 }
 if($misslogin==count($logins)){
 $_POST['logerror'] = "Wrong login";
 require 'cpanellogin.php';
 die();
 }
 }
 }
}

haslo means password

konta means accounts

• Dave: There is no reason to load all of the user names and passwords into an array and it would be a practice to do so. Change your query to only look for the user name entered then compare the hashed password in the database with the hashed password they entered. You'll want to use password_hash and password_verify instead of md5 for your passwords. MD5 is no longer considered secure.
• Imię Nazwisko: But is this code (besides MD5) any unsecure?
• Imię Nazwisko: @Dave there is no reason to load them all, but is there reason not to?
• Dave: It's a waste of resources at the very least and slows things down (albeit by a very small amount). There's just simply no reason to load them all anyway when your query can give you the data you actually need.
• Your Common Sense: @Dave callit a very small amount for a site with just 100000 users.

Answer 1

Security

Weak comparison

Unless there is a good reason, you never want to use ==, but ===.

With your code, passwords that aren't the same would be interpreted as being equal (eg md5('240610708') == md5('QNKCDZO') would be true).

MD5

md5 hasn't been secure for over a decade. There is really no good reason not to use secure hashing such as bcrypt, which PHP provides with password_hash.

Timing attacks

Remote timing attacks are mostly still theoretical, but why take the chance? You really want to use a function that compares the passwords in constant time (password_verify will do that).

Username enumeration

By giving out the information if a username exists or not, you give an attacker the option of bruteforcing usernames first, and only then bruteforcing passwords. This somewhat simplifies bruteforce attacks.

Instead, you want to give a generic error message (eg "The username/password combination is incorrect").

Other

Formatting

Your formatting could be improved to increase readability. You can just use any IDE to do this automatically for you. Some issues:

• not every statement needs its own paragraph! Instead, group statements into logical blocks.
• be consistent with spacing.
• be consistent with indentation and bracket placement.

Approach

The approach doesn't seem great. It's not just the loss of performance, but also the lack of readability.

Analyzing your code for security issues is much more complex than it would be if it were using the normal approach.

But even if for some reason you want to stick with the "read everything from the db and loop over it" approach (which I really would recommend against), you can improve it and simplify your loops a bit.

Something like this might work:

while($row = mysqli_fetch_array($loginsquery)) {
 if ($row['Konta_login'] === $login && MD5($password) === $row['Konta_haslo']) {
 // authenticate
 } else {
 // reject
 }
}

Or if you insist on storing the data in an array first:

while($row = mysqli_fetch_array($loginsquery)) {
 $credentials[$row['Konta_login']] = $row['Konta_haslo'];
}
if (isset($credentials[$login]) && MD5($password) === $credentials[$login]) {
 //authenticate
} else {
 // reject
}

But again, I would strongly suggest the usual approach of querying for the data you want using WHERE (using prepared statements of course) and then simply compare that, instead of getting all data.

• Imię Nazwisko: Thank you very much, that's a response I really wished to get. It's very helpful