7

I have a classifieds website, where everybody may put ads of their products.

For each classified, the user has to enter a password (so that they can delete the classified whenever they wish).

So basically, when somebody wants to delete a classified, they click on the classified, click on the delete button, and enter the pass.

I use MySql as a database.

I use this code basically:

if ($pass==$row['poster_password'])

where row[poster_password] is fetched from MySql...

What do you think? Thanks

  • 18
    Please tell me you didn't save their actual password in MySQL... – animuson May 04 '10 at 18:41
  • 1
    yes i do, but hey, the website isn't online yet, under development... I am just beginning on the security front now... How should I save it then? –  May 04 '10 at 18:42
  • @animuson is it such important for the classifieds website? – Your Common Sense May 04 '10 at 18:42
  • 3
    Read http://stackoverflow.com/questions/401656/secure-hash-and-salt-for-php-passwords – Kendall Hopkins May 04 '10 at 18:43
  • 9
    @Col. Shrapnel Yes it is. Regardless of the type of Website, storing the password in clear text is never ever acceptable. No Exceptions. – Michael Stum May 04 '10 at 18:45
  • @Col. Shrapnel: Yes, it's important everywhere! Because a lot of people use the same password over multiple websites! If someone hacks a website and sees "username:password" and they recognize username from another website, which they happen to have used the same password for, they got a free ticket to another account. – animuson May 04 '10 at 18:46
  • OMG conspirology. "plain text is insafe!", "MD5 is unsafe!". Everyone passing plain text passvords over unencrypted HTTP but always with great security improvement advise! – Your Common Sense May 04 '10 at 18:47
  • 3
    Many users reuse passwords. If I can figure out which password goes with which user, I can try that password elsewhere and see what opens up. Besides, if I can delete someone else's passwords, and you can't tell I did it (you'll probably just say the user did it), then I can tick off your whole customer base: you'll get paid but their ads disappear. Will they file suit? Who knows. – Mike DeSimone May 04 '10 at 18:47
  • @Col Shrapnel, lots of people use the same password on multiple accounts. (I'm not saying it's ok to do THAT either, but people do it). So some little website gets hacked, and now the hacker has account info with passwords, that he can try elsewhere, such as banking sites, ebay, amazon, etc.. So yeah, it's important. – Chris Thornton May 04 '10 at 18:48
  • @Mike Hi-5 for having the same thought at the same time. – Chris Thornton May 04 '10 at 18:49
  • "Everyone passing plain text passvords [sic] over unencrypted HTTP but always woth [sic] great securyty [sic] improvement"... Citation needed. – Mike DeSimone May 04 '10 at 18:50

8 Answers8

9

See this: Secure hash and salt for PHP passwords

Hash their password (maybe with some salt) on the way into the database. Store their hashed password in the database (NOT their actual password). Then fetch their hashed password from the database and hash their input password and compare the hashed passwords.

Some lame pseudo code:

password_hash = hash(password_cleartext)
# store password_hash in database

Later:

input_password_hash = hash(input_password_cleartext)
fetched_password_hash_from_db = fetch(db, password_hash)
if (input_password_hash == fetched_password_hash_from_db) {
    ... authenticated ...
}

For a start with php, try: http://php.net/manual/en/function.sha1.php

Community
  • 1
  • 1
dlamotte
  • 6,145
  • 4
  • 31
  • 40
  • 6
    +1 for first to mention hashing. **HASHING IS NOT ENCRYPTION PEOPLE** - they are completely different concepts *(it is also not the same as a checksum, which is not necessarily cryptographically secure)*. Also, **MD5 is completely broken**, don't use it. – BlueRaja - Danny Pflughoeft May 04 '10 at 18:46
  • Okay, then hash is a function of php I guess? Do you have an acutal example? –  May 04 '10 at 18:54
  • 1
    Do the downvotes have anything to say here? Or do I just get downvoted without explanation? – dlamotte May 04 '10 at 19:00
  • @Camran - See the question Kendall Hopkins linked to in the comments of the original quesion. – MiffTheFox May 04 '10 at 19:01
  • Really don't like how some people respond to the person asking a question on my answer and downvote my answer. If you have a link to redirect the asker, ANSWER the question, don't just comment, worse don't downvote people that DONT know about that other question. – dlamotte May 04 '10 at 19:13
  • @BlueRaja you have a good chance to demonstrate an md5 weakness. Not with many words but with just single one. If you please: http://stackoverflow.com/questions/2768248/is-md5-really-that-bad – Your Common Sense May 04 '10 at 19:25
4

Your code looks safe, but your design may need some work.

SQL Injection

The dangerous part of the code is in storing anything in the database, or showing anything to the users, that is collected from the user. So, the part you have to be careful with occurs prior to your example. Ensure that you're validating, filtering, and escaping any data that you collect from the user, including the password and the ad information.

Encryption

The advantage of storing the password in the database is that you can let the user retrieve the password via email or some other means if they lose it.

However, if you do store passwords, you should store them encrypted, using a secret key, so that if someone is able to direct read access to your database, they can't read all the passwords in plain text. Still, you're going to have to store the secret key somewhere, and if someone gets your secret key and has access to your database, they will have access to all of the passwords.

Hash Values (recommended)

It's best practice and more secure to only store one way hash values (SHA1 or SHA256) of the passwords in the database instead of the actual passwords. This way, you cannot retrieve the password. Hash values are intentionally one way by throwing away some of the data.

Instead of retrieving the original password, you hash the password that the user enters and compare the hash value against the stored hash value to see if it matches. If the user loses the password in this case, instead of emailing the password to the user, you email the user a new, randomly generated password.

Storing only the hash value protects your data even further, since even if the user has read access to your database, the hash values offer no advantage, and there is no secret key that will unlock all of your hash values.

When you hash the passwords, be sure to use a random salt value and store the salt to protect your list of hashes against rainbow attacks.

Summary

Sometimes you don't get to choose the password. Sometimes the password comes from another system, so you don't always have a choice, and sometimes your superiors (maybe even the users) will demand that they be able to retrieve passwords, however, when possible, you should choose the more secure option.

Note that all of this encryption and hash value business only partially protects your server against people who are able to obtain read only access to your data. Sometimes, getting your data is enough of a prize, so if the user can read the password hash, can they read your credit card numbers?

You need to protect your database. Do you have a secure password on your database system? Do you only allow local access to your data? Have you created a database user with least privileges to use in your application? Are you properly protecting yourself from SQL injection and scripting attacks?

If someone has read and write access to your data, the whole password business becomes moot.

Marcus Adams
  • 53,009
  • 9
  • 91
  • 143
2

Don't store the actual password in the database. Instead store a checksum (MD5, SHA1, etc). When you want to compare, perform a checksum of the value the user submits and compare the checksums.

That way you never have the actual password in memory.

Steven
  • 13,501
  • 27
  • 102
  • 146
  • ... or stored anywhere for that matter. – Steven May 04 '10 at 18:44
  • md4, md5, sha0 andsha1 are broken hash functions. Anyone who purposes a vulnerability will get a -1, sorry its the rules. sha256 should be used. – rook May 04 '10 at 18:51
  • 1
    You must have it in the memory when you check it, unless you hash on the client side in javascript. Not that it matters - if an attacker can access the memory, having the password there will be the least of your worries. – Tgr May 04 '10 at 18:56
  • @The Rook, if sha0 is broken, so is sha256. The only difference is the key length. – Malfist May 04 '10 at 20:49
1

Best practice is to keep a salted sha1 hash in the database:

if (sha1($pass.$row['poster_salt'])==$row['poster_password'])

(poster_salt is a random string generated and saved when the user chooses the password.)

That way if an attacker gets access to your database, they still won't get the passwords of the users (which are probably used elsewhere too - most people don't bother to choose different passwords for different sites).

Also, you should use secure (HTTPS) connection. And require sufficiently strong passwords.

(At least if you want good security, which might be an overkill in the case of a simple ad listing).

Tgr
  • 27,442
  • 12
  • 81
  • 118
  • md4, md5, sha0 and sha1 are broken hash functions. Anyone who purposes a vulnerability will get a -1, sorry its the rules. sha256 should be used – rook May 04 '10 at 18:53
  • +1 Best answer so far. @The Rook: That seems a bit unfair, sha1 isn't nearly as broken as the other three (no collisions have even been found yet) – BlueRaja - Danny Pflughoeft May 04 '10 at 18:53
  • 1
    @BlueRaja can you show us a MD5 weakness using a collision please? – Your Common Sense May 04 '10 at 18:57
  • @BlueRaja, your right but there are 2 attacks against sha1 and it should be within reach. This post is still a technically a cwe violation. And yes, I am prick :) – rook May 04 '10 at 19:00
  • 1
    Finding a collision in sha1 takes about 2^60 operations (instead of the 2^80 needed for a brute force attack). Get a good computer with 4 or 6 cores, and you are done with it in three years. You can call that broken in some technical sense of the word, but saying it is not good enough for a classifieds website is just stupid. Especially since there is no better option available in vanilla PHP (you need [hash](http://www.php.net/manual/en/function.hash.php) for sha256, and few hosts allow installing PECL extensions). – Tgr May 04 '10 at 19:11
  • @Tgr Sha1 has been broken twice, it is now only 2^58th operations which is pretty large but its getting smaller. There are PHP implementations of sha256 (http://www.google.com/codesearch?hl=en&lr=&q=lang:php+sha256&sbtn=Search) – rook May 04 '10 at 19:17
  • Let's assume 1000 operations for a single sha1 run. That means you can do about 10^16 sha1 runs in the time needed to generate a collision, which is enough to brute-force test all passwords shorter than 10 characters (upper+lower+digit). Few sites require passwords longer than 8 characters. Not only is the attack you are proposing implausible, there are less implausible attacks which work against stronger encryptions as well. – Tgr May 04 '10 at 19:49
  • Furthermore, it is not obvious that attacks which work against a pure sha1 hash also work against a salted one. – Tgr May 04 '10 at 20:03
  • Moved this discussion into a separate question: [Is SHA-1 secure for password storage?](http://stackoverflow.com/questions/2772014/is-sha-1-secure-for-password-storage) – Tgr May 05 '10 at 09:46
0

You should hash the password somehow and store and compare using the hashed version. See this link for more details:

http://phpsec.org/articles/2005/password-hashing.html

quoo
  • 6,237
  • 1
  • 19
  • 36
  • And yeah, don't store the actual password ANYWHERE. – quoo May 04 '10 at 18:45
  • 1
    I think encryption is a deceiving word here. It's "cryptographic hashing", not encryption. Encryption sounds like simply encrypting the password vs hashing which cannot be "reversed". – dlamotte May 04 '10 at 18:45
  • Passwords should always be hashed. Encryption should never be used for passwords. – rook May 04 '10 at 18:52
  • ah, thanks, corrected, for some reason i thought hashing was a more specific type of encryption. but then.. i'm a front end dev, i don't deal with this stuff too often. – quoo May 04 '10 at 18:53
0

I would encrypt the password before storing it, then decrypt when retrieving it so you can check it against what the user entered in plaintext (per your example code above).

Also, protect yourself against any SQL injections, or someone could see all the passwords (and other data) in your database.

Banjer
  • 8,118
  • 5
  • 46
  • 61
  • -1 encryption should never be used for passwords. This is a clear violation of CWE-257 (http://cwe.mitre.org/data/definitions/257.html) A message digest function such as sha256 should be used. Anyone who purposes a vulnerability gets a -1, thats the rules. – rook May 04 '10 at 18:54
  • -1 for The Rook for saying encryption should never be used, then citing CWE-257 which says "Use strong, non-reversible encryption to protect stored passwords." – Stephen P May 04 '10 at 19:10
0

This implies the passwords are placed into your passwords unencrypted. If this is the case you should be using some sort of encryption when entering the passwords. One way of doing this is the MD5 function which hashes the password.

When doing the insert you would do

Insert into table(email, password, whatever) values('$email', md5($password), whatever)

And when comparing you would do

if (md5($pass) == $row['password'])
Gazler
  • 83,029
  • 18
  • 279
  • 245
  • -1 md4, md5, sha0 andsha1 are broken hash functions. Anyone who purposes a vulnerability will get a -1, sorry its the rules. sha256 should be used – rook May 04 '10 at 18:52
  • Fair enough, could you please provide a source? – Gazler May 04 '10 at 18:55
  • See [here](http://www.google.com/url?sa=t&source=web&ct=res&cd=1&ved=0CAYQFjAA&url=http%3A%2F%2Fciteseerx.ist.psu.edu%2Fviewdoc%2Fdownload%3Fdoi%3D10.1.1.60.3776%26rep%3Drep1%26type%3Dpdf&ei=5m3gS9zCLZO8Nuigsc8J&usg=AFQjCNGSF9GeeOlCrWlb4LKSRlYlLlT2-w): *"Our presented algorithm [...] **allows us to find full MD5 collisions in only minutes** on a 3Ghz Pentium 4".* Wang et al. also provided (about five years ago now) a method which shows how to find MD4 collisions using pencil and paper. SHA1 was broken more recently, though not nearly as badly (there are still no known collisions). – BlueRaja - Danny Pflughoeft May 04 '10 at 18:58
0

my suggestion is the following

the users table have two columns, one called "password" and the other "salt"

$password =  'youruserpassword in plain text';
$salt = bin2hex(openssl_random_pseudo_bytes(32));
$passtostore = hash_hmac('sha384', $password, $salt);
insert into users(password, salt) values($passtostore, $salt);

Then to verify if the user has entered the correct password...

retrive both password and salt from the database and

if(hash_hmac('sha384',$userpass, $row['salt']) === $row['password']) { 
 // is valid 
 }