4 ms·

This is totally untrue, considering the caliber of companies which have been hit with SQL injection vulns, including Google, Facebook (your employer), Reserve B

by 0xy 6y ago

This is totally untrue, considering the caliber of companies which have been hit with SQL injection vulns, including Google, Facebook (your employer), Reserve Banks, governments, etc. I don't know what experience you have, but senior engineers constantly make security mistakes. Linux kernel developers are some of the smartest programmers in existence yet they continually introduce and fail to patch security vulnerabilities.

jdxcode 6y ago
I've been on teams building web applications for 14 years and not once have I seen someone simply rip out a query builder like this in production code and replace it with a non-parameterized string—let alone someone with 20+ years of experience. Accidents and SQL injections happen, usually because of non-obvious query-building but this is a different level.
- 0xy 6y ago
  It happened to your employer. You might not have been on the team but this stuff is rife. WhatsApp in particular has made some absolutely rookie-tier security blunders [1]. [1] https://threatpost.com/whatsapp-bug-malicious-code-injection-rce/152578/ https://threatpost.com/whatsapp-bug-malicious-code-injection...
- Cloudef 6y ago
  When swift was still young the only mysql library available for it did the same mistake. The memory safety sure didnt help here... Sure enough i had to roll my own code.
  - jdxcode 6y ago
    Well not all injection vulns deserve this level of scrutiny. I'm not familiar with that vuln, but I don't see how it could be the "same mistake". My guess is there simply wasn't support for parameterization or there was a non-obvious concatenation problem when building the query string—but please correct me if I'm wrong. This instance is novel because the parameterization protection was removed in favor of concatenation and that the vuln is so obvious a first-year CS student wouldn't struggle to identify it.
dj_mc_merlin 6y ago
No senior engineer should blatantly concatenate strings to make an SQL query. That is a junior mistake, no doubts about it. If you haven't learned that user input is not to be concatenated into code unless you think long and hard about it and you're really sure there's no better option, you are not in a position to lead. Seriously, if this is a mistake a senior engineer could do at your company, why would you work there? I'm not one for gatekeeping but this is learned within a couple of code reviews in your first years.
- 0xy 6y ago
  By your logic, nobody should work at Facebook, Google, Apple etc etc etc. Unless you're claiming that those SQL injection exploits were introduced AND review approved by junior engineers, a practical impossibility according to policy. Your claim that senior engineers don't make whopping security blunders is absolutely false according to all available evidence. The smartest programmers working on kernels make them constantly.
  - ericbarrett 6y ago
    Former Facebooker, on the storage team—if a senior engineer* wrote the supplied code, well, I can't say they'd be fired, but it 100% would not reflect well on them come review time. Also, all their peers would know they made the most boneheaded of mistakes, and they would definitely be getting the hairy eyeball come code review. Raw untrusted string substitution into a SQL query is just unacceptable. It's on the level of storing unhashed passwords, or 1000-line god functions: the kind of mistake you'd expect from a self-taught developer writing PHP in 2002. Literally every popular SQL client and ORM provides quoted substitution; please use it. And, dear reader, if this was news to you, and you write SQL, please go get educated, because it's not the last nor the sneakiest pitfall in this space! * Senior enough to be writing raw SQL code at Facebook
    - 0xy 6y ago
      Facebook has had multiple injection exploits, including SQL and JS RCEs. It's possible no senior were the git blame behind them, but it's absolutely impossible that no seniors reviewed and then maintained that code. WhatsApp had a raw JavaScript RCE literally last year. Also, blaming colleagues for making a mistake is extremely toxic behavior. I've fixed dozens of >9.0 CVSS vulnerabilities at multiple billion-dollar organizations. I'd say senior engineers were involved in their introduction at least half of the time.
lawnchair_larry 6y ago
Yeah, I beat this dead horse every time the armchair security quarterbacks climb on their high horse in these comments. These “dumb” mistakes happen to everyone, at every company. Even security experts. Even one of tptacek’s companies had a well known incident, and people said all the same things about their “dumb” mistakes. Companies are very porous, people take shortcuts, temporary workarounds get forgotten and left in place, etc. You can tell who actually has real experience by who is saying this and getting downvoted for it, ironically. All of the downvoted comments in this thread are correct.
- dj_mc_merlin 6y ago
  I take the same standpoint as you most of the time. But you can't fucking interpolate user input into SQL queries without physically cringing and call yourself an experienced engineer. Can you really pull up a code editor and write that out without your brain sounding off a thousand alarm bells that you're massively messing up something? I can imagine myself doing this, having not slept in the last 20 hours, after 7 martinis and one especially long island. Maybe he did that too, but he should have the common sense as CTO of the company not to!
  - oji0hub 6y ago
    Who knows what was going on. In an ideal world we have the time needed to do things well. In reality, you've got twenty developers asking for something, users requesting features, various government agencies asking for documents, the lawyers inform you about some case and... oh your wife is calling on the other line because... etc etc etc.
    - jdxcode 6y ago
      I get so busy I rip out query builders to SQL concat strings all the time. /s That’s like going 120mph and telling the cop you were just “distracted”. You’re correct that we often make mistakes when we’re busy, but this isn’t that. This is OWASP #1 and the most plain example of it. This is not a “mistake” that engineers make, this is unethical to allow this to go into production. He even has a stackoverflow answer where he explains this exact problem: https://stackoverflow.com/questions/11554015/syntax-sql-where-and-like/11554036#11554036 https://stackoverflow.com/questions/11554015/syntax-sql-wher... If you think I’m wrong, then what could an engineer be responsible for? Literally nothing ever?

by 0xy 6y ago

jdxcode 6y ago

I've been on teams building web applications for 14 years and not once have I seen someone simply rip out a query builder like this in production code and replace it with a non-parameterized string—let alone someone with 20+ years of experience. Accidents and SQL injections happen, usually because of non-obvious query-building but this is a different level.

0xy 6y ago
It happened to your employer. You might not have been on the team but this stuff is rife. WhatsApp in particular has made some absolutely rookie-tier security blunders [1]. [1] https://threatpost.com/whatsapp-bug-malicious-code-injection-rce/152578/ https://threatpost.com/whatsapp-bug-malicious-code-injection...
Cloudef 6y ago
When swift was still young the only mysql library available for it did the same mistake. The memory safety sure didnt help here... Sure enough i had to roll my own code.
- jdxcode 6y ago
  Well not all injection vulns deserve this level of scrutiny. I'm not familiar with that vuln, but I don't see how it could be the "same mistake". My guess is there simply wasn't support for parameterization or there was a non-obvious concatenation problem when building the query string—but please correct me if I'm wrong. This instance is novel because the parameterization protection was removed in favor of concatenation and that the vuln is so obvious a first-year CS student wouldn't struggle to identify it.

dj_mc_merlin 6y ago

No senior engineer should blatantly concatenate strings to make an SQL query. That is a junior mistake, no doubts about it. If you haven't learned that user input is not to be concatenated into code unless you think long and hard about it and you're really sure there's no better option, you are not in a position to lead. Seriously, if this is a mistake a senior engineer could do at your company, why would you work there? I'm not one for gatekeeping but this is learned within a couple of code reviews in your first years.

0xy 6y ago
By your logic, nobody should work at Facebook, Google, Apple etc etc etc. Unless you're claiming that those SQL injection exploits were introduced AND review approved by junior engineers, a practical impossibility according to policy. Your claim that senior engineers don't make whopping security blunders is absolutely false according to all available evidence. The smartest programmers working on kernels make them constantly.
- ericbarrett 6y ago
  Former Facebooker, on the storage team—if a senior engineer* wrote the supplied code, well, I can't say they'd be fired, but it 100% would not reflect well on them come review time. Also, all their peers would know they made the most boneheaded of mistakes, and they would definitely be getting the hairy eyeball come code review. Raw untrusted string substitution into a SQL query is just unacceptable. It's on the level of storing unhashed passwords, or 1000-line god functions: the kind of mistake you'd expect from a self-taught developer writing PHP in 2002. Literally every popular SQL client and ORM provides quoted substitution; please use it. And, dear reader, if this was news to you, and you write SQL, please go get educated, because it's not the last nor the sneakiest pitfall in this space! * Senior enough to be writing raw SQL code at Facebook
  - 0xy 6y ago
    Facebook has had multiple injection exploits, including SQL and JS RCEs. It's possible no senior were the git blame behind them, but it's absolutely impossible that no seniors reviewed and then maintained that code. WhatsApp had a raw JavaScript RCE literally last year. Also, blaming colleagues for making a mistake is extremely toxic behavior. I've fixed dozens of >9.0 CVSS vulnerabilities at multiple billion-dollar organizations. I'd say senior engineers were involved in their introduction at least half of the time.

lawnchair_larry 6y ago

Yeah, I beat this dead horse every time the armchair security quarterbacks climb on their high horse in these comments. These “dumb” mistakes happen to everyone, at every company. Even security experts. Even one of tptacek’s companies had a well known incident, and people said all the same things about their “dumb” mistakes. Companies are very porous, people take shortcuts, temporary workarounds get forgotten and left in place, etc. You can tell who actually has real experience by who is saying this and getting downvoted for it, ironically. All of the downvoted comments in this thread are correct.

dj_mc_merlin 6y ago
I take the same standpoint as you most of the time. But you can't fucking interpolate user input into SQL queries without physically cringing and call yourself an experienced engineer. Can you really pull up a code editor and write that out without your brain sounding off a thousand alarm bells that you're massively messing up something? I can imagine myself doing this, having not slept in the last 20 hours, after 7 martinis and one especially long island. Maybe he did that too, but he should have the common sense as CTO of the company not to!
- oji0hub 6y ago
  Who knows what was going on. In an ideal world we have the time needed to do things well. In reality, you've got twenty developers asking for something, users requesting features, various government agencies asking for documents, the lawyers inform you about some case and... oh your wife is calling on the other line because... etc etc etc.
  - jdxcode 6y ago
    I get so busy I rip out query builders to SQL concat strings all the time. /s That’s like going 120mph and telling the cop you were just “distracted”. You’re correct that we often make mistakes when we’re busy, but this isn’t that. This is OWASP #1 and the most plain example of it. This is not a “mistake” that engineers make, this is unethical to allow this to go into production. He even has a stackoverflow answer where he explains this exact problem: https://stackoverflow.com/questions/11554015/syntax-sql-where-and-like/11554036#11554036 https://stackoverflow.com/questions/11554015/syntax-sql-wher... If you think I’m wrong, then what could an engineer be responsible for? Literally nothing ever?