I'm not a cybersecurity expert but just a webmaster and I wonder if this kind of authentication would be dangerous. I test the password like this on the server side using PHP:

if (isset($_POST['pass_word']) AND $_POST['pass_word'] ==  $passwd)

$passwd comes from my PostgreSQL DB. I thought at least I don't risk SQL injection. Do I risk some other kind of injection? If it's a hazardous way to authenticate please explain why.

  • 5
    Regarding SQL injection - it is not possible to say if you are vulnerable or not just from looking at this snippet. – Anders Dec 29 '16 at 15:19
  • 36
    I don't know php, but it seems like you're not hashing passwords. If you're not hashing your authentication seems ok, but your users are at risk. An attacker may obtain access to your database by other meanings and steal user credentials in plaintext – Mr. E Dec 29 '16 at 15:37
  • 1
    If you have to ask, the answer is probably, "Yes." Unless it's just a standard way of implementing it. If you're a web master and not a dev, try to find a standard module that implements authentication for you. – jpmc26 Dec 29 '16 at 22:29
  • 2
    @DavidFoerster Sorry it's because in my country network security experts are called "experts cybersécurité" – kevin ternet Dec 30 '16 at 8:19
  • 1
    There's an established term "web security" or "websec" that refers specifically to web applications. – Kos Dec 31 '16 at 9:10
up vote 68 down vote accepted

This looks like you're storing passwords in the clear, which is a bad idea. You should ensure passwords are protected using, at minimum, password_hash() and password_verify(), which use bcrypt under the hood. This is simple, easy, safe, and perfectly acceptable for most scenarios.

Alternatively you can use a slightly more secure method such as Argon2, which won the Password Hashing Competition and is resistant to CPU and GPU cracking, and also aims to minimise the potential for side-channel attacks. There's an article which explains how to use Argon2 as part of libsodium's PHP wrapper, or directly using Paragon's "Halite" library, which offers Argon2 with symmetric encryption on top to prevent database-only access from providing usable hashes, due to the fact that the symmetric key is stored on the server's disk as a file. This option is more complicated, but it does offer some additional security if you're truly paranoid. I'd suggest avoiding this if you're unfamiliar with secure development, though, as the chances of you messing something up are increased.

I'd also recommend using === in order to avoid weird cases of false equality using arrays in URL queries or nulls.

  • 26
    md5('aabg7XSs') == md5('aabC9RqS') (-> true) is a good example why using == is a bad idea. – Arminius Dec 29 '16 at 15:30
  • 20
    @Arminius Good example. For others' benefit, this failure case occurs because the first byte of the resultant hash for each is 0e, which is considered to be a "float number format string" by PHP, and type coercion causes them to be compared as numbers. See here for more details. – Polynomial Dec 29 '16 at 15:38
  • 4
    We probably shouldn't be recommending using any sort of equality operator, since that opens you up to timing attacks. – Xiong Chiamiov Dec 29 '16 at 16:54
  • 2
    You may also want to explain why we use password hashing (to protect against leaked databases) or to link to some of the canonical answers on that, salts, etc. – Xiong Chiamiov Dec 29 '16 at 16:57
  • 3
    @jpmc26 The point of the example is that two different strings are apparently equal to php, which was news to me: var_dump('0e087386482136013740957780965295' == '0e041022518165728065344349536299'); outputs true. – SBoss Dec 30 '16 at 15:41

Polynomial's answer is spot on. I wanted to point out other potential vulnerabilities: operator precedence, ease of review, and ease of testing.

Something of the form foo AND bar == baz is vulnerable to getting the precedence wrong. I would scrutinize that in review, there's a long history of security measures being foiled because of compound conditions and precedence.

Now, there's nothing wrong with what you've written, but it's very easy to get it wrong. In most languages it's fine because comparison operators like == are higher precedence than logical operators like AND, so your condition reads as (note the explicit parens):

isset($_POST['pass_word']) AND
  ($_POST['pass_word'] ==  $passwd)

...but it's very easy to get it wrong. For example, AND and && do not have the same precedence. What if instead of using isset you (or someone coming along after you) used a shortcutting logical-or to set a default value? (This is not equivalent to isset, it's just an example of precedence causing security goofs)

$_POST['pass_word'] || '' ==  $passwd

This gotcha line is actually this:

$_POST['pass_word'] || ('' ==  $passwd)

Now anyone with a blank password, perhaps from an error retrieving it, will always login.

When it comes to secure code, make your parens explicit to make your intent known to the reviewer and to the program.


Better yet, avoid compound conditions entirely in secure code. You can't screw up using what you didn't use. For example, this code takes advantage of encapsulation and early return to provide very easy to review and test methods.

// This deals with input and normalizing it.
function can_login($user) {
    if( !isset($_POST['pass_word']) ) {
        // Or it could throw an exception to give the
        // caller more information about why they didn't login
        return false;
    }

    return check_password($user, $_POST['pass_word']);
}

// This only deals with checking a password.
// Don't use this, it still has all the flaws Polynomial
// pointed out.
function check_password($user, $check) {
    // get_password() should throw an exception if it cannot
    // find that user's password to avoid accidentally thinking
    // their password is false or 0 or '' or something. This
    // avoids relying on the caller doing the check for failure.    
    return $check === get_password($user);
}

It might not be the best method, but it is small and easy to test and review. This separates the details of logging a user in (which might be more than a password check, like are they a valid user) from checking their password. Any problems can be spotted and fixed without having to wade through the rest of the login code. A reviewer will check get_password and all the places check_password and can_login are used.

I cannot stress enough how important it is to isolate your secure components into as small a piece as possible, using as simple code as possible, to make them easy to review and test.


BONUS: Here's something I thought of doing, but decided it would make the code less secure.

My original idea was to have one function that returned multiple values: they didn't provide a password, their password was wrong, their password was right. This would allow the caller to know difference between "the password is wrong" and "they didn't provide a password, something is wrong".

function can_login($user) {
    if( !isset($_POST['pass_word']) ) {
        return NO_PASSWORD;
    }

    $password = get_password($user);
    if( $_POST['pass_word'] === $password ) {
        return YES;
    }
    else {
        return NO;
    }
}

And then you call it like so:

$rslt = can_login($user);

if( $rslt == YES ) {
    echo "Login!\n";
}
else if( $rslt == NO ) {
    echo "Wrong password.\n";
}
else if( $rslt == NO_PASSWORD ) {
    echo "No password given.\n";
}

The problem is it makes calling the routine properly more complicated. Worse, if you don't read the docs you'd think it was this:

if( can_login($user) ) {
    echo "Yes!\n";
}
else {
    echo "No!\n";
}

Since can_login always returns a true value, now anyone can login.

It drives home the point: make your secure code as simple and foolproof as possible for the reviewer, for the tester, and for the caller. In this case, exceptions are the right way to go as they provide a channel for getting more information while keeping can_login a simple boolean. If you forget to check the program might crash, but the login fails.

  • thank's for your response. Indeed I use an object (PHP Class) to load from database the count corresponding to login threw an ORM and then I compare password far from any sql statement. As you emphasis risk of being get wrong, I tried to inject in $_POST['pass_word'] something like 1' == '1' || 'password. But that didn't work. So that made me relax a little. – kevin ternet Dec 29 '16 at 20:21

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.