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.
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.
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.
