๐๏ธ Emcf ยท 387 points ยท Posted at 00:41:10 on September 6, 2015 ยท (Permalink)
Squishumz ยท 126 points ยท Posted at 01:48:31 on September 6, 2015 ยท (Permalink)
I love how its wrong both in what it's doing and how it's doing it.
YMK1234 ยท 110 points ยท Posted at 07:47:07 on September 6, 2015 ยท (Permalink)*
Lets try to make a list ...
emails and passwords in plaintext file, instead of DB
not even a CSV file but implicitly associated via line numbers
passwords in plain text
empty else if blocks
redundant comparison in else if blocks
using foreach plus Array.IndexOf to find an index while you could do it directly with for
not comparing normalized version of email address (i.e. trim and toLowerCase)
I bet you the store code does the same (i.e. if you enter " myemail@example.com " it will be stored with those spaces and you will never be able to login ever again)
the whole first foreach being actually redundant because Array.IndexOf
null/whitespace checks for email and password could speed things up considerably
EDIT:
possibility of doing timing attacks against user names at least iterating over the whole array of usernames as pointed out by /u/SadKisser so at least we can't time onto it
always validates against password[0], even if your username does not match (as pointed out by /u/snotfart)
assuming that password file has the same number of lines as the email file (if its shorter, you get out-of-range exception)
EDIT2: proposals at fixing:
only use Array.IndexOf to find the row. That will at least give you "-1" for the case of not having a username and clean up the code greatly
null checks and sanitization (trim and lowercase for email) for the inputs
actually returning a login status and not setting a Text ;)
SadKisser ยท 24 points ยท Posted at 17:32:28 on September 6, 2015 ยท (Permalink)
Iterating through the entire email list, even if the email is found.
YMK1234 ยท 6 points ยท Posted at 17:40:18 on September 6, 2015 ยท (Permalink)
uuuh, good one ... but that means we can't timing-attack on existing users ... will edit my post
emails and passwords in plaintext file, instead of DB
Nah, it's totally secure! Didn't you see the Base64Decode??? If anyone opened "passwords.txt" there is no way they could possibly ever know that they weren't the actual plaintext passwords, but instead actually base64 versions!
It's almost as secure as the password encryption I use:
Vg'f rffragvnyyl vzcbffvoyr sbe rira gur AFN gb penpx guvf pbqr. Tbbq yhpx gelvat naq snvyvat, rirelbar. OGJ, zl erqqvg cnffjbeq vf "Tbq" ohg ab bar jvyy rire xabj!
YMK1234 ยท 1 points ยท Posted at 20:48:15 on September 6, 2015 ยท (Permalink)
you definitely should always use equals, but (as I said, iirc ... didn't do much c# lately) "==" comparison should cut it normally as well (but again, extremely bad practice)
I'm just a beginner and I'm not sure if get this. Could someone explain this to me and others like me.
randfur ยท 52 points ยท Posted at 07:49:23 on September 6, 2015 ยท (Permalink)
So glad you asked, I'm happy to oblige.
Reading from files at every invocation. Disk IO is orders of magnitude slower than memory operations so this will scale very poorly at high queries per seconds (QPS).
Passwords are encoded in base64. Note that this is an encoding scheme and not an encryption. There is zero user security provided by doing this, the base64 encoding suggests the writer is clueless at basic security concepts and just thinks that if they can't read it then no one can. Granted it is possible that these are base64 encoded salted and hashed passwords but somehow I doubt that...
Using a foreach construct to find the index of an entry in the array. (The reader's health starts to degrade here.) If they had just done "for (int i = 0; i < emails.length(); i++)" then they would have the index as soon as they find it or even better just call Array.indexOf() but no, they have to loop over the whole thing until they find a match so that they can call Array.indexOf() which does fucking exactly the same thing except it returns the index which they could have done had they not tried to find an index using a fucking foreach construct which was designed to abstract away the notion of indices in lists!
Checking else if <negation of original conditional> is equivalent to just using a simple else block, all you're doing here is making the code more vulnerable to bugs when someone updates the original if condition and doesn't think to update the else condition to be the exact opposite because who the fuck would write that?
Empty else block. (Aneurysm begins forming.)
No handling for the case where the email is not in the list, it goes along merrily as if you had provided the first email.
Useless comment, not to mention broken English. Comments should never say what code does (unless the code is hard to decipher), it should say why the code is necessary.
I highly suspect the password provided was transmitted unsalted and unhashed through the wire to be string compared with a base64 encoded stored copy of the password. It's possible that all the reasonable security measures are being done elsewhere outside of the Login() function but again, somehow I doubt that.
Repeat of #4.
Repeat of #5.
No return value. The only effect a successful (or exploited) login has on the state of the program is to update a text field. Usually other parts of the system will want to know whether the login was successful or not before providing access to user sensitive data/capabilities but since this is the only effect logging in has I can only assume those parts of the system go and read the contents of a friggen textbox to determine whether the user is logged in or not. (Reader suffers a stroke at this point and accidentally hits the "Review approved + commit" button as their head slumps on the keyboard.)
Granted it is possible that these are base64 encoded salted and hashed passwords but somehow I doubt that...
In this case it's impossible that the passwords are salted except if they're all salted with the same salt. That's still better than no salting but way worse than proper salting.
Pardon my ignorance, but what does salting a password mean? Your comment is hilariously absurd without that knowledge.
seledorn ยท 8 points ยท Posted at 19:34:02 on September 6, 2015 ยท (Permalink)
Usually you add an user-unique offset to the hash so that same passwords don't hash out the same. In case of a breach you wouldn't want the anyone to know who's sharing passwords with whom.
In this case he's saying that they could each share the same salt in which case the same passwords would hash the same but at least you can't use a decoding table for the most common passwords (Rainbow table).
FPJaques ยท 1 points ยท Posted at 12:32:54 on January 13, 2016 ยท (Permalink)
TIL...
I always used the same salt for my hashed PWs (granted, I didn't do anything critical with it, just for an app for learning programming).
Didn't think about the password sharing part, just considered the prevention of rainbow tables
Kwpolska ยท 2 points ยท Posted at 10:45:16 on September 6, 2015 ยท (Permalink)
Passwords are encoded in base64. Note that this is an encoding scheme and not an encryption. There is zero user security provided by doing this, the base64 encoding suggests the writer is clueless at basic security concepts and just thinks that if they can't read it then no one can. Granted it is possible that these are base64 encoded salted and hashed passwords but somehow I doubt that...
Itโs very unlikely, because login functions usually work with plaintext passwords (eg. from POST to a server).
Banane9 ยท 1 points ยท Posted at 20:34:58 on September 7, 2015 ยท (Permalink)
Well, technically number 4 doesn't matter, as there's nothing in the blocks! ;)
snotfart ยท 3 points ยท Posted at 07:16:39 on September 6, 2015 ยท (Permalink)
There are a lot of things wrong with this code, so I'll start you off with a few of them.
Starting with the general program flow, it checks for a password separately from checking the login name, so even if you don't put in a correct login name, it'll check your password against the one for user 0.
It looks like the passwords are "encrypted" with base64.
Both "else"s are followed by another if..., which is unnecessary as the fact that you are at the "else" means that it's true. Also, no code is executed anyway, so the "else" isn't needed either.
The login name is checked with "==" so if your name is "dave" and you log in with "Dave" it'll fail.
YMK1234 ยท 2 points ยท Posted at 07:48:45 on September 6, 2015 ยท (Permalink)
I like how you only need to remember the password to login as the first user. (Which probably also is an admin account) I always get my emails mixed up.
Squishumz ยท 126 points ยท Posted at 01:48:31 on September 6, 2015 ยท (Permalink)
I love how its wrong both in what it's doing and how it's doing it.
YMK1234 ยท 110 points ยท Posted at 07:47:07 on September 6, 2015 ยท (Permalink)*
Lets try to make a list ...
EDIT:
possibility of doing timing attacks against user names at leastiterating over the whole array of usernames as pointed out by /u/SadKisser so at least we can't time onto itEDIT2: proposals at fixing:
SadKisser ยท 24 points ยท Posted at 17:32:28 on September 6, 2015 ยท (Permalink)
Iterating through the entire email list, even if the email is found.
YMK1234 ยท 6 points ยท Posted at 17:40:18 on September 6, 2015 ยท (Permalink)
uuuh, good one ... but that means we can't timing-attack on existing users ... will edit my post
lanerdofchristian ยท 11 points ยท Posted at 14:56:02 on September 6, 2015 ยท (Permalink)
That's a lot of ouch.
One correction, the user part of an email address is case-sensitive, according to RFC 2821 (page 13).
YMK1234 ยท 25 points ยท Posted at 14:57:18 on September 6, 2015 ยท (Permalink)
According to RFC maybe, but I know of no email service that actually does it this way.
lanerdofchristian ยท 6 points ยท Posted at 15:05:21 on September 6, 2015 ยท (Permalink)
Fair enough.
decultured ยท 12 points ยท Posted at 17:47:30 on September 6, 2015 ยท (Permalink)
Nah, it's totally secure! Didn't you see the Base64Decode??? If anyone opened "passwords.txt" there is no way they could possibly ever know that they weren't the actual plaintext passwords, but instead actually base64 versions!
It's almost as secure as the password encryption I use:
bitshoptyler ยท 9 points ยท Posted at 00:09:52 on September 7, 2015 ยท (Permalink)
I feel like that might be rot13, but I'm on mobile and too lazy to check.
YMK1234 ยท 7 points ยท Posted at 17:48:55 on September 6, 2015 ยท (Permalink)
The difference between encoding and encryption ...
DarthTater42 ยท 3 points ยท Posted at 20:36:05 on September 6, 2015 ยท (Permalink)
Using == to compare strings
YMK1234 ยท 9 points ยท Posted at 20:37:55 on September 6, 2015 ยท (Permalink)
well, that actually works in C# iirc, but its not the greatest thing to do if you eg. want invariant equality.
DarthTater42 ยท 3 points ยท Posted at 20:43:18 on September 6, 2015 ยท (Permalink)
Oh interesting. I just know I was taught never to do that.
And the Best Practices for Using Strings in the .Net Framework says to use the String.Equals method.
YMK1234 ยท 1 points ยท Posted at 20:48:15 on September 6, 2015 ยท (Permalink)
you definitely should always use equals, but (as I said, iirc ... didn't do much c# lately) "==" comparison should cut it normally as well (but again, extremely bad practice)
The-Night-Forumer ยท 2 points ยท Posted at 07:24:50 on December 6, 2015 ยท (Permalink)
Why would it be bad practice if the operator was overloaded for the specific purpose of comparing strings?
YMK1234 ยท 1 points ยท Posted at 12:34:18 on December 6, 2015 ยท (Permalink)
As I said in my original post, for instance because
==does not allow to specify the equality type (like invariant culture).The-Night-Forumer ยท 1 points ยท Posted at 16:23:20 on December 6, 2015 ยท (Permalink)
Ah, after looking up invariant culture that makes a lot more sense.
[deleted] ยท 74 points ยท Posted at 01:20:19 on September 6, 2015 ยท (Permalink)
[deleted]
FireCrack ยท 17 points ยท Posted at 03:26:18 on September 6, 2015 ยท (Permalink)
I second that motion, so many things wrong here that I lose count!
On the other hand, it's obviously a toy example (I hope)
YMK1234 ยท 9 points ยท Posted at 15:06:50 on September 6, 2015 ยท (Permalink)
you have no clue what code you can find in production if you look hard enough ...
[deleted] ยท 3 points ยท Posted at 06:34:02 on September 7, 2015 ยท (Permalink)
Whereas this one is flat out awful, it's still not as hilarious as this
doorshavefeelingstoo ยท 26 points ยท Posted at 07:00:34 on September 6, 2015 ยท (Permalink)
I'm just a beginner and I'm not sure if get this. Could someone explain this to me and others like me.
randfur ยท 52 points ยท Posted at 07:49:23 on September 6, 2015 ยท (Permalink)
So glad you asked, I'm happy to oblige.
Reading from files at every invocation. Disk IO is orders of magnitude slower than memory operations so this will scale very poorly at high queries per seconds (QPS).
Passwords are encoded in base64. Note that this is an encoding scheme and not an encryption. There is zero user security provided by doing this, the base64 encoding suggests the writer is clueless at basic security concepts and just thinks that if they can't read it then no one can. Granted it is possible that these are base64 encoded salted and hashed passwords but somehow I doubt that...
Using a foreach construct to find the index of an entry in the array. (The reader's health starts to degrade here.) If they had just done "for (int i = 0; i < emails.length(); i++)" then they would have the index as soon as they find it or even better just call Array.indexOf() but no, they have to loop over the whole thing until they find a match so that they can call Array.indexOf() which does fucking exactly the same thing except it returns the index which they could have done had they not tried to find an index using a fucking foreach construct which was designed to abstract away the notion of indices in lists!
Checking else if <negation of original conditional> is equivalent to just using a simple else block, all you're doing here is making the code more vulnerable to bugs when someone updates the original if condition and doesn't think to update the else condition to be the exact opposite because who the fuck would write that?
Empty else block. (Aneurysm begins forming.)
No handling for the case where the email is not in the list, it goes along merrily as if you had provided the first email.
Useless comment, not to mention broken English. Comments should never say what code does (unless the code is hard to decipher), it should say why the code is necessary.
I highly suspect the password provided was transmitted unsalted and unhashed through the wire to be string compared with a base64 encoded stored copy of the password. It's possible that all the reasonable security measures are being done elsewhere outside of the Login() function but again, somehow I doubt that.
Repeat of #4.
Repeat of #5.
No return value. The only effect a successful (or exploited) login has on the state of the program is to update a text field. Usually other parts of the system will want to know whether the login was successful or not before providing access to user sensitive data/capabilities but since this is the only effect logging in has I can only assume those parts of the system go and read the contents of a friggen textbox to determine whether the user is logged in or not. (Reader suffers a stroke at this point and accidentally hits the "Review approved + commit" button as their head slumps on the keyboard.)
TheOnlyMrYeah ยท 10 points ยท Posted at 10:34:02 on September 6, 2015 ยท (Permalink)
In this case it's impossible that the passwords are salted except if they're all salted with the same salt. That's still better than no salting but way worse than proper salting.
sexual_pasta ยท 5 points ยท Posted at 18:37:20 on September 6, 2015 ยท (Permalink)
Pardon my ignorance, but what does salting a password mean? Your comment is hilariously absurd without that knowledge.
seledorn ยท 8 points ยท Posted at 19:34:02 on September 6, 2015 ยท (Permalink)
Usually you add an user-unique offset to the hash so that same passwords don't hash out the same. In case of a breach you wouldn't want the anyone to know who's sharing passwords with whom.
In this case he's saying that they could each share the same salt in which case the same passwords would hash the same but at least you can't use a decoding table for the most common passwords (Rainbow table).
FPJaques ยท 1 points ยท Posted at 12:32:54 on January 13, 2016 ยท (Permalink)
TIL...
I always used the same salt for my hashed PWs (granted, I didn't do anything critical with it, just for an app for learning programming).
Didn't think about the password sharing part, just considered the prevention of rainbow tables
Kwpolska ยท 2 points ยท Posted at 10:45:16 on September 6, 2015 ยท (Permalink)
Itโs very unlikely, because login functions usually work with plaintext passwords (eg. from POST to a server).
Banane9 ยท 1 points ยท Posted at 20:34:58 on September 7, 2015 ยท (Permalink)
Well, technically number 4 doesn't matter, as there's nothing in the blocks! ;)
snotfart ยท 3 points ยท Posted at 07:16:39 on September 6, 2015 ยท (Permalink)
There are a lot of things wrong with this code, so I'll start you off with a few of them.
Starting with the general program flow, it checks for a password separately from checking the login name, so even if you don't put in a correct login name, it'll check your password against the one for user 0.
It looks like the passwords are "encrypted" with base64.
Both "else"s are followed by another if..., which is unnecessary as the fact that you are at the "else" means that it's true. Also, no code is executed anyway, so the "else" isn't needed either.
The login name is checked with "==" so if your name is "dave" and you log in with "Dave" it'll fail.
YMK1234 ยท 2 points ยท Posted at 07:48:45 on September 6, 2015 ยท (Permalink)
I made a list and don't claim completeness
randfur ยท 23 points ยท Posted at 02:47:26 on September 6, 2015 ยท (Permalink)
I started having breathing problems halfway through.
phoenix616 ยท 36 points ยท Posted at 03:07:51 on September 6, 2015 ยท (Permalink)
I like how you only need to remember the password to login as the first user. (Which probably also is an admin account) I always get my emails mixed up.
myevillaugh ยท 10 points ยท Posted at 03:45:00 on September 6, 2015 ยท (Permalink)
Wow, I missed that bug/flaw in the first reading. There are probably more holes that I haven't noticed yet.
HonorableJudgeHolden ยท 6 points ยท Posted at 09:22:36 on September 6, 2015 ยท (Permalink)
I thought that was the flaw - other than stupid stuff like using "indexof" in a foreach statement.
myevillaugh ยท 8 points ยท Posted at 13:42:07 on September 6, 2015 ยท (Permalink)
Logins and passwords stored in flat files, must be linearly searched. Passwords are not hashed.
HonorableJudgeHolden ยท 5 points ยท Posted at 13:45:13 on September 6, 2015 ยท (Permalink)
Yeah, that's pretty bad too. But if it were like a business app or something where it were expected only 20 people were using it...
Of course, that's still not a reason to NOT hash it.
PLament ยท 13 points ยท Posted at 05:29:08 on September 6, 2015 ยท (Permalink)
The worst part is you can tell they're trying
YMK1234 ยท 3 points ยท Posted at 07:53:56 on September 6, 2015 ยท (Permalink)
dat base64 encoding is so secure, nobody will ever break it!
stone_henge ยท 10 points ยท Posted at 13:47:22 on September 6, 2015 ยท (Permalink)
It's all right guys, the passwords are base64 encoded.
fluoroamine ยท 4 points ยท Posted at 20:43:51 on September 6, 2015 ยท (Permalink)
http://i.giphy.com/IvcwXzJ1HiTTi.gif
jgillich ยท 2 points ยท Posted at 21:12:53 on September 6, 2015 ยท (Permalink)
FTFY
dhicock ยท 10 points ยท Posted at 03:07:16 on September 6, 2015 ยท (Permalink)
I'm genuinely curious to know who thought any of this was a good idea....
[deleted] ยท 5 points ยท Posted at 07:35:14 on September 6, 2015 ยท (Permalink)
I wonder if the matching registration code allows a username or password with \n
fluoroamine ยท 5 points ยท Posted at 20:44:17 on September 6, 2015 ยท (Permalink)
Well you hit enter to login!
HonorableJudgeHolden ยท 7 points ยท Posted at 09:21:38 on September 6, 2015 ยท (Permalink)
Wow... I sincerely hope this was a student project.
fluoroamine ยท 3 points ยท Posted at 20:42:17 on September 6, 2015 ยท (Permalink)
Nah, this is quick on-schedule backend to finance enterprise software probably.
[deleted] ยท 2 points ยท Posted at 15:56:00 on September 6, 2015 ยท (Permalink)
Clearly, the code is highly standardized: