Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

SQL Injection Galore

github.com

Post reply on HN
Print view View original

21–30 of 88 posts

Re: SQL Injection Galore

#21

Post by tudborg »

hahainternet wrote: post #16
fooyc wrote: post #15

Earlier quoted context omitted.

This is exactly what NOT to do. The mysql_real_escape_string part is what PHP did with its "magic_quotes" feature. This feature has been removed for good reasons : - All your variables are cluttered with \' everywhere, so you have to unescape them before doing anything other than using the variable in a SQL query. And re-escape them after that. You will forget to do it. - It doesn't protect you if you expect the vari…

> You should be using htmlspecialchars() instead. Without trying to be too much of an arrogant arsehole, surely they should be using a proper language. That is the real core cause of the problem here isn't it?

You can be an idiot in any language, including PHP. Blaming the tool is exactly what gets you in to this mess in the first place.

The fault is with the programmer. No one else.

Re: SQL Injection Galore

#22

Post by tomorgan »

grey-area wrote: post #6

I find it incredible that there are 86,453 results for this, and 58,123 results for execution of a parameter as a command! Github should set up a little warning for people when they log in to their account if their repos match any of a set of obvious security anti-patterns, because this would be fertile ground for exploits. They could expand it later to some kind of code-review bot which does automatic code reviews l…

I think this is a great idea, and an area where someone as big as Github could make a MASSIVE difference. A CodeSmell metric! Allow contributions to add extra smells to langauges, and then it looks after itself.

Re: SQL Injection Galore

#23

Post by hahainternet »

tudborg wrote: post #21
hahainternet wrote: post #16

Earlier quoted context omitted.

> You should be using htmlspecialchars() instead. Without trying to be too much of an arrogant arsehole, surely they should be using a proper language. That is the real core cause of the problem here isn't it?

You can be an idiot in any language, including PHP. Blaming the tool is exactly what gets you in to this mess in the first place. The fault is with the programmer. No one else.

It's true that you can be an idiot in any language, but I have written production code in every 'high level' language of this type and only PHP seems to suffer from this problem. Take a random mail form, even has its own domain: http://jemsmailform.com/

Finding the obvious problems I leave as an exercise.

Re: SQL Injection Galore

#24

Post by harrytuttle »

I have commit hooks on our repository that look for things like this and prevent the user committing it!

We've got 30 odd rules so far that have saved us from all sorts of pain from exception swallowing to adding test ignores as well.

Re: SQL Injection Galore

#25

Post by Xylakant »

seivan wrote: post #8

For Ruby.. this is probably not as bad, and some of them are probably harmless, but still. * Apartment.destroy(params[:id]) https://github.com/search?q=extension%3Arb+path%3A%2Fapp%2Fc... I've found stuff like this on startups who charges people money - scary. Some of them require authentication but not authorization In other words, they require login, but not WHO you're logged in as.

Your example looks much like AR and AR will sanitize the given parameter at least in this case so that's a non-issue here. Same would be true if you'd call a prepared statement in PHP and hand it a GET parameter. The problem with the given PHP samples is that they do neither: The take the unsafe user-provided parameter and use string concatenation to build a query - the textbook example of an SQL injection vulnerability.

Re: SQL Injection Galore

#26

Post by siddboots »

c0n5pir4cy wrote: post #18
fooyc wrote: post #15

Earlier quoted context omitted.

This is exactly what NOT to do. The mysql_real_escape_string part is what PHP did with its "magic_quotes" feature. This feature has been removed for good reasons : - All your variables are cluttered with \' everywhere, so you have to unescape them before doing anything other than using the variable in a SQL query. And re-escape them after that. You will forget to do it. - It doesn't protect you if you expect the vari…

Fair point, it's actually not my code and I've always been generally skeptical about its safety. I have found flaws in it before. Thanks for the input, I'll look at changing it.

The strength of using a parameterised query (i.e. $dbh->prepare) is that you are letting your database's type system do the work for you. You tell it where your variable should land in your query, and before it even looks at the variable the database works out what type to expect. Either the data you give it fits into the hole you tried to put it in, or the query fails gracefully and your existing data is safe.

Re: SQL Injection Galore

#27

Post by user24 »

hahainternet wrote: post #23
tudborg wrote: post #21

Earlier quoted context omitted.

You can be an idiot in any language, including PHP. Blaming the tool is exactly what gets you in to this mess in the first place. The fault is with the programmer. No one else.

It's true that you can be an idiot in any language, but I have written production code in every 'high level' language of this type and only PHP seems to suffer from this problem. Take a random mail form, even has its own domain: http://jemsmailform.com/ Finding the obvious problems I leave as an exercise.

I kindly direct you to search for "formmail.pl".

Re: SQL Injection Galore

#28

Post by jasonlotito »

hahainternet wrote: post #20
fooyc wrote: post #19

Earlier quoted context omitted.

Nothing specific to the language here. Mentioned functions do exactly what their names imply. Just like ruby or python, PHP has great templating languages, support for prepared statements, etc.

This is often said, but these problems are practically endemic in the PHP 'community'. I can't help but think that the lack of proper documentation or basic explanations is responsible.

I'm glad you put community in quotes. The problem is not endemic in the PHP community.

Re: SQL Injection Galore

#29

Post by sdevlin »

Xylakant wrote: post #25
seivan wrote: post #8

For Ruby.. this is probably not as bad, and some of them are probably harmless, but still. * Apartment.destroy(params[:id]) https://github.com/search?q=extension%3Arb+path%3A%2Fapp%2Fc... I've found stuff like this on startups who charges people money - scary. Some of them require authentication but not authorization In other words, they require login, but not WHO you're logged in as.

Your example looks much like AR and AR will sanitize the given parameter at least in this case so that's a non-issue here. Same would be true if you'd call a prepared statement in PHP and hand it a GET parameter. The problem with the given PHP samples is that they do neither: The take the unsafe user-provided parameter and use string concatenation to build a query - the textbook example of an SQL injection vulnerabil…

He's citing this as a potential cross-authorization vulnerability, not SQL injection.

The form he's demonstrating is a common misstep in Rails. Instead of writing something like:

  current_user.apartments.destroy(params[:id])
The programmer wrote:

  Apartment.destroy(params[:id])
Meaning that the apartment lookup is not being done within the context of the current user, it's global. This means an attacker can delete other users' apartments with crafted URLs.

When I'm testing Rails applications, I always grep the source for something like:

  /[A-Z][A-Za-z0-9_]\.(find|destroy|...)/
To find vulnerabilities of exactly this kind.

Re: SQL Injection Galore

#30

Post by readme »

user24 wrote: post #10
yuvadam wrote: post #7

Bear with me on this one - All the code is already public on Github, so everyone can see these gaping security holes, right? What if some honest, global actor could mass-commit a fix for all these repos in one fell swoop? For example, replace all references to: $_GET with: some_safe_sanitizer($_GET) All affected repos win a free fix, and the world becomes a better place due to having less security bugs. Essentially,…

To do it properly you'd want to use prepared statements[1] which requires a non-trivial, though not particularly complicated, syntax change. So your global actor would have to parse the PHP in a rather more intelligent way that just a string replace. You could just use mysql_real_escape_string [2] but that's less secure than prepared statements, and may break some things (eg if the code is relying on certain things n…

I know the PHP/PDO way to do it, is prepared statements.

I'm not able to see why the parent's suggestion to just escape the strings is not a valid solution from a security perspective.

Post reply on HN