jamesxg1 Posted February 9, 2009 Share Posted February 9, 2009 here is my login.php script: <?php session_start(); require("../db/db.php"); require("../db/settings.php"); //Get the username the user has entered $username = $_POST['username']; //Get the password the user has entered $password = $_POST['password']; //turn the password they entered into md5 to compare with the DB $password = md5(sha1($password)); //keep the name of the user in session //What point does this serve? and how do u even know if the username is in the session yet? -chris //$loginname = $_SESSION['username']; //check to see if logged in already if (isset( $_SESSION['loggedin'] )) { die("<script type=\"text/javascript\" language=\"JavaScript\"> setTimeout('Redirect()',0); function Redirect() { location.href = '../main/index.php'; } </script>"); } //if not logged in, then run other script instead else { //find if the login button was pressed if (isset($_POST['submit'])) { //if username was entered, continue if($username && $password) { $result = mysql_query($sql); //If the user gets to here, then they have typed both a //username and password, so we may now go onto finding //out if they exist in the DB get rows where the username //field matches the username or email field in the database //with same password $sql = "SELECT * FROM users WHERE (username='$username' OR email='$username') AND password='$password'"; $result = mysql_query($sql); //check to see if the account is activated $morerow = mysql_fetch_array($result); //if there was a row returned, then obiously there is an account with the correct username/password. if (mysql_num_rows($result) > 0) { // make sure account isn't pending activation if ($morerow['groupid'] != "P") { //set the global session variables $_SESSION['loggedin']="TRUE"; $_SESSION['username'] = $username; $_SESSION['groupid'] = $morerow['groupid']; $_SESSION['id'] = $morerow['id']; $_SESSION['lastname'] = $morerow['lastname']; $_SESSION['firstname'] = $morerow['firstname']; $_SESSION['email'] = $morerow['email']; die("<script type=\"text/javascript\" language=\"JavaScript\"> setTimeout('Redirect()',0); function Redirect() { location.href = '../main/index.php'; } </script>"); } else echo "<div align=\"center\"><font color=red> <img src=\"../images/exclam.PNG\"/><i>Your account is not authorised for login at this time please try later.</i></font></div>"; } else { echo "<div align=\"center\"><font color=red> <img src=\"../images/exclam.PNG\"/><i>Invalid login<br>If you are a new user please register</i></font></div>"; } } else { echo "<div align=\"center\"><font color=red> <img src=\"../images/exclam.PNG\"/><i> You must enter a username and password!</i></font></div>"; } } } ?> <html> <head> <script language="javascript" type="text/javascript" src="../inc/reg/niceforms.js"></script> <link rel="stylesheet" type="text/css" media="all" href="../template/img/niceforms-default.css" /> <title><?php print $title?> - Login</title> </head> <div align="center"><img src="../images/works.png" /> <form method="POST" action="<?php echo "$PHP_SELF"; ?>"> <p align="center"><div id="container"> <fieldset> <dl> <dt><label for="username">Username:</label></dt><br> <dd><input type="text" name="username" id="username" size="20" maxlength="255" /></dd> </dl> </p> <p align="center"> <dl> <dt><label for="password">Password:</label></dt> <dd><input type="password" name="password" id="password" /></dd> </dl> </p> <fieldset class="action"> <input type="submit" name="submit" id="submit" value="Submit" /> </fieldset> </form> </p> <br /> </form> </div> <p><center> <p><a href="../register/register.php">New User Registration</a> - <a href="../modifyInfo/recoverPassword.php">Forgot Your Password?</a></p> <p></a></p> </center></p> </html> can someone help me add this session_regenerate_id() to it so that i have a more secure website ? thanks. Quote Link to comment Share on other sites More sharing options...
jamesxg1 Posted February 9, 2009 Author Share Posted February 9, 2009 Can anyone help here please :S ?? Quote Link to comment Share on other sites More sharing options...
mat-tastic Posted February 9, 2009 Share Posted February 9, 2009 From the looks of it instead of using session_regenerate_id() the first thing you need to do is filter your input to prevent mysql injection. Use mysql_escape_string(); Quote Link to comment Share on other sites More sharing options...
mat-tastic Posted February 9, 2009 Share Posted February 9, 2009 //Get the username the user has entered $username = mysql_escape_string($_POST['username']); //Get the password the user has entered $password = mysql_escape_string($_POST['password']); //turn the password they entered into md5 to compare with the DB $password = md5(sha1($password)); Quote Link to comment Share on other sites More sharing options...
jamesxg1 Posted February 9, 2009 Author Share Posted February 9, 2009 //Get the username the user has entered $username = mysql_escape_string($_POST['username']); //Get the password the user has entered $password = mysql_escape_string($_POST['password']); //turn the password they entered into md5 to compare with the DB $password = md5(sha1($password)); hiya, thanks , done i tottally forgot about that i should also put strip_tags aswell , but does anyone know how i would use session_regenerate_id() ? Quote Link to comment Share on other sites More sharing options...
mat-tastic Posted February 9, 2009 Share Posted February 9, 2009 as far as I am aware, if copies all the old information over to the new one: so just run session_regenerate_id(); anywhere you want. I am not an expert though. There is a good example here of how it works: http://uk.php.net/session_regenerate_id Quote Link to comment Share on other sites More sharing options...
jamesxg1 Posted February 9, 2009 Author Share Posted February 9, 2009 as far as I am aware, if copies all the old information over to the new one: so just run session_regenerate_id(); anywhere you want. I am not an expert though. There is a good example here of how it works: http://uk.php.net/session_regenerate_id ok i will do 2 secz , and this is my security now i have just reminded about it lol , if anyone see's any vunrabilities please post ? //Get the username the user has entered $username = strip_tags(mysql_escape_string(filter_input(INPUT_POST, 'username', FILTER_SANITIZE_STRING))); $password = strip_tags(mysql_escape_string(filter_input(INPUT_POST, 'password', FILTER_SANITIZE_STRING) )); //turn the password they entered into md5 to compare with the DB $password = md5(sha1($password)); Quote Link to comment Share on other sites More sharing options...
trq Posted February 9, 2009 Share Posted February 9, 2009 You really only need to call session_regenerate_id when a users privilages change. ie; When you log them in. Quote Link to comment Share on other sites More sharing options...
jamesxg1 Posted February 9, 2009 Author Share Posted February 9, 2009 as far as I am aware, if copies all the old information over to the new one: so just run session_regenerate_id(); anywhere you want. I am not an expert though. There is a good example here of how it works: http://uk.php.net/session_regenerate_id Done!, i got i working justed used a IF statment eg. IF loggedin session_regenerate_id(); thanks guys ok i will do 2 secz , and this is my security now i have just reminded about it lol , if anyone see's any vunrabilities please post ? //Get the username the user has entered $username = strip_tags(mysql_escape_string(filter_input(INPUT_POST, 'username', FILTER_SANITIZE_STRING))); $password = strip_tags(mysql_escape_string(filter_input(INPUT_POST, 'password', FILTER_SANITIZE_STRING) )); //turn the password they entered into md5 to compare with the DB $password = md5(sha1($password)); Quote Link to comment Share on other sites More sharing options...
mat-tastic Posted February 9, 2009 Share Posted February 9, 2009 Well I can say is your sure big on your security, I suppose thats a good thing though. Quote Link to comment Share on other sites More sharing options...
printf Posted February 9, 2009 Share Posted February 9, 2009 Why do you add all those security measures, when you don't even know if your variables are set! You really need to rewrite this, thinking about the logical order one should use to validate a user login. 1. if they already logged in, kick them to a service page 2. check if incoming variables are set 3. trim those variables and then check if they empty 4. escape the $username and run your query Don't rely on $_POST['submit'], it is not guaranteed to be set! Quote Link to comment Share on other sites More sharing options...
jamesxg1 Posted February 10, 2009 Author Share Posted February 10, 2009 Hiya, I took up some of your advice and here is the final product , <?php session_start(); session_regenerate_id(); session_encode(); require("../db/db.php"); require("../db/settings.php"); $username = strip_tags(mysql_escape_string(trim(ltrim(rtrim(htmlentities(filter_input(INPUT_POST, 'username', FILTER_SANITIZE_STRING))))))); $password = strip_tags(mysql_escape_string(trim(ltrim(rtrim(htmlentities(filter_input(INPUT_POST, 'password', FILTER_SANITIZE_STRING))))))); $password = md5(sha1($password)); //$loginname = $_SESSION['username']; if (isset( $_SESSION['loggedin'] )) { die (header ('Location: ../main/index.php')); } else { if (isset($_POST['submit'])) { if($username && $password) { $result = mysql_query($sql); $sql = "SELECT * FROM users WHERE (username='$username') AND password='$password'"; $result = mysql_query($sql); $morerow = mysql_fetch_array($result); if (mysql_num_rows($result) > 0) { if ($morerow['groupid'] != "P") { //set the global session variables $_SESSION['loggedin']="TRUE"; $_SESSION['username'] = $username; $_SESSION['groupid'] = $morerow['groupid']; $_SESSION['id'] = $morerow['id']; $_SESSION['lastname'] = $morerow['lastname']; $_SESSION['firstname'] = $morerow['firstname']; $_SESSION['email'] = $morerow['email']; die (header ('Location: ../main/index.php')); } else echo "<div align=\"center\"><font color=red> <img src=\"../images/exclam.PNG\"/><i>Your account is not authorised for login at this time please try later.</i></font></div>"; } else { echo "<div align=\"center\"><font color=red> <img src=\"../images/exclam.PNG\"/><i>Invalid login<br>If you are a new user please register</i></font></div>"; } } else { echo "<div align=\"center\"><font color=red> <img src=\"../images/exclam.PNG\"/><i> You must enter a username and password!</i></font></div>"; } } } ?> <html> <head> <script language="javascript" type="text/javascript" src="../inc/reg/niceforms.js"></script> <link rel="stylesheet" type="text/css" media="all" href="../template/img/niceforms-default.css" /> <title><?php print $title?> - Login</title> </head> <div align="center"><img src="../images/works.png" /> <form method="POST" action="<?php echo "$PHP_SELF"; ?>"> <p align="center"><div id="container"> <fieldset> <dl> <dt><label for="username">Username:</label></dt><br> <dd><input type="text" name="username" id="username" size="20" maxlength="255" /></dd> </dl> </p> <p align="center"> <dl> <dt><label for="password">Password:</label></dt> <dd><input type="password" name="password" id="password" /></dd> </dl> </p> <fieldset class="action"> <input type="submit" name="submit" id="submit" value="Submit" /> </fieldset> </form> </p> <br /> </form> </div> <p><center> <p><a href="../register/register.php">New User Registration</a> - <a href="../modifyInfo/recoverPassword.php">Forgot Your Password?</a></p> <p></a></p> </center></p> </html> <?php error_reporting(E_ALL) ; ini_set('display_errors','1'); ?> Quote Link to comment Share on other sites More sharing options...
jamesxg1 Posted February 10, 2009 Author Share Posted February 10, 2009 is there any at all vunrabilitie there ? i will fix any upon finding them ? Quote Link to comment Share on other sites More sharing options...
trq Posted February 10, 2009 Share Posted February 10, 2009 1: These lines.... $username = strip_tags(mysql_escape_string(trim(ltrim(rtrim(htmlentities(filter_input(INPUT_POST, 'username', FILTER_SANITIZE_STRING))))))); Are rediculous. trim alone does the same job as ltrim & rtrim so they are not needed. Your usernames / passwords shouldn't contain any html so I see no need for a call to htmlentities and you should only need strip_tags if magic quotes are enabled. 2: Your call to mysql_query() should be wrapped in an if() statement so as to trap any errors that may occur. 3: Your call to session_regenerate_id should be where the '//set the global session variables' comment is. 4: This line.... die (header ('Location: ../main/index.php')); Makes absolutley no sense. die expects a string header returns nothing (void). Quote Link to comment Share on other sites More sharing options...
jamesxg1 Posted February 10, 2009 Author Share Posted February 10, 2009 1: These lines.... $username = strip_tags(mysql_escape_string(trim(ltrim(rtrim(htmlentities(filter_input(INPUT_POST, 'username', FILTER_SANITIZE_STRING))))))); Are rediculous. trim alone does the same job as ltrim & rtrim so they are not needed. Your usernames / passwords shouldn't contain any html so I see no need for a call to htmlentities and you should only need strip_tags if magic quotes are enabled. 2: Your call to mysql_query() should be wrapped in an if() statement so as to trap any errors that may occur. 3: Your call to session_regenerate_id should be where the '//set the global session variables' comment is. 4: This line.... die (header ('Location: ../main/index.php')); Makes absolutley no sense. die expects a string header returns nothing (void). Yes true but if they input html at all for injection of anything ect, and how do i wrap it :S ?, and ok i will change now , and the die works fine thoe :S, and i have no errors on the page what so ever :S Quote Link to comment Share on other sites More sharing options...
jamesxg1 Posted February 10, 2009 Author Share Posted February 10, 2009 1: These lines.... $username = strip_tags(mysql_escape_string(trim(ltrim(rtrim(htmlentities(filter_input(INPUT_POST, 'username', FILTER_SANITIZE_STRING))))))); Are rediculous. trim alone does the same job as ltrim & rtrim so they are not needed. Your usernames / passwords shouldn't contain any html so I see no need for a call to htmlentities and you should only need strip_tags if magic quotes are enabled. 2: Your call to mysql_query() should be wrapped in an if() statement so as to trap any errors that may occur. 3: Your call to session_regenerate_id should be where the '//set the global session variables' comment is. 4: This line.... die (header ('Location: ../main/index.php')); Makes absolutley no sense. die expects a string header returns nothing (void). Yes true but if they input html at all for injection of anything ect, and how do i wrap it :S ?, and ok i will change now , and the die works fine thoe :S, and i have no errors on the page what so ever :S oh. . i just noticed lol, i cant put session_regenerate_id(); in that area as that area is checking for suspended accounts Quote Link to comment Share on other sites More sharing options...
trq Posted February 10, 2009 Share Posted February 10, 2009 oh. . i just noticed lol, i cant put session_regenerate_id();() in that area as that area is checking for suspended accounts It may very well do, but its also the same area you actually log your users in. Yes true but if they input html at all for injection of anything ect This should be checked on registration, not log in. how do i wrap it What? Your query? if ($result = mysql_query($sql)) { if (mysql_num_rows($result)) { // log user in. } else { // user does not exist. } } else { // query failed. } and the die() works fine thoe Sorry, but that doesn't make sense. It may work, but if so its a side effect. and i have no errors on the page what so ever :S Confident aren't we? Quote Link to comment Share on other sites More sharing options...
printf Posted February 10, 2009 Share Posted February 10, 2009 I'll give you an example of logical flow control so maybe it will help you to understand how you should order you control structure. You really need to learn this because you won't understand the fundamentals of programing until you do. The only reason I am telling you this is to help you because I have read some of your post(s) giving other people advice on securing their applications and most of what you have shared is bad advice. I know your only trying to helpful, but giving people idea's that go against common security measures won't help anyone. Now for my example on rewriting your code so it does things in the order it should do things... <?php /* session_start () */ session_start (); /* * kick them to service page if they * are already logged in */ if ( isset ( $_SESSION['loggedin'] ) ) { /* close the session, write the session file if it's changed */ session_write_close (); /* using relative paths in url(s) is wrong, use absolute paths only */ header ( 'Location: ../main/index.php' ); /* force script to exit */ exit (); } /* include needed files */ /* * using relative paths is a bad idea by * design, always use absolute paths. If * use a relative path PHP must go and * look for the file... */ require '../db/db.php'; require '../db/settings.php'; /* our error container */ $error = ''; /* check if we have the needed post data */ if ( isset ( $_POST['username'] ) && isset ( $_POST['password'] ) ) { /* trim variable values */ $user = trim ( $_POST['username'] ); $pass = trim ( $_POST['password'] ); /* check trimmed variable values */ if ( ! empty ( $user ) && ! empty ( $pass ) ) { /* escape the incoming $_POST['username'] value */ $user = mysql_real_escape_string ( $user ); $sql = "SELECT * FROM users WHERE username = '" . $user . "' OR email = '" . $user . "' AND password = '" . md5 ( sha1 ( $pass ) ) . "';"; /* run the query */ $result = mysql_query ( $sql ); /* check if we have any results */ if ( mysql_num_rows ( $result ) > 0 ) { /* load the result resource */ $morerow = mysql_fetch_array ( $result ); /* check that the account isn't pending activation */ if ( $morerow['groupid'] != 'P' ) { /* set the session variables */ $_SESSION['loggedin'] = true; $_SESSION['username'] = $user; $_SESSION['groupid'] = $morerow['groupid']; $_SESSION['id'] = $morerow['id']; $_SESSION['lastname'] = $morerow['lastname']; $_SESSION['firstname'] = $morerow['firstname']; $_SESSION['email'] = $morerow['email']; /* close the session, write the session file because it's changed */ session_write_close (); /* using relative paths in url(s) is wrong, use absolute paths only */ header ( 'Location: ../main/index.php' ); /* force script to exit */ exit (); } else { /* account pending activation */ $error = '<div align="center"><font color=red><img src="../images/exclam.PNG"/><i>Invalid login<br>Your account is not authorised for login at this time please try later.</i></font></div>'; } } else { /* no matching user found */ $error = '<div align="center"><font color=red><img src="../images/exclam.PNG"/><i>Invalid login<br>If you are a new user please register.</i></font></div>'; } /* free the result resource */ mysql_free_result ( $result ); } else { /* username and or password invalid */ $error = '<div align="center"><font color=red><img src="../images/exclam.PNG"/><i>Username and or Password was not entered!</i></font></div>'; } } else { /* remove the session, there no need to keep it */ $_SESSION = array (); session_destroy (); } ?> <html> <head> <script language="javascript" type="text/javascript" src="../inc/reg/niceforms.js"></script> <link rel="stylesheet" type="text/css" media="all" href="../template/img/niceforms-default.css" /> <title> <?php echo $title; ?> - Login </title> </head> <body> <div align="center"> <?php if ( ! empty ( $error ) ) { echo ' <br /> $error <br /> ; } ?> <img src="../images/works.png" /> <form method="POST" action="<?php echo "$PHP_SELF"; ?>"> <p align="center"> <div id="container"> <fieldset> <dl> <dt> <label for="username"> Username: </label> </dt> <br /> <dd> <input type="text" name="username" id="username" size="20" maxlength="255" /> </dd> </dl> </fieldset> </div> </p> <p align="center"> <div id="container"> <fieldset> <dl> <dt> <label for="password"> Password: </label> </dt> <br /> <dd> <input type="password" name="password" id="password" /> </dd> </dl> </fieldset> </div> </p> <p> <center> <p> <a href="../register/register.php"> New User Registration </a> - <a href="../modifyInfo/recoverPassword.php"> Forgot Your Password? </a> </p> </center> </p> </form> </div> </body> </html> Quote Link to comment Share on other sites More sharing options...
jamesxg1 Posted February 10, 2009 Author Share Posted February 10, 2009 I'll give you an example of logical flow control so maybe it will help you to understand how you should order you control structure. You really need to learn this because you won't understand the fundamentals of programing until you do. The only reason I am telling you this is to help you because I have read some of your post(s) giving other people advice on securing their applications and most of what you have shared is bad advice. I know your only trying to helpful, but giving people idea's that go against common security measures won't help anyone. Now for my example on rewriting your code so it does things in the order it should do things... <?php /* session_start () */ session_start (); /* * kick them to service page if they * are already logged in */ if ( isset ( $_SESSION['loggedin'] ) ) { /* close the session, write the session file if it's changed */ session_write_close (); /* using relative paths in url(s) is wrong, use absolute paths only */ header ( 'Location: ../main/index.php' ); /* force script to exit */ exit (); } /* include needed files */ /* * using relative paths is a bad idea by * design, always use absolute paths. If * use a relative path PHP must go and * look for the file... */ require '../db/db.php'; require '../db/settings.php'; /* our error container */ $error = ''; /* check if we have the needed post data */ if ( isset ( $_POST['username'] ) && isset ( $_POST['password'] ) ) { /* trim variable values */ $user = trim ( $_POST['username'] ); $pass = trim ( $_POST['password'] ); /* check trimmed variable values */ if ( ! empty ( $user ) && ! empty ( $pass ) ) { /* escape the incoming $_POST['username'] value */ $user = mysql_real_escape_string ( $user ); $sql = "SELECT * FROM users WHERE username = '" . $user . "' OR email = '" . $user . "' AND password = '" . md5 ( sha1 ( $pass ) ) . "';"; /* run the query */ $result = mysql_query ( $sql ); /* check if we have any results */ if ( mysql_num_rows ( $result ) > 0 ) { /* load the result resource */ $morerow = mysql_fetch_array ( $result ); /* check that the account isn't pending activation */ if ( $morerow['groupid'] != 'P' ) { /* set the session variables */ $_SESSION['loggedin'] = true; $_SESSION['username'] = $user; $_SESSION['groupid'] = $morerow['groupid']; $_SESSION['id'] = $morerow['id']; $_SESSION['lastname'] = $morerow['lastname']; $_SESSION['firstname'] = $morerow['firstname']; $_SESSION['email'] = $morerow['email']; /* close the session, write the session file because it's changed */ session_write_close (); /* using relative paths in url(s) is wrong, use absolute paths only */ header ( 'Location: ../main/index.php' ); /* force script to exit */ exit (); } else { /* account pending activation */ $error = '<div align="center"><font color=red><img src="../images/exclam.PNG"/><i>Invalid login<br>Your account is not authorised for login at this time please try later.</i></font></div>'; } } else { /* no matching user found */ $error = '<div align="center"><font color=red><img src="../images/exclam.PNG"/><i>Invalid login<br>If you are a new user please register.</i></font></div>'; } /* free the result resource */ mysql_free_result ( $result ); } else { /* username and or password invalid */ $error = '<div align="center"><font color=red><img src="../images/exclam.PNG"/><i>Username and or Password was not entered!</i></font></div>'; } } else { /* remove the session, there no need to keep it */ $_SESSION = array (); session_destroy (); } ?> <html> <head> <script language="javascript" type="text/javascript" src="../inc/reg/niceforms.js"></script> <link rel="stylesheet" type="text/css" media="all" href="../template/img/niceforms-default.css" /> <title> <?php echo $title; ?> - Login </title> </head> <body> <div align="center"> <?php if ( ! empty ( $error ) ) { echo ' <br /> $error <br /> ; } ?> <img src="../images/works.png" /> <form method="POST" action="<?php echo "$PHP_SELF"; ?>"> <p align="center"> <div id="container"> <fieldset> <dl> <dt> <label for="username"> Username: </label> </dt> <br /> <dd> <input type="text" name="username" id="username" size="20" maxlength="255" /> </dd> </dl> </fieldset> </div> </p> <p align="center"> <div id="container"> <fieldset> <dl> <dt> <label for="password"> Password: </label> </dt> <br /> <dd> <input type="password" name="password" id="password" /> </dd> </dl> </fieldset> </div> </p> <p> <center> <p> <a href="../register/register.php"> New User Registration </a> - <a href="../modifyInfo/recoverPassword.php"> Forgot Your Password? </a> </p> </center> </p> </form> </div> </body> </html> hiya , ok thankyou i will use this instead of the one i have i will only make a simple mod or two to this because my include files need to use the input filter on any vars that are being used ($username, $password), thankyou for this 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.