• (nodebb)

    The end result of this is that only the last letter matters, so from the perspective of this code, there’s no difference between the word “ship” and a more accurate way to describe this code.

    So this "more accurate" way to describe the code is "poop"?

  • Prime Mover (unregistered) in reply to Steve_The_Cynic

    What a pile of crat.

  • some guy (unregistered)

    If the loop was reversed, a different, possibly more expressive word could also match ship.

  • Tom (unregistered)

    There's actually a good reason to manually compare char by char, without exiting early if it doesn't match: to prevent time-based attacks. Of course, you would normally do that to compare hashes, not plaintext passwords... and make sure the implementation doesn't have a horrible bug like this one.

  • Yazeran (unregistered)

    Wow.... Just wow.

    How can someone design a system with so many WTF's as shown here?? It simply boggles the mind. I mean we are approaching a WTFPL of 1 or more....

  • (nodebb) in reply to Tom

    In this case length equality check prevents it from working properly (and leaks length information), but yes, that is a very valid concern.

  • David Mårtensson (unregistered) in reply to Yazeran

    What rule says it has to end at 1 ;)

    I am sure they could cram in multiple per line if they try :P

  • (nodebb)

    Hey hey! The plain text passwords are not stored in a text file which is publicly accessible on the web, are they? Not all is lost!!

  • Yazeran (unregistered) in reply to Mr. TA

    Probably not, but with the 'Little bobby tables' issue also mentioned, they might as well be. I'm not an expert in SQL injection attacks, but if the passwords are in plaintext (and readable by the webserver as indicated by the function description), I'm sure someone could find a way to extract and show it given that we know that SQL injections are possible....

  • Christian (unregistered)

    nested loops everywhere Looks like an old C programmer with no experience in Java or SQL (and no willingness to learn it months before retirement) had to implement a java / jdbc application. Java doesn't have strcmp? No problem, one of the first examples you learn when learning C is how to implement the whole string.h library yourself (at least in my early days it was). The comparePasswords at least looks very C like...

    And basically Tables are simply arrays of structs, so you could treat them like that as well.

    In the defense of the developer things could be a lot worse: the whole table could be pulled into memory on the client, and "joined" there, so one should give credit to the fact that at lest there is a "where" applied to the query :D.

    cheers

  • (nodebb) in reply to some guy

    If the loop was reversed, a different, possibly more expressive word could also match ship.

    Hmm. "sink", "sith", "same", ... All lacking a bit. Ah, yes, I see the one you meant. +1.

  • WTFGuy (unregistered)

    Hmm. The password comparer ends up only comparing the last char. "Loop & compare" is said to be a common design anti-pattern throughout this abomination. ...

    I'm going to bet there are a vast number of other bugs driven by the same fault: mistakenly thinking they're ANDing or ORing the results of the individual compares across all the iterations when they're not really.

    All sorts of mischief there.

  • King (unregistered)

    I suppose they have a business rule that says passwords must be one character long.

  • MiserableOldGit (unregistered) in reply to Tom

    Good observation, but as whoever is responsible for this didn't seem to know about SQL injection or the need to hash passwords I'm guessing it's a coincidence!

  • ADHDeveloper (unregistered)

    I feel in every course on programing there ought to be a day where they have a drill sergeant come in and yell at people about not making their own authentication system, not sending using input back to the database, and not storing passwords in plaintext.

  • (nodebb) in reply to David Mårtensson

    I am sure they could cram in multiple [WTFs] per line if they try :P

    Really should be "per statement". Flashback to the late 90s when most of my work involved a SMB package (originally designed in the 70s/80s) that, independently of how many/few WTFs it may have contained, was chock full of the equivalent of

    doOneThing; doASecondThing; doAThirdThing;

    I remember one such line in one of our custom add-ons that was about twenty 80-character display lines, so about 1.6K of text. "Um, maybe we should split this one up a bit."

  • Sole Purpose Of Visit (unregistered) in reply to WTFGuy

    I Am Not It, Nor Do I Play It On TV...

    The mind boggles as to how little useful code you could extract from the surrounding goop to this OP, as you say.

    What immediately struck me was the lunacy of writing a "password comparison" method that operates on two char arrays. I mean, this is a pretty language independent objection, really. C has strcmp(), Java has String.CompareTo(), and so on and so on. You only really need to write the signature to the method to realise that you are about to reinvent the most massively wonky and useless wheel since 23 October 4004BC.

    And, ussherly, so he did.

  • Anon (unregistered) in reply to ADHDeveloper

    I'm going to assume you meant "not sending user input back to the database" and ask why not? How useful is a system if it doesn't store any user input?

    And I hope you don't mean to sanitize it by removing all interesting characters like ",',>,<, etc. Not only are those perfectly legitimate things for a user to type, but removing them is a short-sighted fix. What happens when the next markup language comes along that has problems with the . character or ^ character.

  • Me (unregistered) in reply to Sole Purpose Of Visit

    I do remember having to write low-level routines such as string-comparison functions etc as exercises while studying computer science at Uni. In fact most of the stuff I had to do and learn at Uni was basically impractical for the real world, although the purpose was more about basic programming skills and the kind of logical thinking that you need to solve problems.

    My guess is that all that code was written by someone fresh out of Uni who didn't quite grasp the real point of all those exercises and simply repeated the only thing he knew verbatim.

  • Wally (unregistered)

    Does someone have stock in Intel? How many cores do we need to run this cra*? More!

  • Wizofaus (unregistered)

    I wander how many testers would think to check "what happens if I enter the wrong password but where the last character is correct"...

  • Naomi (unregistered) in reply to Anon

    I'm going to assume you meant "not sending user input back to the database" and ask why not? How useful is a system if it doesn't store any user input?

    They're referring to SQL injection attacks. You can't include unsanitized user input in your database queries or you're vulnerable to those.

    And I hope you don't mean to sanitize it by removing all interesting characters like ",',>,<, etc. Not only are those perfectly legitimate things for a user to type, but removing them is a short-sighted fix. What happens when the next markup language comes along that has problems with the . character or ^ character.

    And yet you have to sanitize input to defend against XSS attacks. Frankly, anyone who worries about what's going to happen to their Web application when some new markup supplants HTML is profoundly misguided. And in the unlikely event that that happens, you're already going to be reworking your application to support the new markup.

  • Olivier (unregistered)

    I don't really see any clue saying the password were saved as plaintext, beside Remy saying so. Even if the passwords are hashed, the hash will be a string and comparing the password provided by the user and the password stored in the database will still be a string comparison.

    Given the rest of the fuckery, I admit it is probable that passwords were not protected, but the string comparison is not a strong enough evidence.

  • (nodebb)

    This gives me the inspiration to share yet another wonderful management story!

    So, after a few workflow programs got deployed and saved the company many hours we got into full 'DevOps' mode, and suddenly, automation became a priority on our team. Since we have been crunching for as much as I can remember, the PHB decided to outsource the development of the next big advancement.

    Being cash-strapped as well implied we had to accept the lowest offer. I mean, really, we had to. It's not like we can just draw a cross over it and carry on.

    What we got was a database table editor. I honestly would have just used one of the few DB editors but who am I to make such bold decisions! Even the cheapest developer however would like to spend less effort for the same money. That's why he asked what kind of database he has to use. He found out the hard way our database of choice is MySql 4. Being permanently cash-strapped the system administrator couldn't just install another server. Luckily, the developer found some rock-bottom library that still supports this, ofc there is no ORM so SQL strings have to be built by hand... I don't expect him to parametrize them, I look forward to reviewing this thing!

    Yes, you read that over and over, this time it's my turn!

  • Prime Mover (unregistered) in reply to emurphy

    I worked with a colleague whose style was very much like that. And he deliberately removed all unnecessary spaces from his code, and made his variable names as short as he could get away with. And he wrote everything in uppercase.The net result was a wall of uppercase.

    The reason for this was twofold: he was elderly, and had learned his craft on the Oldest of Old Fortran, hence he was never able to get out of the mindset that short variable names was all he was allowed to use, and that the only character set available was uppercase. However, he was delighted to be able to put more than one command on the same line.

    I questioned him about why he thought it was better to cram everything up together, and how much more readable it would be if he were to spread things out a little. "Ah yes," he replied, "but then I wouldn't be able to see so much on the screen at one time."

    And then I looked at his programming environment. He was using a text editor which was configured to fit just 24 lines on the screen, and 80 characters to the line. This text editor was displayed in a window which took up about a sixth of his screen space. "Why don't you make your window bigger, and show more lines, and ..."

    "Ah, but then there's too much on the screen and I get lost. While you're here, can you help me find out where my bug is? My program Doesn't Work."

  • D (unregistered) in reply to Anon

    And I hope you don't mean to sanitize it by removing all interesting characters like ",',>,<, etc. Not only are those perfectly legitimate things for a user to type, but removing them is a short-sighted fix.

    It's an extra layer of protection. Where I work, we block a small set of characters (by default) — not because we lack proper protections against XSS and other such attacks, but because we know that when the data leaves our system, it goes into a nasty tangle of adapters, often written in Perl. And while strictly speaking, they're not our problem, it's easier for us to block '|' characters from being entered than to argue over the consequences when someone tries to write them into a pipe-delimited interchange file that lacks a specification for escaping characters.

  • Oscar (unregistered) in reply to Naomi

    "And yet you have to sanitize input to defend against XSS attacks."

    No, you don't. You need to properly escape your output.

    Input sanitizing is unnecessary: even for database input/queries etc.: you use prepared statements instead of constructed strings to prevent SQL injection.

    "Sanitizing" your input usually breaks if you have multiple outputs: HTML, Javascript/JSON, CSS, PDF, ... all have different escaping rules. And a modern (web) application usually uses at least two of these.

  • Luchs (unregistered)

    Well, they could "fix" their code by creating an array of bools with the length of the passwords if equal length. Write "true" or "false" to each element in the array and then iterate over the whole array and check if all values are true. If they're not, the passwords aren't equal.

    {true,true,false,true} would be "ship" and "shup" and thus not equal anymore, complicating the code even further. Hashing an entry and comparing the hashes with a simple string comparison is too easy for advanced programmers - they need the challange!

    In other words: It can always be worse! :D

  • (nodebb) in reply to Sole Purpose Of Visit

    In fact, I do see an interest in working with two char arrays instead of strings: In Java and .Net, unlike strings, character arrays can be modified. Which means, they can be cleared after use.

    .Net offers the SecureString class that does this better than you, but it's limited to whatever will support returning one or accept one as input. And there's slightly more APIs supporting character arrays than there are supporting SecureString. Still not enough, though.

  • (nodebb) in reply to Sole Purpose Of Visit

    In fact, I do see an interest in working with two char arrays instead of strings: In Java and .Net, unlike strings, character arrays can be modified. Which means, they can be cleared after use.

    .Net offers the SecureString class that does this better than you, but it's limited to whatever will support returning one or accept one as input. And there's slightly more APIs supporting character arrays than there are supporting SecureString. Still not enough, though.

  • MiserableOldGit (unregistered) in reply to Prime Mover
    I worked with a colleague whose style was very much like that. And he deliberately removed all unnecessary spaces from his code, and made his variable names as short as he could get away with. And he wrote everything in uppercase.The net result was a wall of uppercase.

    I was in a place once with a whole bunch of old-school C programmers who worked at the exact opposite extreme, 3 or four carriage returns between each line, indents where about 12 spaces each (spaces, never tabs) and every function and variable name was a CamelCaseEssayDescribingWhatItWasGoingToBeWhenDeclaredObjectWriteObjectNow, bot not really what it was doing now, natch.

    We had reasonably big screens, but reading anything more than a simple block involved paging up and down and left and right several times and trying not get lost. While/switch constructs were the antipattern of choice there, with the counter argument driving the switch choice and being modified inside the case statements, they'd frequently lose track of their indent level so you'd be hunting around for a stray curly brace in a sea of whitespace, or a sneaky goto/label jump between cases.

  • ip-guru (unregistered) in reply to Steve_The_Cynic

    it is if the "Ship" is a Sloop :-)

  • jd (unregistered)

    There is a reason why password handling in Java is done through char[] and not Strings: Strings can be stored effectively for the entire duration of application runtime in String pools, which is why they are passed around as char arrays (for example see JPasswordField from Swing libraries and it's getPassword method). This is of course not helping the plaintext passwords stored in DB, but on it's own is actually a correct way to pass plaintext passwords (eg. before getting them hashed) around.

  • Anon (unregistered) in reply to Naomi

    Yes you can include unsanitized user input in your database queries. If you think you can't, you are doing it wrong. Google "parameterized queries".

    And no you don't have to sanitize input to defend against XSS attacks. Assuming HTML is the only client for the data is profoundly misguided. If you are rendering data in an HTML client you should be properly encoding the data before rendering in that client. Leave the other clients to encode the data as appropriate for their front-end.

  • Anon (unregistered) in reply to Medinoc

    I was going to ask in what situations you found SecureString to be useful, but then I found a remark in the .Net documentation that recommends not using it anymore.

    https://docs.microsoft.com/en-us/dotnet/api/system.security.securestring?view=netframework-4.8#remarks

    The remark points to this as justification:

    https://github.com/dotnet/platform-compat/blob/master/docs/DE0001.md

  • Naomi (unregistered) in reply to Anon

    I know what a parameterized query is, thank you very much.

  • (nodebb)

    And if you're wondering why they need to look at each row instead of just seeing a non-zero number of matches, so am I.

    I thought this was pretty obvious. The original query just got all the usernames, then looped over them to see if there was a match.

    Then someone added the WHERE clause to the query to improve the database performance, but didn't bother changing the calling code.

Leave a comment on “Win By Being Last”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article