
SQL injection search - mike_esspe
https://github.com/search?p=3&q=extension%3Aphp+mysql_query+%24_GET&ref=searchresults&type=Code
======
a1a
While we are at it.. XSS search:
[https://github.com/search?q=extension%3Aphp+%3C%3F%3D%24_GET...](https://github.com/search?q=extension%3Aphp+%3C%3F%3D%24_GET&type=Code&ref=searchresults)

~~~
timothya
Is it an XSS if you can only make it output code to your own browser? I can
already execute whatever JavaScript I want in the console, so what's the
advantage of having the server deliver that code to (only) me?

I can definitely see the issue if the server saves the unfiltered input and
tries to print that out for other uses, but it seems to me that outputting a
raw $_GET variable will only go to the requester and therefore could only run
unfiltered code on that requesters machine.

EDIT: Answering my own question:

This is a security hole because the unsafe JavaScript is stored in the URL for
the page (it is a GET parameter). This bad URL could then be sent as a link in
a spam email or similar. Victims clicking on the link would then see a page
that comes from the legitimate source, but is running unsafe code compromising
that user's session. This sort of attack relies on the attacker distributing
the link with the bad code as a URL parameter, and is not a vulnerability that
a user could encounter when just visiting that site as I had first assumed.

~~~
arthurschreiber
That's _exactly_ what XSS is about. One possible way to exploit things like
this is if I send you a link to a website, that embeds the target page through
an iframe with javascript output injected. I could then have the JS steal your
cookies/session or worse.

------
thiderman
Heh, cute. This means Github could probably do some automated means of
informing these people that their code is insecure and would be a danger to
themselves and their users. I'm not sure if they should, but it's interesting
that they could.

~~~
Navarr
I'm pretty sure anyone can write a github bot. I remember there used to be
several (some of which would submit pull requests!)

~~~
jaytaylor
GitHub has not been friendly to bots in the past[1].

[1] <https://news.ycombinator.com/item?id=4982240> "GitHub Says ‘No Thanks’ to
Bots — Even if They’re Nice"

------
sage_joch
This is a potentially great idea. You could make your build process include
submitting your code to a search engine like this (perhaps in some obfuscated
manner) and making illegal patterns fail if not manually "approved". Just
because the halting problem exists doesn't mean there's not a low hanging
fruit in approaching it.

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

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

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

------
postfuturist
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 ugly, but it's bulletproof regarding injection.

~~~
hcarvalhoalves
"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?

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

~~~
tjohns
In PHP's defense, strstr() originally comes from C:
[https://developer.apple.com/library/mac/documentation/Darwin...](https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man3/strstr.3.html)

------
tofflos
I don't know much about PHP but I happened to rewrite some old forms a couple
of years ago. The original author had relied on a technique called "magic
quotes" (<http://php.net/manual/en/security.magicquotes.php>) which
automatically sanitized user input. When we upgraded our version of PHP "magic
quotes" had been deprecated and dropped.

It would be interesting to know if some of these developers are relying on
"magic quotes" or something similar... and also to know how large share of the
total number of projects these projects represent.

~~~
magoon
Yeah no, magic quotes didn't ever sanitize.

------
tptacek
There's a joke to be made here about "broken crypto search".

~~~
pjscott
I'll make that joke, but it won't be very funny.

[https://github.com/search?p=2&q=MD5+password+extension%3...](https://github.com/search?p=2&q=MD5+password+extension%3Aphp&ref=searchresults&type=Code)

[https://github.com/search?q=CURLOPT_SSL_VERIFYHOST+NOT+depre...](https://github.com/search?q=CURLOPT_SSL_VERIFYHOST+NOT+deprecated+NOT+2&type=Code&ref=searchresults)

There's more low-hanging fruit, if you're willing to use more specialized
searches. For example, guess what mode of operation the PyCrypto library uses
by default for all its block ciphers if you don't explicitly pick a sane one:

[https://github.com/search?p=1&q=extension%3Apy+crypto+ci...](https://github.com/search?p=1&q=extension%3Apy+crypto+cipher+aes+new+NOT+PyCrypto_AES+NOT+Crypto%2B%2B&ref=searchresults&type=Code)

~~~
tptacek
The joke was more like:

[https://github.com/search?p=2&q=AES&ref=searchresult...](https://github.com/search?p=2&q=AES&ref=searchresults&type=Code)

------
orangethirty
Looking around I found a simple CMS sold to small online stores. Through their
links you can find a listing of their customers (people who use their CMS).
Problem is the CMS is open to SQL injection everywhere. If a script kiddie
found this info they could take down a lot of online stores. Not good.

~~~
jarofgreen
Did you tell them?

~~~
muloka
Or better yet write a patch and submit a pull request informing them.

------
PanMan
Nice example, but not all are insecure. For example, the second one here is:

    
    
        $result = mysql_query('DELETE FROM saves WHERE id = '.(int)$_GET['delete']);

~~~
astrodust
That's an example of hazardously bad programming practices. You're one mistake
away from complete disaster. You should be sure that it takes more than one
mistake to expose you to that sort of risk.

Casting to int is not a general purpose escaping system, and further, if you
miss even one of these your entire application can be trashed.

Using mysql_query at all is a sign there's something severely wrong with your
application.

~~~
ratsbane
There is an opposing viewpoint, i.e. if you actually need an int, casting to
int is one of the most reasonable ways of getting it.

~~~
astrodust
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.

~~~
ratsbane
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.

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

~~~
ratsbane
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...](https://github.com/Paton/Saaave/blob/master/_views/browse.php)

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.

------
kbenson
I would like to say i'm surprised, but I'm not. PHP makes this easier by not
even supporting parameter binding in the older, original mysql binding, so
it's more prevalent.

That said, I'm sure a slight tweak to the search would find a lot in other
languages as well.

------
ya3r
Sort the results by "last indexed" and see that people are doing it right now!

[https://github.com/search?q=extension%3Aphp+mysql_query+%24_...](https://github.com/search?q=extension%3Aphp+mysql_query+%24_GET&type=Code&s=indexed)

------
diminoten
There is a _huge_ need in the space for a well marketed quality assurance
contractor who can find problems like this and fix them.

"We found these issues, and we can fix them all. Pay us for finding them or
pay us some more for fixing them, too." sort of thing.

Why don't you see QA shops popping up like this?

~~~
ams6110
You'd need some pretty iron-clad liability waivers... if you miss something,
you're opening yourself up for a "malpractice" sort of scenario.

~~~
metarev
Liability can be addressed with non-reliance disclosures.

Bigger issue is a market problem. People who need the help the most do not
know they need it.

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

~~~
PommeDeTerre
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.

------
hising
I guess people would pay for a service that could identify 90% of all security
issues with an online service by going through source code and available
routes. Anything that is available today?

~~~
_pius
<https://www.tinfoilsecurity.com/>

~~~
dsl
Tinfoil scans remotely for known vulnerabilities (i.e. WordPress version X has
exploit Y). What you really want is source code analysis. WhitehatSec.com and
Veracode.com are the two huge players in this space.

------
josephscott
In addition to $_GET searching for $_POST and $_REQUEST are bad too. Could
even through in $_COOKIE and $_SERVER for that matter.

------
blazespin
GitHub (and friends) sound like a great repository to train and prove the
value of an automated code review product.

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

~~~
soult
So a gaping security hole that compromises all data is less important than
using a deprecated interface?

~~~
astrodust
For an application with no exposure, arguably it is.

When you upgrade to PHP 5.6 and your application grinds to a halt because
mysql_query isn't available, you'll be wishing you'd fixed it sooner.

~~~
zapt02
If you're on RHEL/CentOS, 2023 is gonna be a bad year.

(RHEL/CentOS is currently on PHP 5.3 and will stay there for a long time.)

------
cbsmith
More comprehensive search for bugs:
[https://github.com/search?q=extension%3Aphp&type=Code...](https://github.com/search?q=extension%3Aphp&type=Code&ref=searchresults)

------
boyter
And for examples not limited to github
<http://searchcode.com/?q=mysql_query%20%24_GET%20lang%3APHP>

------
ams6110
Some of these look like deliberate examples of vulnerable code (e.g. the one
named "Injection.SQL.php")

Alarmingly (and sadly) most do not.

~~~
jt2190
No need to be alarmed (or sad.)

Without some real-world context for each project, we just can't say whether
the code we're looking at represents a real security problem. I'm certain that
github hosts just as many one-off, let-me-scratch-some-code-together-
that-I'll-never-use-again type projects as it does hard core, production
quality ones.

That said it's a neat technique for quickly auditing code. Someone should now
write an automated tool for submitting security patches to all of these
projects.

------
wordofchristian
I'd love to see the number of search results graphed over time.

------
artursapek
75 thousand results. And these are just the public repos!

------
marizcombinator
mysql_query is deprecated... use MySQLi or PDO

~~~
martinml
MySQLi or PDO will not automatically solve almost any of the security issues
found in this search.

~~~
astrodust
Using either of them correctly will.

It is very easy to verify that a mysqli or PDO call is correct by looking at
it. The same cannot be said for mysql_query.

------
soheil
This f'ing insane, and absolutely brilliant.

------
tshadwell
;.;

