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

SQL injection search

github.com

Post reply on HN
Print view View original

101–110 of 114 posts

Re: SQL injection search

#101

Post by Joeri »

hcarvalhoalves wrote: post #79
postfuturist wrote: post #62

This isn't a search for SQL injection, its a search for a couple things that you often find in older PHP code that is generally hacked together and likely to have SQL injection vulnerabilities for historical and cultural reasons. However it's perfectly easy to avoid SQL injection even using these things. $id = mysql_real_escape_string($_GET['id']); $res = mysql_query("SELECT foo FROM bar WHERE id='$id'"); That may be…

"mysql_real_escape_string" is the silliest function name ever. I assume there is a "mysql_escape_string" function that doesn't do what you expect it to do?

PHP is a simplistic wrapper around the C API's, so you get all of the legacy of C without any of its performance and memory size benefits.

PHP has some of the insanest defaults due to its C heritage. String functions deal with bytes not characters, so cannot be used safely with utf8 without setting the mbstring.func_overload setting to replace them with unicode-aware versions (except for str_pad, which always deals in bytes). Sort() defaults to binary sorting, and cannot be tricked in any way to sort utf8 in dictionary order if you're running your server on windows (and even on linux it requires an extra parameter on every call). Natsort(), which is supposed to sort like a human would, cannot be made to sort in dictionary order at all. The proper way to sort is by using the Collator class, which is not referenced from the sort() documentation, didn't exist before PHP 5.3, and is in the optional intl extension which is usually disabled by default.

Still better than mysql though, which has a very unique interpretation of unicode collation.

Re: SQL injection search

#102

Post by gingerlime »

btipling wrote: post #78
gingerlime wrote: post #35

Earlier quoted context omitted.

this one seems to produce quite a lot of false-positives though, doesn't it?

Those all look like XSS vulnerabilities to me.

Of course this is a pretty good starting point to find XSS, but I still see quite a few false positives:

the first result I see currently: https://github.com/matsprehn/122B/blob/1d54d2a72f25a23d63ff7...

also spotted this, which looks pretty harmless: https://github.com/cameroni2003/picgrid/blob/0b3becda1f250ef...

a lot others look similar, plus it depends on context...

Re: SQL injection search

#103

Post by oneeyedpigeon »

ratsbane wrote: post #92
astrodust wrote: post #91

Earlier quoted context omitted.

In this case, you don't "need an int", you need a value that's safe to put in a database query. If you're inserting a value in a database, you always, always, always use the proper escaping mechanism. No exceptions. That's why using a library with a reliable, well-defined, easy to use escaping system is absolutely imperative.

In any case the real problem with this line of code is not the int cast, it's that it's using a GET request to delete a record.

Not necessarily - there can still be data in the _GET array, even if the request method is POST.

Re: SQL injection search

#104

Post by PommeDeTerre »

easy_rider wrote: post #88

I'm just amazed and disturbed that people who write this kind of code are aware of version control.

The person who checked in code isn't necessarily the person who wrote it in the first place.

When dealing with legacy code, especially code going back into the 1980s for example, it's not at all unusual for the code to have been stored in several different version control systems over that time. I know of projects that started with SCCS, moved to RCS, then to CVS, then to Perforce, then to Subversion, and most recently to Git.

If those transitions are done quickly, then somebody will often just take all of the code from a checkout of the old VCS, and check it into the new system.

The same can happen when initially using a VCS, after having not used one before, especially when taking a project over from a developer or even a team that was less-talented. Even if the new developer(s) are going to review the code, and fix bugs and other flaws, a version with the initial state of the code is often a useful thing to have.

Re: SQL injection search

#105

Post by a1a »

noinput wrote: post #85
hcarvalhoalves wrote: post #79

Earlier quoted context omitted.

"mysql_real_escape_string" is the silliest function name ever. I assume there is a "mysql_escape_string" function that doesn't do what you expect it to do?

Don't forget strstr(). So nice, they named it twice.

PHP's naming of functions is really horrible.

For example the random usage of underscores: strtoupper(...) substr_compare(...) str_split(...) str_word_count(...)

Re: SQL injection search

#106

Post by ratsbane »

oneeyedpigeon wrote: post #103
ratsbane wrote: post #92

Earlier quoted context omitted.

In any case the real problem with this line of code is not the int cast, it's that it's using a GET request to delete a record.

Not necessarily - there can still be data in the _GET array, even if the request method is POST.

Yes, fair point, but I do think it's a red flag. On looking a bit further I see that yes, in fact, he is using a GET link to delete records: https://github.com/Paton/Saaave/blob/master/_views/browse.ph...

This is an actual, serious problem which I would note in a code review, as opposed to the int thing which cannot, as far as I can can tell, ever lead to an exploit or malfunction which would have been avoided by using a named escape function in this code. If anyone can think of a specific example to prove this wrong, please say so.

Re: SQL injection search

#107

Post by peterkelly »

sage_joch wrote: post #69
jesstaa wrote: post #65

Earlier quoted context omitted.

Yep, It's called 'static analysis'. http://en.wikipedia.org/wiki/Static_program_analysis

Heh. I sure romanticized a semi-mundane thing that already exists. ;-)

Don't worry. That's basically what every tech company does these days anyway.

Re: SQL injection search

#109

Post by kbenson »

ownagefool wrote: post #77
kbenson wrote: post #17

Earlier quoted context omitted.

The search obviously doesn't find all cases, but is a good start. While there's nothing technically wrong with the example given, I might argue that since that won't work in all cases, it might be better to enforce a more rigorous policy of SQL query cleansing, or using bound params. Although this example is so simple I might not. Then again, the fact that $_GET is even available at the location the query is taking p…

Pretty much every language will make getting direct user input then passing it to a database easy. What generally makes this less easy (or at least less intuitive) is a framework. Don't compare the likes of Rails or Django to PHP. Compare Laravel4 or Symfony2. That doesn't mean PHP doesn't deserve some stick, it does, but most of it's current reported problems spawn from backwards compatibility. Nobody should be usin…

What I meant is that $_GET, $_POST and $_REQUEST aren't available in functions unless you declare them global. The fact that $_GET is being used in the query means either that the query is being performed in the main of that script (probably lots of small php files meant to be called by the browser), or that they are in a function/method and then pull in the global $_GET array.

IMHO, neither are good design choices, as one couples the program to HTTP too tightly (and not in a sane way), the other leads to crazy spaghetti PHP as the control flow can be affected by global variables set (or received from the user) far from where they are used.

Re: SQL injection search

#110

Post by LawnGnome »

kodablah wrote: post #87
Killswitch wrote: post #8

Using unsanitized $_GET is the least of their problems considering mysql_* is deprecated.

I believe the ext/mysql is deprecated, not the function names. Mysqlnd is a drop in replacement and unencumbered by the copyleft license issues that plague the original extension.

The mysql_* function API is part of ext/mysql and is deprecated — mysqlnd is a lower level library that basically replaces libmysqlclient in the stack (and is used from PDO and mysqli as well).

I'd be interested in a cite on the licensing issues, incidentally; I'm not aware of anything. (There _is_ an issue with the JSON extension at present, though, so it's not impossible.)

Post reply on HN