Hobbyist_PHPer Posted April 7, 2011 Share Posted April 7, 2011 Hi everyone, So, like my name says, I'm just a hobbyist PHPer, but I write the occassional PHP application for people, I've been doing it for quite a while and I fear that perhaps my way of securing my applications may be a bit antiquated... I was hoping that you guys/gals might be able to take a look and give me some help with perhaps how I could go about making these apps more secure... So, without further ado, here it is... standard application page, e.g. index.php <? session_start(); if(!$_SESSION['Condition'] == 'Logged') { header("Location: login.php"); } elseif($_SESSION['Condition'] == 'Logged') { require "connection.inc"; ?> <? } ?> login.php page <? if(isset($_POST['Login'])) { include_once 'connection.inc'; $count = 0; $query = "SELECT UserID FROM Users WHERE UserName = '$_POST[username]' AND UserPassword = '$_POST[password]'"; $results = mysql_query($query)or die(mysql_error()); $count = mysql_num_rows($results); while($row = mysql_fetch_array($results)) { $UserID = $row['UserID']; } if ($count == 1) { header("Location: loginaction.php?UserID=$UserID"); } } ?> <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> <html xmlns="http://www.w3.org/1999/xhtml" > <head> <title></title> <link rel="stylesheet" type="text/css" href="StyleSheet.css" /> <script language="javascript" type="text/javascript"> function loginValidate(form) { if (form.username.value == '') { alert('You must supply a Username.'); form.username.focus(); return false; } if (form.password.value == '') { alert('You must supply a Password.'); form.password.focus(); return false; } else { return true; } } </script> </head> <body> <? include_once 'header.inc'; ?> <div id="LoginBox"> <div id="SubFormBoxHeading"> Log In </div> <form id="thisform" action="<? echo $_SERVER['PHP_SELF']; ?>" onsubmit="return loginValidate(this)" method="post"> <table> <tr> <td colspan="2"> <? if (isset($_POST['Login']) && !$count == 1) { echo '<h3>Wrong Username and/or Password</h3>'; } ?> </td> </tr> <tr> <td class="Labels">Username:</td> <td><input type="input" id="username" name="username" size="20" /></td> </tr> <tr> <td class="Labels">Password:</td> <td><input type="password" id="password" name="password" size="20" /></td> </tr> <tr> <td colspan="2"> <div style="text-align: center; margin-top: 20px; margin-bottom: 20px;"> <input type="submit" id="Login" name="Login" value="Log In" /> </div> </td> </tr> </table> </form> </div> <? include_once 'footer.inc'; ?> </body> </html> loginaction.php page <? session_start(); $_SESSION['Condition'] = 'Logged'; $_SESSION['UserID'] = $_GET['UserID']; header("Location: index.php"); ?> and finally, the logout.php page <? session_start(); unset($_SESSION['Condition']); unset($_SESSION['UserID']); session_destroy(); header("Location: index.php"); ?> Quote Link to comment Share on other sites More sharing options...
phil88 Posted April 7, 2011 Share Posted April 7, 2011 Firstly, congratulations on looking to better your skills. A lot of people get into a way of doing things and are too stubborn to change, even if the way they're doing it is awful. That being said, you're right in thinking that your code is vulnerable to security exploits The first one that stands out to me is this line of your login.php file; $query = "SELECT UserID FROM Users WHERE UserName = '$_POST[username]' AND UserPassword = '$_POST[password]'"; It's open to a SQL Injection attack, which could cause all kinds of problems from deleting or updating your tables, to silently stealing your data. The problem is that those $_POST values could contain absolutely anything. If we send admin';# as the username, for example, your query will become; $query = "SELECT UserID FROM Users WHERE UserName = 'admin';# AND UserPassword = '$_POST[password]'"; # is a MySQL comment, so everything after it will be ignored. The query will then return the 'admin' row regardless of the password entered. Read up on sanitizing the input values, preventing mysql injections and using prepared statements (Google will find hundreds of tutorials, there's no need me rewriting the wheel here). Another problem that stands out is your loginaction.php script. There's nothing to stop a user navigating to www.yourdomain.com/loginaction?php?UserId=1 (or any other arbitrary user id) and logging in as that user id. It would make more sense to have the session creation in login.php inside the $count == 1 if statement. Quote Link to comment Share on other sites More sharing options...
Hobbyist_PHPer Posted April 7, 2011 Author Share Posted April 7, 2011 Thank you very much for that information, I certainly do appreciate it... Once I feel that I have conquered those, I'll post my new code... Edit: Would these be similar to the Stored Procedures that I use in my .NET w/SQL applications? Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.