Comment by noduerme
2 years ago
To their credit, a lot of people went back a decade later and fixed those. Although it doesn't stop people from repeating the mistakes.
I just got beaten up in HN for asking how the hell sql injection is still a problem. People get defensive, apparently.
Sounds about right.
Not even a few years ago I worked with people who insisted it was ok to write injection unsafe code if you knew for sure that you owned the injected values. Didn't matter that maybe one day that function would change to accept user-supplied data, that's not their problem! It was a Rails app and they were literally arguing wanting to do:
over:
in those certain situations. So, you know, it takes all kinds, I guess.
This is a case of militancy.
If we're talking about a typed integer there is no chance of that turning into an sql injection attack.
If we're talking about a string, I'd probably insist on parameterizing it even if we completely own it just on the off chance that the future changes.
To draw an analogy, gun safety is important and everyone knows it. But I don't practice gun safety while watching television on my couch because the gun is locked away. I practice gun safety when I'm actually handling the thing that is dangerous.
And yes, I realize it being locked away is technically gun safety, it's an imperfect analogy, please roll with it.
Your analogy is not flawed, but your conclusion is.
It is a perfect analogy because you are practicing gun safety by locking the gun away. If someone that you are not expecting wanders into your home while you are sitting on the couch, such as a child, they will not suddenly have access to the firearm. This is exactly why you don't assume that you will never receive unsafe input in this situation.
2 replies →
to be pedantic, just being "typed" is not enough these days with dynamically-typed server code.
1 reply →
> If we're talking about a typed integer there is no chance of that turning into an sql injection attack.
Unless the database table switches to non-integer ids at some point.
Ruby is a dynamic language.
I think I agree with your coworkers. If the data is predefined constants, then you don't need to worry about injection. All functions have preconditions which must be met for them to work. As long as that's specified, that's acceptable.
Imagine the internals of a database. An outer layer verifies some data is safe, and then all other functions assume it's safe.
The example you're sharing is a bit of straw man. It's just as easy to use the parameter, so of course that's the right thing. But interpolating a table name into the string from a constant isn't wrong.
I'm not sure if this is a troll or not and I don't really want to debate this kind of thing on HN, but you've baited me. It is not a straw man. As I said, the source of the input could change in the future and it could be missed. The safe version is no more complicated than the unsafe version, so why wouldn't you just do the safe one? There is zero advantage to the unsafe way and it's straight up reckless to defend it.
I'm one of those people who moved from Ruby to Elixir. Ecto, Elixir's defacto database wrapper, will throw and exception if you try and write interpolated code like this, so luckily I don't have to have these insane arguments anymore (well, I work alone now, so there are several reasons I don't have to have them).
EDIT: My bad, I glazed past the last part of your statement.
Ya, I think this is probably where some of the defensiveness comes from: using a library vs rolling your own. If you're rolling your own, of course you're going to need to interpolate table names and whatnot, but it shouldn't even be possible to interpolate values. My example and argument is based of Rails, though, where you never specify a table name or anything like that. So in the specific case of my coworkers, they were wrong.
7 replies →
I think you were broadly misunderstood. If the defined constants come from or are checked against the ones stored in the database, fair play. If they're floating around in some static consts in a code file, also ok as long as that's extremely well documented and someone knows what's what. If some boss pays to cut corners for it to be written with magical constants like "WHERE life.meaning!=42" and then fires the person who they hired to write that script, they deserve whatever they get.
Just like the meaning of life, it's best not to come to premature conclusions. Could all work out, or it could be a funny joke for aliens in the end.
> I just got beaten up in HN for asking how the hell sql injection is still a problem.
It's possible for developers to think they're actually doing the right thing, but it turns out they're not.
https://www.npmjs.com/package/mysql#escaping-query-values
> This looks similar to prepared statements in MySQL, however it really just uses the same connection.escape() method internally.
And depending on how the MySQL server is configured, connection.escape() can be bypassed.
Yeah, the Nodejs ecosystem is sketchy in this regard. I've never put a Node-mysql site into production. Basically everything I write that runs DB queries is in PHP with PDO. But I got interested in Node for side projects and spotted this escaping flaw in node-mysql. That npm package also has two escaping modes, one which it calls "emulated" and which is probably less trustworthy. It doesn't seem like it was ever ready for primetime. I don't know if node-mysql2 addresses that... I ended up writing a promise wrapper for the original one that also turns everything into prepared statements. You still need to make sure NO_BACKSLASH_ESCAPES is off, although I have no idea why you'd ever turn it on.
So yeah, I'm coming from a PHP mindset where you can generally trust your engine to bind and escape values. My experience with Nodejs in this particular area caused me to write a lot of excess code (mostly to satisfy my own curiosity) and still convinced me not to trust it for the purpose.
In that light, I can understand how someone who jumped into the Nodejs ecosystem would think they were dealing with reliably safe escaping, and didn't realize what they were actually getting if they didn't read the fine print.
Hi! Sorry to report this, but I've pushed a SQL injection vuln to prod when I was still very green.
In my defense, we trusted the input. But that's post-rationalisation, because I simply didn't know what I was doing at the time.
It gets worse. If I'd done it properly, my senior would have beaten me up in code review for "complexity". That was a man who would never use a screwdriver when a hammer was already in his hand.
I once argued with a senior dev (later engineering manager, I guess he is a director of development now somewhere), that storing password hashes in unsalted SHA1 was bad.
His defense? "This system is internal only and never connected to the internet"
Senior titled devs don't necessarily know their shit.
A little off topic, but I love how you mention his career progression before sharing the example of his ignorance, because this seems to be a pretty common theme in tech companies (I've witnessed it more times than I can remember or count). The people I knew in my career who were most full of shit are pretty much all now Directors and VPs, enjoying a life of success, and the ones who were the most actually knowledgable are still grinding away as IC's, worried about layoffs. This industry is really bad about rewarding competence.
4 replies →
Non security expert here. Walk me through the attack scenario here.
The database has access control right? So only a few people in the org can read the data. And you are imagining a case where they:
a) find an inverse image of a password hash and use that login as another person to do something bad.
b) reverse the password from the hash to use in another context.
If a is an issue, why does this individual have sensitive data access in the first place? b is still unlikely. Any inverse image is unlikely to be the password if there is salting.
It sounds like an improvement could be made, but maybe not the highest priority. Can you inform me?
To be fair, I’ve pushed vulnerabilities to prod when considered a senior and with 10+ years of experience. Nobody is immune to their own stupidity and hubris.
People who don't understand things often get cranky when they're told it's easy. Seems fair though, it does seem rude to tell someone missing a leg it's easy to run... But it also seems rude to get upset at someone who's good at something they've studied so perhaps everyone is bad at understanding the person they're talking to, and people should assume more good faith.
That’s why I prefer to use “straightforward” rather than “easy.”
People seem to take that much better.
I also like "simple". Lots and lots of very hard things are not at all complicated.
Hitting a homerun is straightforward, but it’s not easy.
2 replies →