SQL Injection Galore
github.com
github.com
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 looking for common antipatterns, off-by-one errors, and anything else that is easy to detect automatically. There are quite a few tools like this for analysis of desktop apps, but code for web apps are not checked as much in a systematic way.
Are there any external services which do this already for public github repos?
That's where i'm storing my dirty laundry at least...
$ mkdir project_name.git; cd project_name.git; git init --bare
Then locally, from the repo directory:
$ git remote add origin username@server.com:~/path/to/project_name.git
That's what I do (on Webfaction). Of course, then you don't get all the fancy issue tracking and all that from github, but it works if all you need is a few kb of storage for a remote. That also makes it easy to deploy PHP projects with a single post-receive hook, since it's all in the same box.
$ git init --bare project_name.git
If someone is encountering the same problem, the code is there for them to use. If they happen to use it in production, that's hardly my fault.
Thinking about it again probably it'd be best as a per-repro flag that made a little button appear somewhere saying 'security', 'code police' or something similar, leading to more info. Obviously it'd take a bit of work and generate some false-positives, but I could see a system like this being really handy if they accepted open contributions for filters, even if it was opt-in it would probably still help a lot of people.
Re dirty hacks, I guess I wouldn't publish things like that to github, I'd keep them in a local or private repo, as they're not for sharing. If they are for sharing in spite of vulnerabilities, turning off a warning shouldn't be too much hardship for you, and it could actually help others looking at the code quickly tell it wasn't to be used in production as it would mark it as unsafe.
* 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.
The form he's demonstrating is a common misstep in Rails. Instead of writing something like:
current_user.apartments.destroy(params[:id])
Apartment.destroy(params[:id])
When I'm testing Rails applications, I always grep the source for something like:
/[A-Z][A-Za-z0-9_]\.(find|destroy|...)/
I'm a little sad I hadn't thought to do this already...
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
some_safe_sanitizer($_GET)
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 not being escaped, etc). Also that extension is deprecated in favour of prepared statements.
Also, some of the dodgy code is using $_GET vars for things other than values, like field and table names (!!!!) which would need a more significant refactoring in order to fix.
However it would be a great little problem to work on, and you could probably write a bot to fix 80% of the code pretty easily.
The other issue is how many people will understand and accept the pull requests...
[1] http://php.net/manual/en/pdo.prepared-statements.php
[2] http://php.net/manual/en/function.mysql-real-escape-string.p...
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.
Include a check to make sure multiple bugs in a single repo are handled by just one pull request too.
$table_name = $_GET['from'];
if (!in_array($table_name, $VALID_TABLES)) {
die('Invalid table name');
}
$set_to = (int)$_GET['set_to'];
mysql_query("INSERT INTO $table_name (value) " .
"VALUES ($set_to);
The case with $set_to is still pretty bad form, but it has been sanitized above by casting to an int.
Not saying this is great code, just pointing out the fact that there's no possible way to do a mass-commit fix.
* All read-only queries use a read-only (SELECT only) database account. Injecting; DROP TABLES or INSERT, UPDATE, etc (DML commands) just error out.
* User accounts and logins are stored in a different database, so only the code responsible for login and registration can access those tables/database.
* All queries should use prepared statements which effectively treats all injected text as just text rather than database commands.
https://github.com/search?q=extension%3Aphp+exec+%24_GET&typ...
System (as you mentioned) or EXEC injections, however, may get out of hand.
Edit - made an issue.
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.
- Use raw $_GET variables in their code - Use $_GET in MySQL queries - Use mysql_real_escape_string instead of prepared statements or otherwise properly sanitizing input - Use PHP in general
We already know we should all be using prepared statements or combining data sanitizing methods with an ORM and to not trust raw user input and that its cool to hate on PHP. So I ask again, what are we learning here? That there's a ton of programmers who are doing sloppy incompetent work? Not news.
Furthermore, I think context is important. I'm not ashamed at all to say out loud that I've done these sorts of things knowingly and purposefully despite knowing it's not safe or correct. Why? Maybe I was creating simple examples to teach others the basics of getting user input and working with it in a database. Of course you'd warn people not to do things like that in a production environment but you show it that way anyway to get across some basic ideas without throwing too much at people. Or what if I were writing a simple series or scripts or small app that would only be used by myself or a select group of people? What if that application needed to be done in a hurry and was only for internal use and not accessible to the outside world? Lots of things can always go wrong but at a certain point you have to be able to trust someone and its not always better to be right than happy. I'm sure if anyone wants to be pedantic they can poke holes in my examples but the point is that these sorts of sins aren't always so awful depending on the context. I don't see any purpose in this other than to either look for open source applications to exploit in the wild or to just pick on a really easy target to make us all feel superior.
You could do a similar search for common anti-patterns in almost every language you can think of, so this is not specific to PHP, just perhaps a bit more common due to the low-bar for picking up PHP.
A useful aim of this sort of post to me is to underline that people should not do this in a script exposed to the internet, and if people read comments on searches like this, they'll find out why. I don't think it's appropriate to use variables in query strings even for learners as it's teaching bad habits - I have personally seen scripts go into production based on bad examples at a former place of work (in perl, not php), allowing spammers to use a contact form. You can guarantee that for each of these common vulnerabilities there are scripts in the wild scanning websites and trying to exploit them.
A surprising number of people still do make mistakes like this, so it does more good than harm to point that out I think, and certainly doesn't make me feel superior, it makes me wonder which vulnerabilities I've missed in my own code.
> Maybe I was creating simple examples to teach others the basics of getting user input and working with it in a database.
I'm one of the countless people who learned PHP off of examples that do just that, for clarity, and it was years before I learned that it was not meant to be a real-world example. Sites that offer such examples (I'm looking at you, w3schools) are probably responsible for 90% of all the bad code that shows up on lists like this, because the exchange amounts to:
Student: "How do I do XYZ?"
Teacher: "It's easy! Just do 123."
Student: "Thanks!" <leaves, closes door>
Teacher (to empty room): "Haha, just kidding. You should really forget that entirely and do 456 instead."
I don't think it's valuable to learn about mysql_query("delete from `tbl` where id = {$_GET['id']}") at all in the first place, if you just have to unlearn it and start doing it the right way later. It doesn't actually simplify anything.
We are not the problem, but we should consider trying harder to be the solution. Maybe new PHP programmers do need to be educated, and there's a lot of outdated source material out there giving them bad advice and spreading bad practices. Maybe people putting their stuff on Github don't entirely realize how networked and public it is... and they need a polite reminder that other people might end up paying for their haste or negligence. We can do more than just point and laugh. PHP might be bad but a lot of us know it doesn't have to be as bad as most of it still is.
The code looks something like this:
$_GET = array_map ( 'strip_tags', $_GET );
$_GET = array_map ( 'mysql_real_escape_string', $_GET );
Your devs will learn to trust $_GET "because it's cleaned by our include".
They'll never sanitise input themselves "because it's cleaned by our include".
Code that they write for other projects will trust $_GET "because it's cleaned by our include" - except it's not because this is a 3rd party script.
Code you import from other projects will be double-escaping.
Also mysql_real_escape_string is deprecated.
Stop treating SQL queries as strings. They're code. You wouldn't write code by concatenating strings with user input would you?
Use prepared statements.
edit: missed the part when you said that you clean $_POST as well. I was wondering "What do you do when you need to submit markup?" Now I know that it's magic. The $_REQUEST array is actually the unsanitised array, whereas $_POST is the sanitised one. Of course! Isn't it obvious!
Sigh.
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 variable to be numeric, and use it in a query without enclosing it in quotes:
// do not do this
mysql_query("SELECT do_not_do_this WHERE x = $var");
// $var could be `0 OR 1 = 1`, even after escaping
$var = get_data_from_db_or_other_source();
// do not do this
mysql_query("SELECT do_not_do_this WHERE x = '$var'");
---
Also, you shouldn't be using strip_tags at all:
- It doesn't strips quote characters, so you would be vulnerable to html parameter injection, if you used a variable "sanitized" like this in a html parameter.
- It alters the data is some non-reversible way. What if you genuinely want to display (not render or "execute") some html code in a blog post on your web site ?
You should be using htmlspecialchars() instead.
---
What to do:
- escape, do not "filter" or "sanitize"
- escape data just in time, not ahead of time (e.g. escape your variable just before using it in a mysql query, or just before outputing it in a html document)
- prefer prepared statements
- use the right escape function depending for the context in which the variable is used: mysql_real_escape_string for mysql query, htmlspecialchars for html, escapeshellarg for command line arguments, etc.
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?
Just like ruby or python, PHP has great templating languages, support for prepared statements, etc.
The problem is mostly the widespread support and low barrier to entry. PHP is supported on every crappy $2/m hosting plan.
It's also the language of a lot of the net's more popular CMSs. (WordPress?).
People have a site and want to make it do stuff. The site is already either using PHP or that's all the host supports.
PHP gets most of the users who aren't trying to get into programming (and may have no interest in it whatsoever) - they just wanna get something done. Some of those people develop an interest and stick around and learn the language, but aren't really forced to learn any software development methodologies.
PHP acts as a filter, collecting all of the non-programmers and people who have stumbled into programming accidentally or unintentionally. People who write Ruby or Python are generally people who set out to be a programmer.
The language itself has its warts, but I see the above, not the warts, as the primary driver of all the crappy code out there.
The fault is with the programmer. No one else.
Finding the obvious problems I leave as an exercise.
That said, all the mysql_* functions have been officially deprecated and we steer newbies to using pdo and named parameters.
Thanks for the input, I'll look at changing it.
I mean, sure, it works, but it's ugly and inconvenient. PDO does it much, much better.
I wonder whether a similar search for the mysqli OO interface yields the same amount of unsanitised queries. (Assumption: People using the OO interface looked through the documentation more closely and maybe understand the ways in how mysqli is better than mysql. But that's just an unfounded assumption.)