Yup, it’s 2014 and there are countless articles written about security your PHP code. Hell, most people use a framework. Today, I took over from another developer on a rather large internal quoting system, from another company. The company, rather ironically, class themselves are “the web experts”. The following gem was written on 23/10/2014. I present the following in all it’s scary, insecure glory:
<?php // Here, there was a database connection include + a single PHP file with a "begin_the_sessions()" function begin_the_session(); $cleaned = $_POST; $_SESSION['security'] = 0; $_SESSION['last_bad_username'] = $cleaned['u']; $result = mysql_query("SELECT `user_id_number`,`pass`,`fullname`,`is_user_approved`,`level` FROM users_lookup_table WHERE username= '".$cleaned['u']."' AND pass = '".$cleaned['p']."' AND `is_banned_number` = '0'") or die (mysql_error()); if (!$result) { $_SESSION['security']++; $attempts_left = 26 - $_SESSION['security']; echo "Details for the user <b>".$cleaned['u']."</b>, with the password <b><i>".$cleaned['u']."</i> were not found.</b>"; if ($attempts_left > 0) { echo '<br>You have ' . $attempts_left . ' tries left. Please contact the support line if you still cannot login'; } } // More nasty code followed ... . . .
Firstly, the system is rather large and deals with some seriously scary financial data. However, ignoring the fact that the system fails to use any sort of framework, templating system or even conventions, the is something I’d class as a bad piece of PHP code. There are enough security holes in those few lines of code to make you cry. Even ignorning things like passwords stored in plain text, using the depreciated mysql_* set of functions and not validating/sanitizing/escaping data the code is bad.
You may even be wondering about the ominious looking “begin_the_sessions()” function call on line one. Surely, that must address the horrible code that follows. Nope, I present to you the contents of that function (code comments are not mine!):
function begin_the_sessions() { // start sessions so people stay logged in // see http://php.net/manual/en/function.session-start.php for a guide $sess_id = $_GET['PHPSESSID']; $_SESSION['securitykey'] = $session_id; // Here, there was literally 20 lines of convouted nonesense that checked if the session folder existsed, was writable. IF the folder wasn;t it created the folder and did lots of silly other things return session_start(); }
Methinks a Laravel rewrite is in order 🙂
Wow, just WOW. Horrendous. I actually can’t believe that an agency could stay in business whilst writing code like that. I hope to God that that website contains no data about me… maybe I’ll see if I can truncate a few tables just in case 🙂 Haha, I joke… but I am glad you are re-writing it!
My word, that is just awful. WTF is going on with that “begin_the_session()” function? Seeing code like this makes me feel ill 🙁
Eh, no escaping on that query?