Page 1 of 1

Forgot Password: Is my code okay? Newbie

Posted: Thu Apr 24, 2014 9:27 pm
by tristan5522
Hey everyone, this is my first post. Currently this code resets the user's password and replaces it with some random code in the database. Not sure what I am doing wrong, any help would be greatly appreciated.

Code: Select all

<?php
$heading = "Forgot Password";

if(isset($_GET['action']) && $_GET['action'] == "fpwd")
{
if(count($_POST) > 0)
{
if(isset($_POST['user_email']))
{
$email_address = $_POST['user_email'];

$sqlemail = "select user_email from ".TABLE_user." where user_email = '$email_address'";
$resemail = mysql_query($sqlemail);

$password = "user".rand(1000,50000);

$sql_update = "update ".TABLE_user." set 'password' = '".md5($password)."' where 'user_email' = '$email_address'";
$res = mysql_query($sql);


$to = $email_address;
$subject = 'Reset Password';
$message = 'Your new password: '.$password;
$headers = 'From: '.STORE_EMAIL.'' . "\r\n";

if(mail($to, $subject, $message, $headers))
{
fw_goto_page_header(fw_create_link(FILENAME_FORGOT_PWD,'msg=1'));
}

}
}
}

?>

Re: Forgot Password: Is my code okay? Newbie

Posted: Thu Apr 24, 2014 9:37 pm
by Celauran

Code: Select all

$sqlemail = "select user_email from ".TABLE_user." where user_email = '$email_address'";
You're selecting a column whose value you already know? Odd. More importantly, you're passing user data directly into a query. This will end badly. Sanitize your inputs. Always.

The mysql_ functions have been deprecated and should not be used. Consider using PDO instead, ideally in conjunction with prepared statements.

md5 is entirely unsuitable for hashing passwords. Use bcrypt.

Lastly, you shouldn't be emailing users their passwords. A better solution would be to email them a link where they can choose a new password.

Re: Forgot Password: Is my code okay? Newbie

Posted: Fri Apr 25, 2014 2:31 pm
by social_experiment
Celauran wrote:Lastly, you shouldn't be emailing users their passwords.
Yeah, this is a bad practise yet it is so popular. I recently worked on a e-commerce system and even the payment gateway company emailed me a password.

Code: Select all

<?php $password = "user".rand(1000,50000); ?>
This isn't very effective; rather go for something different; your reset password should contain alpha-numeric characters and be at least 10 characters in length.

Code: Select all

<?php
function createNewPassword()
{
		$randomData = array(    // Array of characters used to create password		
		'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 
		'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 
		'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z', '1',
		'2', '3', '4', '5', '6', '7', '8', '9', '0', 
		'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 
		'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 
		's', 't', 'u', 'v', 'w', 'x', 'y', 'z', '!',
		'@', '#', '$', '%', '^', '&', '*', '(', ')',
		'-', '=', '`','\'', '"', '{', '}', '[', ']', 
		'<', '>', '/', '?', '.', '|', '_', 
		);

	$randomString = false;			
			
	shuffle($randomData);
	$array = array_rand($randomData, 15);
			
	foreach ($array as $key => $value) 
	{
		$randomString .= $randomData[$key];
	}
	
	return $randomString;
}
?>
As a rule you should as the user to reset their password once you have sent a reset password email; before they are even allowed to login, that ensures they change the password immediately.

Re: Forgot Password: Is my code okay? Newbie

Posted: Fri Apr 25, 2014 2:39 pm
by Celauran
I don't know that you want to be generating a new password at all. You can't mail it to them, so what's it for? You don't necessarily want to overwrite the original password; what if they end up remembering it?