8 Replies - 1695 Views - Last Post: 13 March 2013 - 12:43 PM Rate Topic: -----

#1 adamoutram  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 2
  • Joined: 12-March 13

Problem with php 'change password' script / SQL statement

Posted 12 March 2013 - 10:48 AM

Hello,

I am creating a user form and form action where the user (already logged in using session variable) can change their md5 (i know MD5 is outdated and unsecured, this is for test purposes) encrypted account password stored in the sql database 'users' table.

I have a form which requests the inputs 'currentpassword', 'newpassword' and 'confirmnewpassword'. The form passes the entered data to passwordaction.php using $_POST.

The username is acquired from the $_SESSION 'autheticatedUser' and passwords acquired from the previous $_POST form variables. I then use an sql statement to get the password from the database for comparison to 'currentpassword' variable, DOES THIS COUNT AS INSECURE CLIENT SIDE VALIDATION? ?

I then have an SQL UPDATE statement to update the password row of the specified user in the database and the user is redirected and notified of success or failure using $_SESSION headers.

I have been reading and re-reading through my code trying to figure out where ive gone wrong as when trying to change a user account password I keep being returned to my login page (using $SESSION header) telling me it has updated properly however when i check the database the password has not been updated.

Im hoping someone elses view or perspective may be able to help me see what ive missed, can anyone suggest why my sql UPDATE statement is not working?

any constructive criticism welcome


below is my code for the 'action' php page


<?php

session_start();

$username = $_SESSION["authenticatedUser"];
$currentpassword = md5($_POST['currentpassword']);
$newpassword = md5($_POST['newpassword']);
$confirmnewpassword = md5($POST['confirmnewpassword']);

/* make a connection with database */
$con = mysql_connect("localhost", "root", "") or die(mysql_error());

/* select the database */
mysql_select_db("groupproject") or die(mysql_error());

$queryget = mysql_query("SELECT password FROM users WHERE username='$username'") or die(mysql_error());
$row = mysql_fetch_assoc($queryget);

$currentpasswordDB = $row['password'];

//check passwords

if ($currentpassword==$currentpasswordDB)

{
	if ($newpassword==$confirmnewpassword)
	{
	//success, change password in DB
		$querychange = mysql_query("UPDATE users SET password='$newpassword' WHERE username='$username'") or die(mysql_error());
	}
	else header("Location: passwordmismatch.php");

	if ($querychange = mysql_query){
		
		$_SESSION["passchange"] = "Your password has been changed, Please Log in";
		
		header("Location:login.php");
		
	}
	else header("Location:userchangepassword.php");

}

else header("Location: passwordmismatch.php");



?>



Is This A Good Question/Topic? 0
  • +

Replies To: Problem with php 'change password' script / SQL statement

#2 Dormilich  Icon User is offline

  • 痛覚残留
  • member icon

Reputation: 3392
  • View blog
  • Posts: 9,589
  • Joined: 08-June 10

Re: Problem with php 'change password' script / SQL statement

Posted 12 March 2013 - 11:28 AM

what is line #33 supposed to do?

besides that, you’re constantly overwriting headers. i.e. the redirect to passwordmismatch.php (#31) will never happen since in the next if-else it is overwritten with a new redirect.
Was This Post Helpful? 0
  • +
  • -

#3 adamoutram  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 2
  • Joined: 12-March 13

Re: Problem with php 'change password' script / SQL statement

Posted 13 March 2013 - 10:10 AM

View PostDormilich, on 12 March 2013 - 11:28 AM, said:

what is line #33 supposed to do?

besides that, you’re constantly overwriting headers. i.e. the redirect to passwordmismatch.php (#31) will never happen since in the next if-else it is overwritten with a new redirect.


line 33 was supposed to say if the sql update query was successful then complete the next action, i now know if (sql_query==true){ would have been the correct expression however i have now removed this line of code.

Ive been tweeking the code today, i echoed the variables (username,currectpassword, currectpasswordDB, newpassword, confirmnewpassword) onto the page when the form was submitted, i found that newpassword and confirmnewpassword are coming through as 2 different hash encrypted variables, ive check my form and the php action and cant work out why they are turning out differently, surely the same letters (input for newpassword and confirm new password) encrypted with the same hashing algorithm should output the same encrypted character set?

so heres my updated change password php script, can anyone suggest what might be the problem?

<?php

session_start();

$username = $_SESSION["authenticatedUser"];
$currentpassword = md5($_POST['currentpassword']);
$newpassword = md5($_POST['newpassword']);
$confirmnewpassword = md5($POST['confirmnewpassword']);

/* make a connection with database */
$con = mysql_connect("localhost", "root", "") or die(mysql_error());

/* select the database */
mysql_select_db("groupproject") or die(mysql_error());

$queryget = mysql_query("SELECT password FROM users WHERE username='$username'") or die(mysql_error());
$row = mysql_fetch_assoc($queryget);

$currentpasswordDB = $row['password'];

//check passwords

if ($currentpassword==$currentpasswordDB)

{
	if ($newpassword==$confirmnewpassword)
	{
	//success, change password in DB
		$querychange = mysql_query("UPDATE users SET password='$newpassword' WHERE username='$username'") or die(mysql_error());
		
		$_SESSION["passchange"] = "Your password has been changed, Please Log in";
		
		header("Location:login.php");
	}
	else header("Location: passwordmismatch.php");

}

else header("Location: passwordmismatch.php");

mysql_close($con);

?>


Was This Post Helpful? 0
  • +
  • -

#4 Cbeppe  Icon User is offline

  • D.I.C Head
  • member icon

Reputation: 31
  • View blog
  • Posts: 215
  • Joined: 16-September 09

Re: Problem with php 'change password' script / SQL statement

Posted 13 March 2013 - 11:20 AM

Sorry, I'm just seeing this in passing, so this is going to be really short and possibly incorrect, but here goes:

  • Try to echo out the unencrypted values of '$newpassword' and '$confirmnewpassword'. Maybe the two values aren't what you think.
  • Check whether the new password and the confirmation matches before making the database query. It's one less query and it will get it out of the way so that you won't have nested if-loops.
  • Consider using PDO or MySQLi - Completely unrelated, but still good advice.


EDIT: VERY quickly.written and untested code to go with it:
<?php

session_start();

$username = $_SESSION["authenticatedUser"];
$currentpassword = md5($_POST['currentpassword']);
$newpassword = md5($_POST['newpassword']);
$confirmnewpassword = md5($POST['confirmnewpassword']);

// Echo unencrypted variables
echo "New Pass: $_POST[newpassword] <br /> Confirm Newpass: $_POST[confirmnewpassword]";

// Check confirmation 
if ($currentpassword!=$currentpasswordDB) {
	header("Location: passwordmismatch.php");
}

/* DB Stuff */
$con = mysql_connect("localhost", "root", "") or die(mysql_error());
mysql_select_db("groupproject") or die(mysql_error());
$queryget = mysql_query("SELECT password FROM users WHERE username='$username'") or die(mysql_error());
$row = mysql_fetch_assoc($queryget);
$currentpasswordDB = $row['password'];

//check passwords
if ($currentpassword==$currentpasswordDB)
{
	//success, change password in DB
	$querychange = mysql_query("UPDATE users SET password='$newpassword' WHERE username='$username'") or die(mysql_error());
	$_SESSION["passchange"] = "Your password has been changed, Please Log in";
	header("Location:login.php");
} 
else header("Location: passwordmismatch.php");

mysql_close($con);

?>


This post has been edited by Cbeppe: 13 March 2013 - 11:26 AM

Was This Post Helpful? 0
  • +
  • -

#5 andrewsw  Icon User is online

  • Fire giant boob nipple gun!
  • member icon

Reputation: 2884
  • View blog
  • Posts: 9,567
  • Joined: 12-December 12

Re: Problem with php 'change password' script / SQL statement

Posted 13 March 2013 - 11:29 AM

You have a spelling error here:

md5($POST['confirmnewpassword'])


Turn on all error reporting as well:

error_reporting(E_ALL);
ini_set('display_errors', '1');

Was This Post Helpful? 1
  • +
  • -

#6 andrewsw  Icon User is online

  • Fire giant boob nipple gun!
  • member icon

Reputation: 2884
  • View blog
  • Posts: 9,567
  • Joined: 12-December 12

Re: Problem with php 'change password' script / SQL statement

Posted 13 March 2013 - 11:35 AM

You should also be checking the return result of your UPDATE query, which should be TRUE if successful .. and using parameterized queries, etc.
Was This Post Helpful? 0
  • +
  • -

#7 Cbeppe  Icon User is offline

  • D.I.C Head
  • member icon

Reputation: 31
  • View blog
  • Posts: 215
  • Joined: 16-September 09

Re: Problem with php 'change password' script / SQL statement

Posted 13 March 2013 - 11:56 AM

@andrewsw:

I think you found the entire problem with that spelling error :)

Just goes to show how important a second pair of eyes can be.
Was This Post Helpful? 0
  • +
  • -

#8 andrewsw  Icon User is online

  • Fire giant boob nipple gun!
  • member icon

Reputation: 2884
  • View blog
  • Posts: 9,567
  • Joined: 12-December 12

Re: Problem with php 'change password' script / SQL statement

Posted 13 March 2013 - 12:05 PM

View PostCbeppe, on 13 March 2013 - 11:56 AM, said:

@andrewsw:

I think you found the entire problem with that spelling error :)/>

Just goes to show how important a second pair of eyes can be.

No problem. If you turn on all error reporting during development then I assume it would have highlighted the error, or least produced a warning.
Was This Post Helpful? 0
  • +
  • -

#9 Cbeppe  Icon User is offline

  • D.I.C Head
  • member icon

Reputation: 31
  • View blog
  • Posts: 215
  • Joined: 16-September 09

Re: Problem with php 'change password' script / SQL statement

Posted 13 March 2013 - 12:43 PM

It definitely would. I never put the code past my text editor though. Just moved some stuff around to illustrate what I was saying. Error reporting should always be on when developing.
Was This Post Helpful? 0
  • +
  • -

Page 1 of 1