3

I'm not very educated on PHP or Web-security in general, but i strongly suspect that the code generated by some software the company I'm working for is using, is unsafe.

Here are some snippets of what i am concerned about:

First concern:

$sql = "SELECT password, fullname FROM ".$mysql_table." 
WHERE username = '".mysqli_real_escape_string($db,$_POST['username'])."'";

Is it bad to retrieve the password for the given username and then comparing them in the PHP, or is it better practice to use the password in the query itself, something like this:

... WHERE username = $username AND password = $hashed_password

Second concern:

$crypt_pass = md5($_POST['password']);
if ($crypt_pass == $data['password'])
{
    //LOGIN SUCCESS
}

Is using md5-hashing and not using salt, enough?

Third concern:

 setcookie('username', $_POST['username'], time() + 3600*24*30);
 setcookie('password', $_POST['password'], time() + 3600*24*30);

Is it a good idea to store plain/text usernames and passwords in a cookie?

Is any of this code unsafe and if so, what should be done instead?

Elin
  • 6,507
  • 3
  • 25
  • 47
b00n
  • 135
  • 5
  • Are you storing your password in plaintext? If so then that's definitely a concern! :P – Muhammad Abdul-Rahim May 21 '15 at 14:17
  • No. Passwords are MD5-Hashed and stored in the database. – b00n May 21 '15 at 14:18
  • 6
    MD5 should not be used for storing passwords. – gabe3886 May 21 '15 at 14:20
  • 1
    Without salt though? There are pre-calculated hashes of a very large number of strings, making it trivial to conduct a lookup. – Muhammad Abdul-Rahim May 21 '15 at 14:22
  • 5
    In regards to third concern, NEVER store plaintext passwords anywhere, especially in cookies! – Muhammad Abdul-Rahim May 21 '15 at 14:22
  • 2
    I would be worried that the passwords aren't salted, and that MD5 is being used. The third concern is the scariest... simply passing a username and password pair(is this a plaintext password?) in the clear? Is this over HTTP (please say no). – Gil May 21 '15 at 14:23
  • The whole point is that there are multiple potential points of failure. YOu can fail with a keylogger, with a cookie stealer, with someone on the wire grabbing the plaintext if you don't ssh, or with someone stealing your database, as has been the case for many of the biggest instances where passwords have been exposed. When they have your database and a good computer, it's just a matter of time to reveal most of them (think 12345). That is why bcrypt is better, it slows them down. And if you are not careful and your users reuse email and pass, an attack on you is an attack on them. – Elin May 21 '15 at 15:41
  • "Is using md5-hashing and not using salt, enough" - no. The Sony Playstation network hack was possible because passwords had been hashed using unsalted MD5. A related point: a (now deleted) answer was posted earlier today on this question, and it claimed that salted MD5 is safe. The expert view (as indicated on Wikipedia and in the PHP manual) is the opposite of this claim - don't use MD5 at all for password hashing. The user in question was unwilling to provide authoritative sources for this claim. – halfer May 21 '15 at 16:41

2 Answers2

4

There is actually one more issue you are missing:

Problem: This code is using "==" to check for equality on the hashes. PHP can merrily decide to coerce the first parts of these strings to numbers and then compare the "valid" parts of these numbers. For example, PHP will decide that "0e" starts a scientific notation number, whose value will always be zero. Thus, any two hashes starting with 0e and containing only numbers after will match each other.

"0e111111" == "0e123456"; # true, in PHP world.

There are lots more ways this can go wrong too. Always use "===" in PHP to compare hash.

Probably not a problem: Sending the password hash from the database to the web server is unlikely to ever be a problem, if an attacker can listen to that traffic, you are already in deep trouble. And the alternative is to include your hashed value in your query to the database server - either way, something is going over the link. If you need security here, you probably need a dedicated authentication system that has nothing to do with your database.

Major Problem: Yes, you must have per user, long, salts. Without long salts, your users passwords are stored in the rough equivalent to plain text. An attacker will simply look up the MD5 hashes in a rainbow table to see the corresponding passwords. For example, 482c811da5d5b4bc6d497ffa98491e38 looks secure, but you can look it up to find out it's "password123".

Major Problem: Storing the password in the cookie is horrible. It's going to be traveling over the network for every request, stored unencrypted on the users computer, and sent along with every request to anything on your domain (to images, to any evil files that have been upload). Also, those cookies are not set HttpOnly, which means any javascript on the page (from anyone) will be able to read the user's password.

Daniel Von Fange
  • 857
  • 9
  • 10
  • You confirmed most of my concerns, but the "== vs ===" thing I had no knowledge of. Thank you. All of the code i wrote was generated using [WYSIWYG Web builder](http://www.wysiwygwebbuilder.com/). I guess one should stay away from that application. – b00n May 21 '15 at 16:52
0

First concern

Not particularly. As long as it is good protected against XSS and SQL injections and such. Personally however I prefer to compare them both against the database.

Second concern

MD5, as @gabe3886 said, should not be used for storing passwords. I personally use: http://www.openwall.com/phpass/. But there are loads of hash implementations around. Try searching the web.

Third concern

No it is not. See: https://stackoverflow.com/a/2100386/3455727

What should be used instead: Well, I have no idea why you even would store the username AND the password in a cookie, having them in the database should be sufficient.

Community
  • 1
  • 1
  • The storage of passwords in a cookie was being used as a 'remember me' feature. – b00n May 21 '15 at 14:57
  • Remember me is a whole different topic. But it should most certainly not be implemented by storing the actual user name and password. – Elin May 21 '15 at 15:31
  • Use a unique, random key coupled with user id as the remember me token, and store it in the database. Never do a remember me this way. – Amelia May 21 '15 at 15:32
  • ANd hash it with the UA string so that it can't be reused in a different browser or on another machine. – Elin May 21 '15 at 15:43