webweever Posted April 27, 2011 Share Posted April 27, 2011 I'm trying to write a script that changes my user passwords. I store the passwords in the DB as a md5 hash. My code is below, I keep getting error that the original username and password do not match. I require that the original credentials match so I can verify that I'm changing the password for the correct user. I suspect my problem is here but I'm not sure: $result = mysql_query("SELECT password FROM $tbl_name WHERE username='$username' and password = '".md5($pass)."'"); <?php $username = check_input($_POST['username']); $password = check_input($_POST['password']); $newpassword = check_input($_POST['newpassword']); $confirmpassword = check_input($_POST['confirmpassword']); if (!isset($_POST['submit'])) { // if page is not submitted to itself echo the form } else { $result = mysql_query("SELECT password FROM $tbl_name WHERE username='$username' and password = '".md5($pass)."'"); if(mysql_num_rows($result)){ if($newpassword==$confirmpassword){ $sql=mysql_query("UPDATE $tbl_name SET password='$newpassword' where username='$username'"); if($sql) { echo "Password Changed"; } else { // In case when problem while updating your new password echo "Error changing password, please email webmaster@mydomain.com"; } } else { // In case when new-password and retype-password do not match echo "New and confirmed password do not match please try again."; } } else { // In case of you have not correct User name and password echo "Current username and password do no match."; } } ?> <div class="pageContent"> <div id="main"> <div class="container"> <h1></h1> <h2>More text goes here.</h2> </div> <div class="container"> <!-- All protected data goes in here --> <?php if($_SESSION['id']){ echo '<form action="" method="post">'; echo '<h2>Username: </h2><input type="text" name="username" size="50" maxlength="255"><br/>'; echo '<h2>Password: </h2><input type="text" name="password" size="50" maxlength="255"><br/>'; echo '<h2>New Password: </h2><input type="text" name="newpassword" size="50" maxlength="255"><br/>'; echo '<h2>Confirm Password: </h2><input type="text" name="confirmpassword" size="50" maxlength="255"><br/>'; echo '<input type="submit" name="submit" value="Change Password">'; echo '</form>'; } else { echo '<h1>Please, <a href="index.php">login</a> and come back later!</h1>'; } ?> <!-- End: All protected data goes in here --> </div> <div class="container tutorial-info"> Footer goes here. </div> </div> </div> Any ideas? Quote Link to comment Share on other sites More sharing options...
drisate Posted April 27, 2011 Share Posted April 27, 2011 Juste a fast look and your using the var $password in your code $password = check_input($_POST['password']); But $pass un your MySQL querry. Was that an error? Quote Link to comment Share on other sites More sharing options...
Muddy_Funster Posted April 27, 2011 Share Posted April 27, 2011 also, $tbl_name isn't defined Quote Link to comment Share on other sites More sharing options...
cyberRobot Posted April 27, 2011 Share Posted April 27, 2011 Are you using mysql_real_escape_string(); maybe that's one thing that check_input() does? http://php.net/manual/en/function.mysql-real-escape-string.php If not, it would be fairly easy to change someone's (or everyone's) password with a quick SQL injection. Quote Link to comment Share on other sites More sharing options...
webweever Posted April 27, 2011 Author Share Posted April 27, 2011 Good catches, it's always great to have another set of eyes. I fixed the table name and the $password and still get the same error. mysql_num_rows(): supplied argument is not a valid MySQL result resource in myaccount.php on line 175 Current username and password do no match. Line 175 is: if(mysql_num_rows($result)){ <?php $username = check_input($_POST['username']); $password = check_input($_POST['password']); $newpassword = check_input($_POST['newpassword']); $confirmpassword = check_input($_POST['confirmpassword']); if (!isset($_POST['submit'])) { // if page is not submitted to itself echo the form } else { $result = mysql_query("SELECT password FROM members WHERE username='$username' and password = '".md5($password)."'"); if(mysql_num_rows($result)){ if($newpassword==$confirmpassword){ $sql=mysql_query("UPDATE members SET password='$newpassword' where username='$username'"); if($sql) { echo "Password Changed"; } else { // In case when problem while updating your new password echo "Error changing password, please email webmaster@signalwarrant.com"; } } else { // In case when new-password and retype-password do not match echo "New and confirmed password do not match please try again."; } } else { // In case of you have not correct User name and password echo "Current username and password do no match."; } } ?> That check_input function for my form looks like this: function check_input($data) { $data = trim($data); $data = stripslashes($data); $data = htmlspecialchars($data); return $data; } Could it be the function that's causing the problem and which is more secure; escape strings or the function I'm using? Quote Link to comment Share on other sites More sharing options...
btherl Posted April 27, 2011 Share Posted April 27, 2011 The first mysql query is causing an error - you'll need to print out mysql_error() to see why it's failing. For example: $result = mysql_query("SELECT password FROM members WHERE username='$username' and password = '".md5($password)."'") or die("Query failed: " . mysql_error()); Even better is if you can put the query into a variable first, and print out the query itself when displaying the error. Quote Link to comment Share on other sites More sharing options...
webweever Posted April 27, 2011 Author Share Posted April 27, 2011 I'm an Idiot... i had the column names wrong. Also had to md5 the new password. Thanks everyone for the help Aside from this I am interested in opinions on the function I'm using to check the form inputs which is more secure or is there a difference? Below is what works if anyone is interested. <?php $username = check_input($_POST['username']); $password = check_input($_POST['password']); $newpassword = check_input($_POST['newpassword']); $confirmpassword = check_input($_POST['confirmpassword']); if (!isset($_POST['submit'])) { // if page is not submitted to itself echo the form } else { $result = mysql_query("SELECT pass FROM members WHERE usr='$username' and pass = '".md5($password)."'")or die("Query failed: " . mysql_error()); if(mysql_num_rows($result)){ if($newpassword==$confirmpassword){ $sql=mysql_query("UPDATE members SET pass='".md5($newpassword)."' where usr='$username'"); if($sql) { echo "Password Changed"; } else { // In case when problem while updating your new password echo "Error changing password, please email webmaster@signalwarrant.com"; } } else { // In case when new-password and retype-password do not match echo "New and confirmed password do not match please try again."; } } else { // In case of you have not correct User name and password echo "Current username and password do no match."; } } ?> <div class="pageContent"> <div id="main"> <div class="container"> </div> <div class="container"> <?php if($_SESSION['id']){ echo '<form action="" method="post">'; echo '<h2>Username: </h2><input type="text" name="username" size="50" maxlength="255"><br/>'; echo '<h2>Password: </h2><input type="text" name="password" size="50" maxlength="255"><br/>'; echo '<h2>New Password: </h2><input type="text" name="newpassword" size="50" maxlength="255"><br/>'; echo '<h2>Confirm Password: </h2><input type="text" name="confirmpassword" size="50" maxlength="255"><br/>'; echo '<input type="submit" name="submit" value="Change Password">'; echo '</form>'; } else { echo '<h1>Please, <a href="index.php">login</a> and come back later!</h1>'; } ?> </div> Quote Link to comment Share on other sites More sharing options...
btherl Posted April 27, 2011 Share Posted April 27, 2011 function check_input($data) { $data = trim($data); $data = stripslashes($data); $data = htmlspecialchars($data); return $data; } trim() is fine, assuming you don't want whitespace at the start and end. stripslashes() is only required if your server has the magic quotes option on. htmlspecialchars() should not be used here - it should be used just before outputting data in an HTML page. mysql_real_escape_string() should be used, as the final step before the data goes into a MySQL query. If you already have existing passwords in your database that use that processing then you may have trouble changing it, as the passwords may not match afterwards. That's something to consider. Quote Link to comment Share on other sites More sharing options...
btherl Posted April 27, 2011 Share Posted April 27, 2011 Here's the usual processing I use (more or less, I actually use Smarty for the HTML and Postgres for my database) User data => trim => validate character set => Internal data Internal data => mysql_real_escape_string => A mysql query Internal data => htmlspecialchars => An HTML page "Internal data" is the form I store the data in internally. If it goes into mysql it gets mysql escaped, and if it goes into HTML it gets HTML escaped. And if it goes to some other destination like XML then it gets XML escaped, etc etc. Quote Link to comment Share on other sites More sharing options...
Drummin Posted April 28, 2011 Share Posted April 28, 2011 Now I run password through m5 like this before query. Is this a good practice? $link = mysqli_connect($host, $login, $pass); $user=mysqli_real_escape_string($link, $_POST['username']); $mdpass=MD5($_POST['pass1']); $query = "SELECT ID,level FROM ".$conf['tbl']['users']." where pass='$mdpass' AND user=\"$user\""; $result = mysql_query($query); $query_data = mysql_fetch_row($result); IF (!$query_data[0]) { $error = "You have submited an incorrect login and password combination. Please try again...."; } ELSE { Quote Link to comment Share on other sites More sharing options...
btherl Posted April 28, 2011 Share Posted April 28, 2011 Drummin, that looks fine from a security viewpoint. The output of md5() never contains dangerous characters so it doesn't need to be escaped. You might want to check if your query fails though. Quote Link to comment Share on other sites More sharing options...
Drummin Posted April 28, 2011 Share Posted April 28, 2011 Login has been working fine, I just wondered if it looked secure. Thank you for the reply. 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.