4
\$\begingroup\$

For a very simple databaseless website, I'm using an associative array to store logins like this:

$logins = array(
'user' => 'password',
'user2' => 'password2',
);

I check login attemps using this code:

$user = strtolower($_POST['username']);
$pass = $_POST['password'];
if(!isset($logins[$user]) or $logins[$user] != $pass){
    // failed login
}else{
    $_SESSION['username'] = $user;
}

What would the security vulnerabilities in this be?

\$\endgroup\$
10
\$\begingroup\$

In this response, I assume that you are making a little script for yourself and some friends, i.e. not a large or critical system (e.g. that could delete other files). So you want some security, but not to extreme measures.

Passwords in Plain Text

As pacifist mentioned, storing passwords as clear text is always a bad idea. If you're just making a little page for yourself and some friends, at least store the passwords hashed with a salt (a random string per user). You could then store your login data the following way:

$logins = array(
 'user' => array('hash' => 'ae092...', 'salt' => 'a7#(ad~I$...'),
);

Checking for a valid login would then be used the following way (using PHP's crypt() to make SHA-256 hashes):

// Added trim() so something like "user " becomes "user"
$user = strtolower(trim($_POST['user']));
$pass = $_POST['password'];

if(isset($logins[$user]) && crypt($logins[$user]['salt'].$pass, 'SHA-256') == $logins[$user]['hash']) {
   // valid login!
}

This is not much more difficult and already provides you a little more safety. This is still not sufficiently safe for critical applications – you can read all about password hashing over at Security StackExchange. The linked answer covers a lot of aspects and is a great starting point.

Again, the above code is a simple way of handling logins for a small, non-critical page.

At the very worst, if you can't be bothered to give an individual salt to every user, add a global prefix/suffix to each password – a so-called pepper. You'll still be better off (preventing online rainbow table look-ups), because it doesn't take any knowledge at all to search the SHA-256 hash of "admin" and look up the results. However, searching the hash of admin!OTdf-u23;)(#$@_!#Rlf (where the text after admin would be your pepper) will most likely not return any results.

Brute Force

OK, but now let's assume your hashes/passwords don't ever leak. So, if I would want access, how would I proceed? I would just send probable passwords to the login form. You have no protection against abusive login attempts, so I can try as many passwords as I please. After the plain text storage of passwords, this is your weakest point, IMHO.

If it's a small, trivial script, it's probably not worth implementing anything to block that as no one will bother, though if there could be some substantial damage, it might be best to block invalid login attempts after a while. You know how things are now, but maybe you'll pick a fight with the wrong guy later on, or an automatic script will pick up on your login form.

A quick & dirty blocking mechanism is to keep an array of the number of invalid login attempts per IP address ($logins = array('127.0.0.1' => 3)). Once it reaches a certain number, you either block it permanently (if it's a small script and your friend is affected, he'll contact you) or you log the time and block it for a day or so.

Issues with Warnings

I recommend you output all warnings at least during development; this could help you find some issues. This can be done by adding error_reporting(E_ALL) to your login page.

A few issues that will arise is the direct reference to $_POST without checking first; use isset($_POST['user']) to see if the key you're asking for exists in the first place.

If you want to take it a step further, you could also do a check with is_scalar(), as sending an array as $_POST['pass'] will make crypt() issue a warning. This is something a lot of websites tend not to cover, which allows an attacker to find out the full path of your server and possibly the inner workings of your scripts (say, if the error comes from /hidden_include_folder/validator.php, this file will be shown in the error).

Working with Files

You need to account for concurrent file modifications. Again, if this is just a little fun script, it's a risk you can take, but anything of substantial size and/or importance should really be handled with a database like MySQL, which accounts for concurrent modifications and handles them for you automatically.

Note that keeping data in a file does not scale well; if you think the amount of data you're storing could grow a lot, consider setting up a database. I've seen it happen more than a few times that PHP reads a large file, then runs out of memory while writing to it, and the file ends up being empty – not a fun situation.

\$\endgroup\$
  • 1
    \$\begingroup\$ Nice answer! Welcome to Code Review, feel free to visit the "regulars" in Code Review Chat \$\endgroup\$ – Vogel612 Jun 26 '14 at 9:51
  • \$\begingroup\$ Very good answer! +1 \$\endgroup\$ – Madara Uchiha Jun 26 '14 at 11:08
  • \$\begingroup\$ >1 MB is considered large for PHP? \$\endgroup\$ – JAB Jun 26 '14 at 18:47
  • \$\begingroup\$ @JAB Background: I did a lot of patching for a CMS written in PHP that used flat files only. Things would start to be on the dangerous side after one or two megabytes for some users. However, I just did some tests on my machine and even processing a file of 23 MB wasn't even slow, so maybe concurrent modifications were always to blame, and never file size. Interesting! \$\endgroup\$ – ljacqu Jun 26 '14 at 19:14
  • \$\begingroup\$ @ljacqu That would make more sense, then. I haven't really used PHP before, but 1MB seems a bit small in general for running into lack-of-memory issues unless you're operating off an embedded system or are, for some crazy reason, storing the entire contents of the file on a size-limited stack. \$\endgroup\$ – JAB Jun 26 '14 at 19:26
5
\$\begingroup\$

Storing passwords is a very bad idea; if the source-code somehow leaks or the server gets compromised somehow the passwords will be leaked. Only a hash of the password should be stored - so the code can check for a match but never actually retain the full password longer than required (and not to disk - it should only need to briefly be in memory).

Is the site at least using TLS? If not, replay attacks will likely work.

Is there a reason you're not just using .htpasswd files?

\$\endgroup\$

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.