• D (unregistered)

    WTF? Why are there no comments?

  • ShockedITellYou (unregistered)

    Woah. Writing stupid units tests doesn't help you. I'm shocked.

  • Donkey (unregistered)

    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.

  • Troy McClure (unregistered) in reply to D
    D:
    WTF? Why are there no comments?

    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!

  • (cs)

    I think I actually got dumber when I read this!

  • :D wonderful comment!!! (unregistered) in reply to sir_flexalot
    sir_flexalot:
    I think I actually got dumber when I read this!
  • Gareth (unregistered)
    Apparently "Karl", the person in charge of the authentication module, wasn't too good at taking criticism as long as the unit test passed.

    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...

  • Spoe (unregistered) in reply to D
    Karl was enlightened only when Christian made sure that "testuser" wasn't the last entry in the database.

    Um, wouldn't he have wanted to make sure it was the last record to provide the brick upside the head of illumination?

  • dumbest forum user probably (unregistered) in reply to sir_flexalot
    sir_flexalot:
    I think I actually got dumber when I read this!

    :D wonderful comment!!! And I got dumber as well, look at what I did with my previous comment :D :D

  • (cs) in reply to Spoe
    Spoe:
    Karl was enlightened only when Christian made sure that "testuser" wasn't the last entry in the database.

    Um, wouldn't he have wanted to make sure it was the last record to provide the brick upside the head of illumination?

    You're thinking "make it take as loooong as possible to teach him a lesson" perhaps? Aim higher: If it's not the last entry in the database, then the unit test won't work.

  • Pon (unregistered)

    The REAL WTF is that he's persisting hashes.

  • Name withheld by request (unregistered) in reply to Spoe
    Spoe:
    Karl was enlightened only when Christian made sure that "testuser" wasn't the last entry in the database.

    Um, wouldn't he have wanted to make sure it was the last record to provide the brick upside the head of illumination?

    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

  • res2 (unregistered) in reply to Name withheld by request
    Name withheld by request:
    Spoe:
    Karl was enlightened only when Christian made sure that "testuser" wasn't the last entry in the database.

    Um, wouldn't he have wanted to make sure it was the last record to provide the brick upside the head of illumination?

    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

    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.

  • Aaron (unregistered)

    The Real WTF™®? is that he didn't use a salt.

  • imma (unregistered) in reply to Name withheld by request

    The problem isn't so much the speed ... It's that only the last person in the table can log in at all

    • imma
  • kay (unregistered)

    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!

  • Da' Man (unregistered)

    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:

    SELECT passhash, fullname, rights FROM users WHERE login = 'xympflegrümpf';

    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!)

  • (cs) in reply to imma
    imma:
    The problem isn't so much the speed ... It's that only the last person in the table can log in at all - imma

    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.

  • Troy McClure (unregistered) in reply to Aaron
    Aaron:
    The Real WTF™®? is that he didn't use a salt.

    He's trying to keep his cholesterol down

  • anon (unregistered) in reply to Pon
    Pon:
    The REAL WTF is that he's persisting hashes.

    Persisting hashes of the password is a good thing (it's a bad practice to store raw passwords).

  • J (unregistered) in reply to Da' Man

    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.

  • SnapShot (unregistered) in reply to Pon
    Pon:
    The REAL WTF is that he's persisting hashes.

    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.)

  • blaaaaaaaaaaaaa (unregistered)

    Damn. Bait and switch. I came hungry enough for a smorgasbord and only got an amuse bouche.

  • J (unregistered) in reply to SnapShot
    SnapShot:
    Pon:
    The REAL WTF is that he's persisting hashes.

    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.)

    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!

  • Kiss me, I'm Polish (unregistered)

    I must admit I was looking for code like

    if (struser == "testuser")
{
    this.authenticated = true;
}

    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.

  • AT (unregistered) in reply to J
    J:
    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.

    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?

  • snoofle (unregistered) in reply to Gareth
    Gareth:
    Apparently "Karl", the person in charge of the authentication module, wasn't too good at taking criticism as long as the unit test passed.

    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...

    How would a test for failed authentication have illustrated a missing where-clause in this case?

  • snoofle (unregistered) in reply to snoofle
    snoofle:
    Gareth:
    Apparently "Karl", the person in charge of the authentication module, wasn't too good at taking criticism as long as the unit test passed.

    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...

    How would a test for failed authentication have illustrated a missing where-clause in this case?
    Nuts - forgot to sign in - can't edit...(captcha = burned... hmmm...) I meant in the case with only one user in the table...

  • J (unregistered) in reply to AT
    AT:
    J:
    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.

    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?

    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?

  • Rafael Larios (unregistered) in reply to snoofle

    if he fails in a simple authenthication process i don't really want to know what the rest of his code would be.....

  • felix (unregistered) in reply to J
    J:
    SnapShot:
    Pon:
    The REAL WTF is that he's persisting hashes.

    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.)

    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!

    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.

  • Mee (unregistered) in reply to kay
    Just scanning over the code, it didn't immediately jump out that something was wrong.
    Jesus...
  • (cs) in reply to :D wonderful comment!!!

    Wonderful reply. Excellent commenter. Would read again! A++++!!!!

  • (cs)

    Damn,

    50% of the WTF's out there concern the WHERE clause don't they???!

  • Fredric (unregistered) in reply to J
    J:
    AT:
    J:
    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.

    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?

    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?

    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...

  • (cs)

    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

  • Pon (unregistered) in reply to felix
    felix:
    J:
    SnapShot:
    Pon:
    The REAL WTF is that he's persisting hashes.

    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.)

    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!

    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.

    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.

  • Anonymous Coward (unregistered) in reply to Ice^^Heat
    Ice^^Heat:
    Damn,

    50% of the WTF's out there concern the WHERE clause don't they???!

    At least that one was resistant to SQL injection... ;-)

  • (cs)

    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.

  • (cs) in reply to Fredric
    Fredric:
    J:
    AT:
    J:
    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.

    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?

    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?

    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...

    What the hell is wrong with (in a imaginary syntax):

    hashedPass = escape(md5_or_whatever_hash_function(theRawPassword));
user       = escape(userEntered);
sql = "select * from users where user = $user and hashedPass = $hashedPass";
executeSQL(sql)

    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.

  • jpa (unregistered)

    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.

  • AT (unregistered) in reply to Fredric
    Fredric:
    J:
    AT:
    J:
    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.

    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?

    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?

    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...

    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.

  • (cs) in reply to jpa
    jpa:
    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.

    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.

  • Killen (unregistered) in reply to Aaron
    Aaron:
    The Real WTF™®? is that he didn't use a salt.

    Actually, there's no proof that he didn't use a salt in this example.

    Captcha: muhahaha

  • AT (unregistered) in reply to fmobus
    fmobus:
    Fredric:
    J:
    AT:
    J:
    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.

    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?

    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?

    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...

    What the hell is wrong with (in a imaginary syntax):

    hashedPass = escape(md5_or_whatever_hash_function(theRawPassword));
user       = escape(userEntered);
sql = "select * from users where user = $user and hashedPass = $hashedPass";
executeSQL(sql)

    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.

    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.

  • (cs) in reply to Rank Amateur
    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.

    Exactly. It's a feature. Not a bug.

  • Simon (unregistered) in reply to AT

    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.

  • (cs) in reply to AT
    AT:
    fmobus:

    What the hell is wrong with (in a imaginary syntax):

    hashedPass = escape(md5_or_whatever_hash_function(theRawPassword));
user       = escape(userEntered);
sql = "select * from users where user = $user and hashedPass = $hashedPass";
executeSQL(sql)

    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.

    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.

    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.

  • (cs) in reply to AT
    AT:
    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.

    Or tell the client that their password was correct but not their username.

    ;]

  • (cs)

    Furthermore, one could easily add a "salt" mechanism to avoid birthday attacks. For instance (again, imaginary syntax):

    user = escape(user);
salt = executeSql("select salt from users where user = $user");
hashedPass = escape(hash_function_with_salt(rawPass, salt));
userRecord = executeSql("select * from users where user = $user and hashedPass = $pass");

    (or something like this).

Leave a comment on “But It Worked in the Demo”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article