- 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
WTF? Why are there no comments?
Admin
Woah. Writing stupid units tests doesn't help you. I'm shocked.
Admin
Aside from the WTF with the where clause...
I liked the fact that this.authenticated = false; gets called for every record that doesn't match.
Admin
Because the story just got put up minutes ago. Some of us have real work to do. Not me per se - it is Friday after all!
Admin
I think I actually got dumber when I read this!
Admin
Admin
So the Real WTF[tm] is that there was only a unit test for successful authentication. All someone had to do was write a test for failed authentication to show Karl where his mistake was.
At least the guy was using unit tests...
Admin
Um, wouldn't he have wanted to make sure it was the last record to provide the brick upside the head of illumination?
Admin
:D wonderful comment!!! And I got dumber as well, look at what I did with my previous comment :D :D
Admin
Admin
The REAL WTF is that he's persisting hashes.
Admin
It wouldn't matter where the test record was, The method will still move though all the records in the table before completing. So it may be more effective to demontrate that the method continues to execute long after Authenticated is set to True.
BTW: Nice shiney new captcha
Admin
Actually it does matter, if a match is found before the end, the next loop through would set Authenticated back to false. The only user who could get in would be the last one in the database.
Admin
The Real WTF™®? is that he didn't use a salt.
Admin
The problem isn't so much the speed ... It's that only the last person in the table can log in at all
Admin
Haha. I almost missed that error as well.
Just scanning over the code, it didn't immediately jump out that something was wrong.
It wasn't until the author mentioned that the test was seveal orders of magnitude slower with 10k users, that I realized what was up.
So I don't blame him for missing it as well... he probably coded the function quickly, without much sleep, on a deadline and trusted the test cases to be functional---i.e. test for all scenarios, not just one!
Admin
Would adding a WHERE clause suffice? I actually don't like the "SELECT * FROM" either.
What if, in it's next incarnation, the users table also includes user images (BLOB), copies of the user's id card (another BLOB) and a 500-page novel the user was writing while waiting for authentification.
I would actually use sth. like:
and only then check if the password hash matches.
BTW: I give that bloke one thumb up for using hashes rather than sending plain passwords over the network (you don't see that kind of good thinking so often, believe me!)
Admin
It's worse than that. Assuming they are using SQL Server, when the server has many people logging in, then SQL Server will switch to merry-go-round scans of the users table, and each person may end up with a different "last" record. Nothing like a random output wrapped in a race condition. Yummy.
Admin
He's trying to keep his cholesterol down
Admin
Persisting hashes of the password is a good thing (it's a bad practice to store raw passwords).
Admin
Why add a WHERE clause at all? There is no good reason to send the password back to the web server, hashed or not. A stored proc that checks the login and returns a true / false would eliminate the issue entirely.
Admin
Why's that? I persist hashes instead of the plaintext password all the time. But I'd be interested to know why that's not recommended (other than the inability to email passwords to the user if they forget theirs; I generally generate a new random password instead.)
Admin
Damn. Bait and switch. I came hungry enough for a smorgasbord and only got an amuse bouche.
Admin
Ditto to that. I never store the actual password, and I get quite upset with organizations that do. Especially when they have some random idiot in a call center asking me to repeat my password over the phone to them to verify who I am! WhyTF are they allowed to see it! WhoTF's bright idea was that!
Admin
I must admit I was looking for code like
But it's so rich! Here we go - misused SELECT, no WHERE clause, just plain wrong true/false setting. The only positive thing is that it is not SQL injection prone. But I think that's just an accident. Someone buy this guy a book.
Admin
Do you think that makes the system more secure? You do realize that the hashed password already must go from the user's computer to the Web server, then from the Web server to the database right?
Admin
Admin
Admin
Do you think it makes the system less secure? What if the user enters the wrong password? What if the username isn't even valid? Why send the correct password back to the web server when it can do the check at the database?
Admin
if he fails in a simple authenthication process i don't really want to know what the rest of his code would be.....
Admin
There are password authentication schemes that store the password in plaintext so that the secret must never travel unencrypted over the wire. CRAM-MD5 [RFC 2195] for example. Can come handy if no TLS is in place. It's all about tradeoffs.
Making this information available to a call center agent is wholly different WTF though.
fg
CAPTCHA: Tacos. Tasty, just had some.
Admin
Admin
Wonderful reply. Excellent commenter. Would read again! A++++!!!!
Admin
Damn,
50% of the WTF's out there concern the WHERE clause don't they???!
Admin
I would as well go for a stored proc, it's definitely more secure because you don't have to send the correct password anywhere.
The way this code handles it, by sending all users and their password to the client is just about the worst security you can have...
captcha: waffles. Oh great, now I feel like eating some...
Admin
While being entirely unable to login represents some inconvenience for the typical user, this implementation is preferred because of the superior security. A hacker can’t just guess anyone’s password. It has to be the password of the last user in the table. You can’t have security without some cost to usability. --Rank
Admin
Sorry, I should have been more specific. If they are just using something like "pass.GetHashCode()" for the hash, then that is not guaranteed to stay the same in future versions of the framework. Therefore, no user will be able to log in. If, however, they are using a fixed algorithm, like MD5, then that's fine.
Admin
At least that one was resistant to SQL injection... ;-)
Admin
I was on-site when a report just would not finish. The customer had a lot of data. The report worked "fine" without a lot of data.
The culprit we were reading a table without a where clause. Upon further inspection we never used any data from table. DOH!
Deleted the whole SQL statement and amazed the customer.
Admin
What the hell is wrong with (in a imaginary syntax):
It doesn't send unencrypted passwords over the wire, is immune to injection. You just have to count how many rows returned and you're done.
Admin
I'm surprised that no-one has yet to mention the insecurity of having a testuser stored in the database, and probably also the password for it in the unit testing code.
Admin
First, the web server and the client are not the same thing. Second, authenticating with a stored proc provides a security "advantage" only in the miniscule number of cases where someone is sniffing the network and also attempting failed logins with known usernames. Of course, they can still capture the hashed passwords of all the other users accessing the system at the same time as those passwords are sent to the Web and database servers.
Admin
Well, usually one would setup a "mockup" database on the test environment and use a "hot" database for production... For a clearer explanation on this, see Rails.
Admin
Actually, there's no proof that he didn't use a salt in this example.
Captcha: muhahaha
Admin
How does the hashed password in your SQL query get to the database server? On a floppy disk? This method also doesn't allow you to tell the user whether their username or password is invalid.
Admin
Exactly. It's a feature. Not a bug.
Admin
Why would you want to tell your users that? It's generally accepted that telling the user that the username was okay while the password is not is a bad security practice, because then you've just confirmed to the attacker that now they only need to guess a password, and not a username/password combination.
Admin
The $hashedPass variable interpolated and properly escaped in the query, maybe?
Regarding not being able to tell whether the name or the password is wrong, I think this is actually a good thing.
Admin
Or tell the client that their password was correct but not their username.
;]
Admin
Furthermore, one could easily add a "salt" mechanism to avoid birthday attacks. For instance (again, imaginary syntax):
(or something like this).