NEVER trust user input
I've had the joys recently of being part of a code audit for a potential client for a change or re-build of a system. The code in itself was complete textbook...of how not to code a system. It looked like it had been built long long ago when OO principles didn't exist, and when no-one knew about security unless they were in that field. Certainly developers knew nothing about security.
It started like any audit I've been involved in, get it set up and then log in. Usually that means asking for a password after checking the database to be sure they aren't stored in plain text. Nope, these were plain text. Ok, so we can get in, and chalk 1 up in the bad developer column. After a small poke around to see what it was doing, it became apparent that every page was its own contained file. There were some includes for navigation, but most of it all stylesheets, JavaScript includes and other common resources were written in each and every file. I wouldn't want to be the one to have to update the version of jQuery on that one!
When we discovered this, we found that the queries being ran on each page were not sanitising any of the user input. They were putting it blindly into queries and using old mysql_* functions; long since deprecated. The bad developer count just hit 2, 3 and 4 from one simple check. There were some instances where the developer was checking for one result being returned and failing if there was more, but that only gets a fraction of a percent off, when I could simply do a 'limit 1' via the GET variable. Still, this was in the application where there users probably aren't technical enough to try, they wouldn't have that sort of code on a login page, right?
I can't even just add 1 to the bad developer for this. It's a minimum of +10 for allowing such simple "hacks" to let someone into the system. Below is an example of literally what I could put as the username:
' or user='admin'#
No password needed. No sanitisation done. I might forgive them if it was a 12 year old system, but the earliest entry in the database (the developer's username) was last year. It used jQuery 1.10 and bootstrap 3.3.8, which only came out in November. Bad person hitting a keyboard (no longer afforded developer status).
Ok, so what can be done to prevent these things? Firstly, stop using mysql_* functions. They were deprecated in PHP 5.5, and there's other alternatives around which are better, like PDO and mysqli_*. That said, in the given example code, I could have done the following with mysqli_query functionality and potentially had worse consequences:
';drop table users;#
Which reminds me so much of the following XKCD comic:
Aside from using current database interaction functionality, you should NEVER put data sent from a browser into a query. Always sanitise it in one way or another. Ideally use parameterized queries which will take care of sanitisation for you, and also will build the query in a much safer manner (PDO is my friend, it can be yours too).
There's millions of web pages out there on how to prevent SQL injection, how to sanitise data input, how not to code, and why not to trust users. Okay, so there's also a lot of bad resources out there just the same, but there's no reason anyone should be passing anything from the browser straight to the database.
Developers like to blame users when something in the system goes wrong. Sure it would be a user issue if I logged in in that manner, but it's more of a developer issue for allowing it in the first place. Those things aren't limited to just logins either though. If you delete data using the following manner:
delete.php?id=2
Without input sanitisation, I can most likely do:
delete.php?id=2+or+id+>+0#
Wave goodbye to your data or, best case, enjoy finding everything being flagged as deleted in the database.
NEVER trust your users to input things correctly, and never trust their input.
