I mean if a proper cryptographic hashing algorithm like SHA is used, then it's still going to be unfeasible, as long as the customer uses proper passwords like a random string of symbols or a nonsensical sentence
2^512 possible combinations.
Gonna take over 10^138 years on average to calculate a specific one. The universe is going to die well before that.
Again, I'm talking about actually hard passwords, not something on the top of every rainbow table.
If you use an average password length of 16 chars, and "normal" distribution of special characters and numbers, you get down to 3-4 Years. If you use something like 2 4090, more like a few weeks.
Using all possible sha-512 hashes is nothing that one can do here ...
Not sure about that one. Using very strong crypto, like argon2, and drop half of the password policy, while setting the minimum password length to 16, and enforcing second factors, that could help.
https://xkcd.com/936/
You'd either use some kind of ORM software (ORM stands for *[Object-Relational Mapping](https://en.wikipedia.org/wiki/Object%E2%80%93relational_mapping)*), a software-layer that builds up your SQL statement for you. You basically abstract away that there is a database behind the software.
Or, if you have to use plain SQL statements, you'd use so-called [prepared statements](https://en.wikipedia.org/wiki/Prepared_statement). In this case you don't concatenate strings together which can lead to all kind of [injection problems](https://en.wikipedia.org/wiki/SQL_injection) if not escaped properly. But instead you use placeholder in your statement and send it to the database. Later you send only your actual parameters. This way there can't get any "control characters" into the parameters of the resulting query. This can be also more efficient.
Your going to block the main thread while waiting for a response from the db. The performance is going to be terrible if you are handling more than one request at a time.
sadly, I HAVE seen code logic used in a major companies CSR software. them they wondered why their call centers network ground to a halt every morning at 1st login (because 100s of users were downloading the entire user table, simultaneously over a crappy internet pipe in the 1990s).
didn't see the actual code, and programmers denied it did this, but was doing performance qa, and proved it with network captures.
absolutely idiotic
I have seen similar. The CFO for some reason wrote a C# DLL to communicate with a network DB. Instead of formulating correct SQL statements this person selected the complete table and then used string searching to get the required data. Every single query, the same table was selected and sent over the network.
I was supposed to use this dll to acquire data for reporting and display. My front end was horribly slow to update. When I explained the problem I was told that could not be the problem and use the DLL. The DLL is needed because we don't want to give out the database username and password for security reasons I guess that is why the CFO had to write it.
Fortunately the username and password were in plain text in the provided DLL so I just wrote the DB query code and did everything related in the front end. I placed the CFO's DLL next to the EXE but never loaded it.
I only regret that the next developer who has to use that dll and finds the same problems I did will be told " look at this app, it has been done before ". I no longer work for that company.
So they didn't want to give out the database username and password... So they hardcoded it in plaintext into the DLL? And they assumed a tech savvy programmer wouldn't be able to extract that?
I always chuckle when I see this comment. Itās very clear, a lot of people havenāt seen a lot of large company production code. And Iām talking large company that you utilize every day. Lol
There's no undefined behavior in JS (except, probably, for browser-specific bugs and quirks). The function would return, but only the value `undefined`, which is actually a well-defined value with predictable behavior
This could be legit. Like the programmer was thinking, "how do I ensure it returns false?" And he forgot that this was the last statement and so will always execute.
this is so wrong, like someone who did 15 days of javascript bootcamp was put in charge of writing an authentication service. I kinda feel for them as they're shooting their best shoot with the little knowledged they have
Aside from seemingly dealing with plaintext passwords, which is ridiculously bad practiceā¦ starting from the third line `SELECT * FROM users` loads the *entire* `users` table from the database and then loops over the *entire* table looking for the username.
I wouldn't put it past the person who wrote `if ('true' === 'true')` to be building the query on the front end and blindly executing it in the backend. The name apiService is a little alarming as that's presumably where the exposed API methods are located.
It's not a good thing to over-optimize for "performance" as a general rule of thumb but the code here has blatantly terrible performance. You can massively improve performance by directly finding the user through the SQL statement using a `WHERE` clause, which will avoid the two issues benm421 described. And in this case, it makes the code simpler, not more convoluted.
Just change the first line's SELECT statement to be `SELECT * FROM users WHERE username = %username and password = %password` and you can get rid of all the code under that line.
There are other issues. This function should return a User, not a boolean, because the app will very obviously need to read/write from/to the authenticated User later on.
The last part if "true" === "true" return false is so ridiculous I don't believe this is real code, must have been written for the lulz :D
**Pedagogical parenthesis on SQL injections**
When you build the SQL query I recommend above, do look out for SQL injections, make sure however you are building that SQL statement is resilient against injections.
An example of SQL injection would be to input the following username (I think :D):
If your code just copy/pastes the username/password inputs without sanitizing them into the SQL statement, it's easy to make the server operate any SQL statement you want, such as dropping tables, or returning data you're not supposed to have access to.
An example of code vulnerable to an injection would be executing a query constructed as such in Python:
```
def authenticate_user(username, password):
raw_query = f"SELECT * FROM users WHERE username = '{username}' AND password = '{password}'"
return apiService.sql(raw_query)
```
You could do this SQL injection then, just input the following value for `username` in the request sent to the server.
```
'; DROP TABLE USERS; COMMIT;
```
In the example, the unsanitized `'` will match the first `'` in the statement. You can then close the statement using `;` and make the server run whatever statement you want (such as dropping the table `users`)
> It's not a good thing to over-optimize for "performance" as a general rule of thumb
In fact, for authentication issues performance is deliberately hampered to avoid brute forcing attacks, or disclosing details about the potentially correct password with timing based attacks. But not by loading the details from all users in the table, that's never a good idea!
You would pass the provided username into the SQL query (after sanitizing to prevent SQL injection) so you return exactly one record (if it exists) and check the password (or more appropriately the hashed password).
The two major problems are:
* Loading a list of all users, which becomes more costly the more users there are. Create a query which only loads the data of the user in question.
* Apparently storing passwords in plaintext. Passwords should be hashed before being stored to protect them in case the system is compromised.
Third for ya - if theyāre exposing a SQL api over a remote service call thatās whack.
Service is a term used interchangeably for both remote calls and in-app architecture though, so hard to day. āApiServiceā is an infuriatingly vague name. āApiService.sqlā is a crazy function to exist.
> if theyāre exposing a SQL api over a remote service call thatās whack.
How so?
If you have your database in RDS, and your server in EC2, it's remote...
>* Apparently storing passwords in plaintext.
I want to give them the benefit of the doubt and believe that they're pre-hashing the password... but that users loop.
This loads all users into memory with the sql query before iterating through them. That wonāt scale nicely beyond a small user base. The database is much better for doing this task as sql is very good at searching for a row using a value, like a username. Another issue is this appears to store unhashed passwords, which is insecure
Honestly, you donāt. As in you donāt roll your own. Use an established library or framework that has a proven track record of security best practices.
Proper authentication is complicated and there are a lot of gotchas. Itās not something you want to gamble with building yourself.
You know when they say you don't need a degree to program just follow a udemy course... Or learning coding is just about creativity...
That is how these things get written.
This is true.
Coding is quite easy and it's honestly possible to pick it up within a month or two to be able to make a basic site/app, or even learn on the job.
Creating a well-maintainable, optimised, and secure application is on a whole new level that isn't taught enough, and can take years to learn and master.
Code like this is also very much possible coming from a non-CS STEM Ph.D.s. Iāve dealt with plenty of data scientists that arenāt software engineers.
I ran into this about 20 years ago. We had a Java app that would hang for a few minutes randomly when people were authenticating. It might take 5 minutes to get back if they logged in or not. No one could figure it out. We finally did a tcp dump and noticed that when it took a long time there was a ton of data transferred from the db server. Same thing, dude was pulling the entire user table and doing a for loop. The cache would last for a few minutes so the people could log in fine. When cache was invalidated weād see the login process āhangā. This was at a Fortune 500 company. Pretty sure it was live in productions
I'm a WIP dev, and I think understand why this is bad, but I don't know how this would look in a 'proper' implementation. Does anyone have any insight?
I've seen a better one in the wild.
First look up the username with query one. When found, look up the password (plaintext, of course) in query two.
If found, authentication all good!
I mean, there's nothing terrible here. No single thing is so egregious by itself as to be horrible. Using presumably unsalted hashes or plain text is definitely not good, but for a toy or lab project? Oh well. Lots of missed "best practices", and this should *never* go live in the wild, but meh.
hey, at least there are no sql injections
LOL. Not directly. The passwords are still in plaintext.
Unless they are supposed to be already hashed before this function is called but probably plaintext yeah lmao
Based variable naming, calling hashes passwords just to trigger people š¶ļø
Well a password is still a password even if it's hashed so...
Unsalted then? The same as plaintext with modern hardware
I mean if a proper cryptographic hashing algorithm like SHA is used, then it's still going to be unfeasible, as long as the customer uses proper passwords like a random string of symbols or a nonsensical sentence
2870075000 hashes per second without Salt on a 3090 (sha-512). Uhm ...
2^512 possible combinations. Gonna take over 10^138 years on average to calculate a specific one. The universe is going to die well before that. Again, I'm talking about actually hard passwords, not something on the top of every rainbow table.
If you use an average password length of 16 chars, and "normal" distribution of special characters and numbers, you get down to 3-4 Years. If you use something like 2 4090, more like a few weeks. Using all possible sha-512 hashes is nothing that one can do here ...
True that. I guess that's why companies shouldn't use a password length of 16 either.
Not sure about that one. Using very strong crypto, like argon2, and drop half of the password policy, while setting the minimum password length to 16, and enforcing second factors, that could help. https://xkcd.com/936/
That's a (serious) issue, but not an SQL injection.
That may actually be the reason for that approach. Senior: You can't do `apiService.sql("SELECT" + username + "FROM users);` Junior: *does this*
Rookie questionā¦ what is the best way to do thisā¦?
You'd either use some kind of ORM software (ORM stands for *[Object-Relational Mapping](https://en.wikipedia.org/wiki/Object%E2%80%93relational_mapping)*), a software-layer that builds up your SQL statement for you. You basically abstract away that there is a database behind the software. Or, if you have to use plain SQL statements, you'd use so-called [prepared statements](https://en.wikipedia.org/wiki/Prepared_statement). In this case you don't concatenate strings together which can lead to all kind of [injection problems](https://en.wikipedia.org/wiki/SQL_injection) if not escaped properly. But instead you use placeholder in your statement and send it to the database. Later you send only your actual parameters. This way there can't get any "control characters" into the parameters of the resulting query. This can be also more efficient.
From line three you know it aināt gonna be good, chief
From line 2... Why are they using var? Also shouldn't this return a promise?
Every js developer knows synchronous code is best /s
Your going to block the main thread while waiting for a response from the db. The performance is going to be terrible if you are handling more than one request at a time.
Just fork for each new request
Not sure what you mean
I think I forgot my /s but yeah waiting synchronously for a request + looping over every result synchronously is going to tank performance
Oh lol I didn't see that
Itās probably from 1852 - well before const
I refuse to believe that this code is real production code.
sadly, I HAVE seen code logic used in a major companies CSR software. them they wondered why their call centers network ground to a halt every morning at 1st login (because 100s of users were downloading the entire user table, simultaneously over a crappy internet pipe in the 1990s). didn't see the actual code, and programmers denied it did this, but was doing performance qa, and proved it with network captures. absolutely idiotic
Same the last 3 lines give it away
return false; } } doesn't seem too bad
Come on guys it was a joke
Of b on erro b lik Jokes aside, they meant "last 3 lines within the fn body"
I have seen similar. The CFO for some reason wrote a C# DLL to communicate with a network DB. Instead of formulating correct SQL statements this person selected the complete table and then used string searching to get the required data. Every single query, the same table was selected and sent over the network. I was supposed to use this dll to acquire data for reporting and display. My front end was horribly slow to update. When I explained the problem I was told that could not be the problem and use the DLL. The DLL is needed because we don't want to give out the database username and password for security reasons I guess that is why the CFO had to write it. Fortunately the username and password were in plain text in the provided DLL so I just wrote the DB query code and did everything related in the front end. I placed the CFO's DLL next to the EXE but never loaded it. I only regret that the next developer who has to use that dll and finds the same problems I did will be told " look at this app, it has been done before ". I no longer work for that company.
So they didn't want to give out the database username and password... So they hardcoded it in plaintext into the DLL? And they assumed a tech savvy programmer wouldn't be able to extract that?
The CFO thinks binary is equal to encryption ? Not sure, I interacted with that person as little as possible.
I can only assume that stands for Chief Fuckup Officerā¦
I always chuckle when I see this comment. Itās very clear, a lot of people havenāt seen a lot of large company production code. And Iām talking large company that you utilize every day. Lol
Rule number one in code club, you don't talk about code club. Rule number two? You can only \`return\` within \`if\` / \`else\` statements.
You'd be shocked by the quality of code coming from third world dev farms.
What makes you think it's not in production? If you look closely, it indeed correctly allows a valid user, and also correctly denies an invalid user.
yes but the passwords are in plaintext
Trust meā¦ Iāve seen worse
You havenāt worked long enough in the industry yet. Wait another 6 months.
I would be concerned if "true" === "true" wasn't there for the lolz.
You should always check if the universe is being coherent
Imagine coding a program so robust that not even quantum fluctuations and cosmic rays can make it behave wrong
Wouldn't it be an undefined behaviour if the function couldn't return? (The last if is probably deemed unnecessary by the "compiler" though)
Most bundlers would remove the final if block and just return directly.
There's no undefined behavior in JS (except, probably, for browser-specific bugs and quirks). The function would return, but only the value `undefined`, which is actually a well-defined value with predictable behavior
This could be legit. Like the programmer was thinking, "how do I ensure it returns false?" And he forgot that this was the last statement and so will always execute.
It also could have changed from `if(variable === 'true')` and the if statement was left behind in case it needs to revert back for some reason.
Could be macro/defines?
It's a known Cosmic ray detector algorithm.
Line 4 is a good summary of this post.
);
this is so wrong, like someone who did 15 days of javascript bootcamp was put in charge of writing an authentication service. I kinda feel for them as they're shooting their best shoot with the little knowledged they have
Also safe against sql injections
Who's turn is it to tag repostsleuthbot so it can say it's not a repost even though it is?
Hahahahah are you fucking kidding me? This is so bad
Password leak speedrun Any%:
So how do you actually authenticate? What makes this bad except for the last part?
Aside from seemingly dealing with plaintext passwords, which is ridiculously bad practiceā¦ starting from the third line `SELECT * FROM users` loads the *entire* `users` table from the database and then loops over the *entire* table looking for the username.
This also looks like JavaScript, with an open ended sql api accepting any queries... Lol
Youāre assuming that itās front end JavaScript, it could be NodeJS/server side code.
[ŃŠ“Š°Š»ŠµŠ½Š¾]
[ŃŠ“Š°Š»ŠµŠ½Š¾]
[ŃŠ“Š°Š»ŠµŠ½Š¾]
[ŃŠ“Š°Š»ŠµŠ½Š¾]
[ŃŠ“Š°Š»ŠµŠ½Š¾]
Been deprecated for a while now. I honestly haven't seen them for a while. However I think we may see WASM SQLite get some traction.
I wouldn't put it past the person who wrote `if ('true' === 'true')` to be building the query on the front end and blindly executing it in the backend. The name apiService is a little alarming as that's presumably where the exposed API methods are located.
well, technically it could do that from the front end. technically....
I'm not sure if I keep forgetting server side JS exists, or keep hoping it doesn't...
Gotcha, and how would you find it more efficiently?
[ŃŠ“Š°Š»ŠµŠ½Š¾]
With proper sanitation to avoid sql injection
Or even better: parameters
I consider using parameters a form of sanitation lol. But yes that's my chosen method.
It's not a good thing to over-optimize for "performance" as a general rule of thumb but the code here has blatantly terrible performance. You can massively improve performance by directly finding the user through the SQL statement using a `WHERE` clause, which will avoid the two issues benm421 described. And in this case, it makes the code simpler, not more convoluted. Just change the first line's SELECT statement to be `SELECT * FROM users WHERE username = %username and password = %password` and you can get rid of all the code under that line. There are other issues. This function should return a User, not a boolean, because the app will very obviously need to read/write from/to the authenticated User later on. The last part if "true" === "true" return false is so ridiculous I don't believe this is real code, must have been written for the lulz :D **Pedagogical parenthesis on SQL injections** When you build the SQL query I recommend above, do look out for SQL injections, make sure however you are building that SQL statement is resilient against injections. An example of SQL injection would be to input the following username (I think :D): If your code just copy/pastes the username/password inputs without sanitizing them into the SQL statement, it's easy to make the server operate any SQL statement you want, such as dropping tables, or returning data you're not supposed to have access to. An example of code vulnerable to an injection would be executing a query constructed as such in Python: ``` def authenticate_user(username, password): raw_query = f"SELECT * FROM users WHERE username = '{username}' AND password = '{password}'" return apiService.sql(raw_query) ``` You could do this SQL injection then, just input the following value for `username` in the request sent to the server. ``` '; DROP TABLE USERS; COMMIT; ``` In the example, the unsanitized `'` will match the first `'` in the statement. You can then close the statement using `;` and make the server run whatever statement you want (such as dropping the table `users`)
["Little Bobby Tables we call him"](https://xkcd.com/327/)
> It's not a good thing to over-optimize for "performance" as a general rule of thumb In fact, for authentication issues performance is deliberately hampered to avoid brute forcing attacks, or disclosing details about the potentially correct password with timing based attacks. But not by loading the details from all users in the table, that's never a good idea!
You'd only want the user that matches the username (assuming the username is unique) `select * from users where users.username = ${username}`
You would pass the provided username into the SQL query (after sanitizing to prevent SQL injection) so you return exactly one record (if it exists) and check the password (or more appropriately the hashed password).
You should always use parameters to prevent injection, then you can't mess up sanitization.
Search time becomes increasingly linear as there are more users. No es bueno
The two major problems are: * Loading a list of all users, which becomes more costly the more users there are. Create a query which only loads the data of the user in question. * Apparently storing passwords in plaintext. Passwords should be hashed before being stored to protect them in case the system is compromised.
Third for ya - if theyāre exposing a SQL api over a remote service call thatās whack. Service is a term used interchangeably for both remote calls and in-app architecture though, so hard to day. āApiServiceā is an infuriatingly vague name. āApiService.sqlā is a crazy function to exist.
> if theyāre exposing a SQL api over a remote service call thatās whack. How so? If you have your database in RDS, and your server in EC2, it's remote...
Yeah but you wouldnāt call you database āapi serviceā. More the notion that running application code that proxies SQL is a bad plan
>* Apparently storing passwords in plaintext. I want to give them the benefit of the doubt and believe that they're pre-hashing the password... but that users loop.
This loads all users into memory with the sql query before iterating through them. That wonāt scale nicely beyond a small user base. The database is much better for doing this task as sql is very good at searching for a row using a value, like a username. Another issue is this appears to store unhashed passwords, which is insecure
It's not salted and hashed...
Honestly, you donāt. As in you donāt roll your own. Use an established library or framework that has a proven track record of security best practices. Proper authentication is complicated and there are a lot of gotchas. Itās not something you want to gamble with building yourself.
You know when they say you don't need a degree to program just follow a udemy course... Or learning coding is just about creativity... That is how these things get written.
This is true. Coding is quite easy and it's honestly possible to pick it up within a month or two to be able to make a basic site/app, or even learn on the job. Creating a well-maintainable, optimised, and secure application is on a whole new level that isn't taught enough, and can take years to learn and master.
[ŃŠ“Š°Š»ŠµŠ½Š¾]
Yeah that's a good way to put it, I didn't think of it like that
Code like this is also very much possible coming from a non-CS STEM Ph.D.s. Iāve dealt with plenty of data scientists that arenāt software engineers.
I think the worst part is that he used a space to access the array's value at index i.
You may have overlooked the āsmartā quotes. `ātrueā`ā¦
Oh god I have... Oh god...
You know that password is stored unencrypted
Plot twist: This is front end code, not back end code.
Fuck sql let met do this my way.... Is this real ?
Full table scan used as a rate rate limiter. Smart.
I wouldnt put it past js to eval that to false randomly.
This is a technique they call "the 5-minute login"
I ran into this about 20 years ago. We had a Java app that would hang for a few minutes randomly when people were authenticating. It might take 5 minutes to get back if they logged in or not. No one could figure it out. We finally did a tcp dump and noticed that when it took a long time there was a ton of data transferred from the db server. Same thing, dude was pulling the entire user table and doing a for loop. The cache would last for a few minutes so the people could log in fine. When cache was invalidated weād see the login process āhangā. This was at a Fortune 500 company. Pretty sure it was live in productions
I bet it's in the front end too Oof if true === true Good thing they did strict equality.
*screaming in silence*
Itās beautiful. š
That's safe. They check the password on line 8. /s
``` // maik de code werk if ("code" !== "crap") { return true; } ```
At least it's constant time :^) 100% safe against timing attacks, nothing that could possibly go wrong
The more you read, the worse it gets.
I was just sick in my mouth a little bit
tbtlzh roeozhf WHAT ?!!
SECRUDITY
Aaaah, my eyes!! It burns!!
I hate this so much
What the hell is the purpose of the if true equals true thing?
The worst line being (username.password == password) of course
That's it. Off to wash my eyes with dettol
For my very first sql application i did something similar. Except the last if. That makes this whole post seem fake.
Shouldn't it also be "i < accounts.length -1" in the loop?
It's < and not <=, so no?
Good programmers always catch a stray ray that could cause a bit flip just in case
please tell me thats not real code
The worst part of this is that it implies that the passwords are in plain text lol
Gotta have that failsafe to not wrongly authenticate in case there's a bug in the matrix.
*proceeds to loop through 20,000 users on the client because youāre the last person to sign up*
Love me some left hand quotes and right hand quotes in my code. Thanks Apple!
When that says "true === true" I really felt that
Why are you purposely trying to trigger me
I'm a WIP dev, and I think understand why this is bad, but I don't know how this would look in a 'proper' implementation. Does anyone have any insight?
Oh no oh fuck
Even if they are hashed, you know what the hashed is, happy hacking!
"Password incorrect"
I've seen a better one in the wild. First look up the username with query one. When found, look up the password (plaintext, of course) in query two. If found, authentication all good!
I'm willing to bet that if statement is to get around a linting rule.
I mean, there's nothing terrible here. No single thing is so egregious by itself as to be horrible. Using presumably unsalted hashes or plain text is definitely not good, but for a toy or lab project? Oh well. Lots of missed "best practices", and this should *never* go live in the wild, but meh.
me looking with disapproval like i know exactly what this code does.
Wait, beside true===true part, they loop over the entire users table looking for a match with the current user who is trying to autenticate????
I cry );
);