3
\$\begingroup\$

I use this block of code to check if the user is logged in (login stored in cookies as a plain text and pass MD5 crypted). As I'm a novice in PHP, I'm not sure that this is a correct way. Are there other easier ways of checking that you use?

if(isset($_COOKIE['login']) AND isset($_COOKIE['pass'])){
    $login = $_COOKIE['login'];
    $pass =  $_COOKIE['pass'];
    if($qw->query("SELECT count(id) FROM users where password = '".$pass."' and  email = '".$login."';")->fetchColumn() == 0){
        setcookie('login','');
        setcookie('pass','');
        header('Location:auth.php');
    }

} else {
    setcookie('login','');
    setcookie('pass','');
    header('Location:auth.php');
    die();

}
\$\endgroup\$
2
\$\begingroup\$

Security

As others have mentioned, this is not secure at all.

Apart from the SQL injection (which can be fixed easily, your already using PDO anyways), the way your code works it doesn't matter that you are encrypting your passwords. You could change the weak md5 hashing to bcrypt or PBKDF2 and it wouldn't improve security one bit.(*)

An attacker doesn't have to bruteforce your hash, as they can simply log-in with the hash.

Cookies are stored and send by the user, and thus can be manipulated by an attacker. So if they gather a password hash, they can just set $_COOKIE['pass'] to it.

The solution is to use sessions, which are stored on the server.

(*) actually, users tend to reuse their passwords, so changing it would improve their security, but not the security of your server

Code

  • I would prefer email as the cookie name instead of login, making it clear what it should be
  • I would also prefer passwordHash instead of pass for the variable and cookie name.
  • You die if no cookies are set, but not if the check was unsuccessful. This doesn't seem right. If you redirect, always die.
  • try to avoid nested if-else statements.
  • you don't need to reset the cookie if it wasn't set in the first place.

Your code might then look something like this:

if(!isset($_COOKIE['email']) || !isset($_COOKIE['passwordHash'])){
    header('Location:yoursite.com/auth.php');
    die();
}
$email = $_COOKIE['email'];
$passwordHash =  $_COOKIE['passwordHash'];
if($qw->query("SELECT count(id) FROM users WHERE password = '" . $passwordHash . "' AND  email = '" . $email . "';")->fetchColumn() == 0){
    setcookie('login', '');
    setcookie('pass', '');
    header('Location:yoursite.com/auth.php');
    die();
}

If you want to avoid SQL injections, use something like this:

$query = $qw->prepare("SELECT count(id) FROM users WHERE password = :passwordHash AND  email = :email");
$query->bindParam(':passwordHash', $passwordHash);
$query->bindParam(':email', $email);
$queryResult = $query->execute()->fetch(PDO::FETCH_ASSOC);

And check out this link to read about using sessions to check if a user is logged in (instead of using cookies).

For better password hashing methods than md5, check out this post, especially the links on using bcrypt and pbkdf2 in PHP.

\$\endgroup\$
3
\$\begingroup\$

Are there other easier ways of checking that you use?

Any particular reason why you're using cookies to persist a user's authentication through every request?

I recommend using sessions to keep track of user authentications by storing its identifier and checking for its presence to know whether a user is already authenticated.

Using the MD5 algorithm for passwords is a bad idea, not because of its cryptographic weaknesses, but because it's fast. This means that an attacker can try billions of candidate passwords per second on a single GPU.

Did you know that PHP 5.5+ has the password_hash() function that takes care of all the troublesome details for you when it comes to creating secure password hashes?


  • I would use && as a convention instead of and if you're not utilizing its lower precedence. Better yet, you can do isset($_COOKIE['login'], $_COOKIE['pass']);. See the isset() manual.

  • When using PDO::query($statement) it requires you to properly escape all data to avoid SQL Injections and other issues. Use prepared statements to get rid of that.

  • Be consistent when writing code. If you like your SQL keywords uppercased, then uppercase them all, not just "most of them". It prevents potential confusion and it makes for code that's easier to read.

  • header(): "HTTP/1.1 requires an absolute URI as argument to » Location: including the scheme, hostname and absolute path, but some clients accept relative URIs.". So if you want to be safe you should use absolute paths.

  • Early exits will more often then not make your code cleaner and avoid nested if-statements that might hurt your code's readability.

Why are you setting the login and password cookies to an empty string regardless if the authentication has been successful or not? I would keep the cookies if the authentication was successful and ignore them if otherwise.

My 2 cents for now.

\$\endgroup\$
  • 1
    \$\begingroup\$ Pretty thorough "two cents" ;-) +1 \$\endgroup\$ – janos Sep 28 '14 at 8:59
1
\$\begingroup\$

This is not safe. Cookies are re-sent with every single HTTP request, until they expire, and they are visible to all computers the request travels through. Just because the password is in an encrypted form, that doesn't make it safe to share with the world, at all. An attacker who gets the encrypted password could crack it, by encrypting guesses and comparing with the encrypted password intercepted from the cookie. And of course they won't be guessing randomly, they will use the various password cracking tools and methods, such as dictionaries of known passwords, or brute force.

The bottom line is: it's not ok to leak passwords, not even in encrypted form.

The correct way to check if a user is still the same user who previously identified himself with a correct password, is using sessions. That is, upon correct login, you can store the username in the PHP session, and in subsequent requests you can check if the username is set in the session, and check if it matches with who the user claims himself to be. See the documentation of php sessions for more details, and google "php login session" for tutorials.

\$\endgroup\$
  • \$\begingroup\$ Why the downvote? \$\endgroup\$ – janos Sep 28 '14 at 17:31

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.