- Feature Articles
- CodeSOD
- Error'd
- Forums
-
Other Articles
- Random Article
- Other Series
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
I'm a novice at DB work, so please forgive the nature of this question. Why c/shouldn't you do something like this pseudocode?
Admin
Sorry, even I know to escape user inputs...
Admin
don't be stupid, tiredness is never be an excuse for "return all records and do the WHERE afterwards"
Admin
You just gotta love it! And if you coded it... you gotta eat humble pie!
Captcha: bling
Admin
You just gotta love it! And if you coded it... you gotta eat humble pie!
Captcha: bling
Admin
Absolutely.
That's the whole problem. I see this kind of crap all the time from people who don't understand SQL...Even "SELECT * FROM users WHERE password = 'password'" would be nice and efficient, because the server is optimized to find that data and return it, and that query is so simple...
I was working on a project the other day where a coder had done basically "SELECT * FROM table" and then was character parsing a 256 byte message field of EVERY SINGLE RECORD for a one week period, looking for a flag. I mean, he easily had 40 lines of code dealing with this parsing.
When I asked him about it, he said, "That's the only way to figure it out."
Ignoring regex, I changed his query to "SELECT * FROM table WHERE message LIKE "%flag%" Query ran in ~5 seconds, which is pretty slow, but while LIKE is inefficient compared to the way it would have been done in an intelligently designed modern database, it was blazing fast compared to his crappy character parsing.
Admin
I'd bet someone told him to "hash" his passwords. So he named the variable passhash, but it's still plaintext. :)
Admin
Some people should be flogged publicly. What happens in reality is that nobody mentions this wtf and the idiot never learns....
Admin
decryptMd5? How's that going to work?
Besides. MD5 should be avoided as it's too easy to create hash-collisions. SHA1 would be better but also not perfect anymore.
The NIST currently has a "competition" for the next SHA. http://www.csrc.nist.gov/pki/HashWorkshop/timeline.html
Admin
Why didn't he use a stored procedure?
come on...let's argue about it
Admin
Kind of like the answer to the question about SQL autoincrement numbers at http://www.phpfreaks.com/forums/index.php/topic,123788.msg512266.html
See the second answer. The mind boggles.
Admin
I think more people need to read this site.
HOW MANY wtf's have we seen that were simply an issue of a missing WHERE clause?
Admin
God I hope that was totally obfuscated code. The SqlDataReader wasn't closed, which means that the database connection wasn't closed, which means that eventually their SQL Server will go BOOM!
Admin
So, what is the right way to do it?
Admin
Perhaps I should have mentioned I know equally little about encryption... After posting, I looked it up out of curiosity and learned a few things.
So, what is the right way to do it?
Admin
Damn, what gives with this forum? I posted once, then looked for it on the page, it wasn't there so I posted again, and now both show up... wtf?
Admin
Personally I'd do something like this:
In the DB store the username, and the hashed password.
Client requests salt.
Server generates random salt, sends it to the client and remembers it.
Client sends username/hash(salt+hash(password)) to the server.
Server selects username/password from the database.
Server hashes the password with the random salt, then compares them.
That should be reasonably secure. Even if the hashed password is sniffed en-route, it's useless, and the reason for hashing twice is so you can store the password in the database and use a dynamic salt.
Admin
A hash would serve to obfuscate the password, but it isn't a two-way encryption/decryption scheme, so you can't "de-MD5."
However, I actually like the idea of using a one-way hash rather than a two-way encryption scheme. As to the bad idea of giving customer service reps access to the passwords - you can't say "go ahead and store it in plaintext and then just don't allow people to have access." Someone will always be able to get that password - DBAs, developers, admins, etc. It can leak out, there can be bugs that allow it to be shown, report writers can mistakenly place it in a report for everyone to see, etc.
How about giving NO ONE access, as in, make it literally impossible to decrypt the password out of the database, even for the DBA. Generating a new random password and giving it to a user that forgot his password is preferable to being able to read it from the database so you can hand it back to him.
You can hash the user's plaintext password input, then compare it to the stored hash in the database, and if they match - voila, you know they typed in the correct password, without actually knowing what that password is (unless your code is capturing user input and storing that as well somewhere, which would defeat the purpose). A hash that isn't likely to produce collisions would need to be used.
Of course in a web page the browser->server communication would need to be encrypted with SSL or the like to eliminate sniffing.
Admin
Oh-no buddy. You just opened up a big can of worms. If you're not careful, you'll bring Alex'es site down with a flood of responses!
Admin
For your enjoyment (or lack thereof), my real-life production ASP.NET code:
The stored proc:
All passwords are stored in as an MD5 hash. I don't store sensitive user data (besides an email address), so I'm happy enough with MD5. The hash is salted.
I don't tell the user if they entered the wrong password, just that they entered the wrong credentials. If they forget their password, I create a new one and sendit to them.
Note, the stored proc is simply a convenience layer (or inconvenience, your call). However, without the stored proc, I'd damn well still be doing a parameterized query. You don't want to just append in the username either like some people have posted. You never know when someone's username is going to be '--;Drop database;'
Now, this isn't necessarily the "best" way; it depends on your needs and your situation.
Admin
Hashing Passwords
http://msdn.microsoft.com/msdnmag/issues/03/08/SecurityBriefs/
Admin
In addition, with this approach, you have to execute two queries if you use a salt (salts are generally accepted as good practice since they mitigate dictionary attacks). First you have to look up the salt, then hash, then authenticate. It's pointless extra overhead for no tangible improvement in security.
If you're trying to avoid ever having useful information go across the wire, there are schemes for that using throwaway tokens, like Kerberos. That's almost certainly overkill for a typical web app and you can stop most replay scenarios just by using SSL; either way, insisting on authentication at the database level instead of the web-server level doesn't accomplish anything useful.
wayne: MD5 is not broken as it applies to basic authentication. It's possible to create two arbitrary strings with the same MD5 hash, but it's practically impossible if you start with an existing hash and find a collision that's limited to, say, 20 alphanumeric characters. The white paper on MD5 only showed it to be broken as a form of digital signature/checksum.
webzter: You call SecurityUtility.Encrypt(password) with no additional parameters. Where exactly is the salt? Are you just slapping some characters at the end of the hash itself (which would be useless), or am I missing something?
Admin
And I know just which book: 'SQL for dummies'.
Admin
It was looking up your username in the database...
Yeah, low hanging fruit and all.
CAPTCHA: worst. president. ever.
Admin
Close, but wrong.
He needed to write a unit test for an authentication that should pass, but for a user that was not the last inserted into the database.
As written, this would fail that authentication, because after setting this.authorized to true for having found it, it would then move on to the next record, causing this.authorized to be re-set to false.
Captcha: DOOM! Yes, definitely, if code like this propagates.
Admin
Well, first of all, as others pointed out you can't decrypt md5, so you would have to send the username in plaintext. This is no big deal. There is another reason that makes this inherently insecure: Youre client and server are exchanging a secret (password) which is used to check wether the user is the person he claims to be. You're just changing the secret from being (password) to being (md5(password)) - an attacker would only need to sniff the md5sum to gain access.
The reason why you store password hashed (preferably salted) in the database instead of plaintext passwords is that even if the database gets hacked, the attacker does not gain knowledge about the password used - the hash is irreversible.
Admin
I do not know C# but I guess this code should work:
Admin
Why on god's green earth would you tell a potential attacker if a password is correct?
Admin
Which presents a totally different WTF: Using a database account that can do such destruction in an insecure context.
Admin
What worries me most about storing plain-text passwords is has as much to do with social engineering attacks as it does to the mechanics of protecting secrets. I have friends who will remain unnamed who use the same user name / password no matter what they are doing. In other words, their bank account uses the same password as their account at Joe's House of Porn. If I was a sleezy admin on Joe's House of Porn I wouldn't hash the passwords; I'd wait until I'm ready to retire and then try that username / password combo for every user on every bank or day trader site I could get my hands on.
Luckily I'm not a sleezy admin at Joe's House of Porn... However, if you a help desk has access to your password I'd be pretty darn sure that that password and user name are not being reused anywhere else.
Admin
CAPTCHA: pirates. yarrrrrh
Admin
I consider that a security feature. If we told the end user which is wrong, a hacker could just try random usernames until s/he gets one right, then go on to the password.
Admin
You're missing the point - it could be a user-friendly feature, just like password retrieval:
"The username you entered is incorrect. Please enter an email address and we will send you your correct username"
Admin
public string HashIt(string password) { return("########") }
It's fast and cuts down on those pesky calls from users who have forgotten their passwords.
For extra security, return 9 or 10 hashes.
--Rank
Admin
Second that vote.
Admin
"Unrecognized username entered. Entered password "password1" matches 17 users. Please select your correct username from the list below."
Admin
Thank you for using myspace.com.
captcha: dubya. MmmHmm, I love me some internets.
Admin
I sure hope you're kidding. You should never, ever, ever tell the user if the issue with their login was the password or the username, or both.
The system should issue a Login Failure message, and not indicate what part of the credentials was bad.
Think of entering random usernames until you found one that works, or enter random passwords until you found one that had a matching username...
Admin
I think Alex should make the comment system prepend/append all comments with [sarcasm][/sarcasm] tags by default. That would save about eleventy billion keypresses daily.
http://forums.thedailywtf.com/forums/permalink/92026/93672/ShowThread.aspx#93672
Admin
Doing "SELECT * FROM users WHERE password = 'password' is still very poor practice because you're getting all users who have that password, now you get to loop over them on the web server to find the one with the right username. Worse like the original posted said, using SELECT * is almost, always a bad idea.
The database server is optimized to find things like this for you, there's almost no reason to ever loop over a record set searching for something.
"SELECT count(*) FROM Users WHERE password = 'hashed-password'" and I'd be using bind parameters as well.
Admin
Hey Alex,
You posted early M-Th... busy today? <waiting to laugh>
Admin
Remind me not to hire you when I need a security program written.
The alternative is to store passwords in plain text--which of course means that anyone with access to the database can find out anyone's password in a heartbeat--plus probably means that password may have to travel a network in plain text. In other words, rather useless security.
Admin
Quite a few things. This thread is just full of wtfs today...
If the User table has huge fields you're potentially getting back a massive amount of information you don't even need. SELECT * is almost always a bad idea.
Either specify the columns you need or use the Count() function.
Second a login check should only ever return 0 or 1 rows.
The username should be a unique key, counting rows is absurd. How about if( result.recordCount ) ? since 0 is false and 1 is true in most languages!
Everyone should also be using bind parameters in the query instead of adding them to a SQL string... but that's another issue entirely.
Admin
I had the same thought. I can't think of anything wrong with it as presented except that two or more users trying to log on with the same username (or any usernames, if the stored random salt was not saved per username) at the same time could cause issues for one another. In the absence of attackers, that shouldn't be a big deal since the username should be unique, and people mistyping their usernames shouldn't happen often enough to collide much with people typing the same username correctly. It does, however, mean that someone else could mess around with a user and try to keep them locked out of the system by continually trying to log in as them with a garbage password.
The only other problem I can think of for it offhand is because it stores the unsalted password hash, a less diligent later programmer could decide that he wanted to move the unsalted hash back and forth between the client for some reason. Even if this hypothetical later code did not replace authentication or allow it from another context, an attacker who knew where to look for unsalted hashes could perform a dictionary attack to try to match a sniffed password hash, and on finding a match could go through the normal authentication channels. Not a problem if no one starts throwing unsalted hashes around, but of course if no one ever pulled a WTF, this site wouldn't be around.
Admin
You never check the username. That would log you in for guessing any user's password correctly without even knowing the username.
No more of this count(*) bs:
"SELECT TRUE FROM users WHERE username = ? AND password = ?" (parameterized).
Replace "TRUE" with "userid" for efficiency if you're into that sort of thing :D
Admin
Oh I quoted the wrong one, I wanted to reply to that other chap, AT. I understood very well you [sarcasm]. Sorry 'bout that.
CAPTCHA: poindexter...?!?
Admin
Speaking of count(*)...
Here's a fun trick for existence checking that doesn't require the database to loop over every matching row to count them up, when you really only care if it's there or not:
SELECT 1 FROM whatever WHERE your_criteria=something LIMIT 1;
If it's there, you'll get 1 row returned. If it's not, you'll get 0 rows returned. You don't even have to get the actual row data back, you can just check num_rows() (or whatever your local equivalent is). What's more, the DB stops after it finds the first match, which is all you care about anyway!
If you've got a lot of activity, or less unique set of constraints, using count(*) can turn into a lot of wasted DB activity.
But that's not WTFy at all. All of this does remind me, however, of the following anecdote:
A friend of mine did a quick count on the database of his semi-popular website to see how many passwords were '12345' (he was using a non-salted hash for passwords at the time, so getting the appropriate hash for that password was trivial). As it turns out, over 5% of his users had that as their password, and another 2-3% had '123456' as their password. One can only assume that that carries over into the rest of the world, so getting access to an online account might be as easy as just getting 20 or so usernames and trying them 'til one works. shudder
That green slime had it coming.
Admin
Sometimes the code samples are cleaned up to better illustrate the WTF. "SELECT *" may or may not have been in the original code. Agreed in principle, though, that SELECT * is not a good practice.
Admin
Using that method, why not just:
sql = "select user from users where user = $user and hasshedPass = $hashedPass";
You could substitute the first user with something meaningful (like rights or something) and that way you're not returning all the info back to the client (*), but rather just what the client actually needs to know.
Just a thought.
CAPTCHA: ewww
ewww, indeed.
Admin
For SQL Server, using * is bad. Very bad. Not only do you face issues with returning every single column, but if you've added, deleted or changed columns in the table, SQL Server might throw an error back when you SELECT *, because it's picking up the table definition from a cache.