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

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.