Change Password problems

[Mat_dennison]

hi

i've been asked to create a change password form for a friends band website, and ive come up with this code (below) and im having trouble to get it to work, i have the Database connecting, (i am able to log users in), also im not sure if my IF statements are set up correctly. When i go to change password, it looks as if the page is just refreshing its self with no errors coming up.

any help would be very appreciated

Thanks in advance

<?php
session_start();
require("db_connect.php");

$username = $_POST['uname']; 
$password = $_POST['pword'];
$npass = $_POST['nPass'];
$cnpass = $_POST['cnPass'];

$sql =      "SELECT * 				
		FROM login_details 
		WHERE username='$username' 
		AND password='$password'";  
$results = mysql_query($sql, $connect);
$numofrows = mysql_num_rows($results);
if ($numofrows == 0) {
				if ($npass == $cnpass) {
							$sql =	"UPDATE login_details  
									SET password='$npass' 
									WHERE username='$username'";
									header("Location: secure_page.php");
									die();
				} else {
									$_SESSION['error2']= "Passwords Do Not Match"; 
									header("Location: change_pass.php");
									die();
							}

	} else {
		$_SESSION['error1']= "Icorrect Username Or Password"; 
	header("Location: change_pass.php");
	die();
	}
?>

 

[Psycho]

Well, let's see what some of the problems are:

 

1. You don't validate any of the user input (e.g. trim the data, don't allow empty values, ensure there are values for the new password, etc)

2. You don't escape the input for the database leaving yourself open for sql injection

3. You don't hash the password which can compromise the users

4. You have your IF/ELSE conditions backwards

5. You create a query to update the password - but you never run it.

 

Also, both results of the page do a redirect. That's not necessarily a problem (assuming you actually completed everything you should before doing so). But, the fact that you get a page that never loads, it is quite possible that you have an infinite loop problem. Look at the two pages that get redirected to, do either of those redirect to other pages or back to the one above?

[Mat_dennison]

My friend has a lot of trust me in me atm, as im still learning PHP as we speak, im still unsure how to do some of them things.

I can do the trim bit, and change the if statements around, but struggling on the rest.

thanks

 

[Psycho]

Here is a rewrite of that entire script. I added logic to clean/parse the data and validate that there is something (at least 1 character) in the values. I also re-organized the logic so you don't need any ELSE statements by putting the error conditions first.  If anything fails it takes appropriate action and exits the script. It may be a little more verbose, but definitely more intuitive.

 

Lastly, I commented out all the header redirect lines and inserted echo statements to state the result. Once you get the correct results you can remove the echo statements and uncomment the header() functions.

 

<?php

session_start();
require("db_connect.php");
    
//Parse user input
$username = mysql_real_escape_string(trim($_POST['uname']));
$password = mysql_real_escape_string(trim($_POST['pword']));
$npass    = mysql_real_escape_string(trim($_POST['nPass']));
$cnpass   = mysql_real_escape_string(trim($_POST['cnPass']));
    
//Verify all required fileds are entered
//NOTE: Should also test for minimum field lengths / valid data etc
if(empty($username) || empty($password) || empty($npass) || empty($cnpass))
{
    //Not all required fields have values
    $_SESSION['error1']= "Missing required fields";
    echo "Missing required fields";
//    header("Location: change_pass.php");
    die();	
}

//Create and run query to authenticate user
$sql = "SELECT * 
        FROM login_details 
        WHERE username='$username' 
          AND password='$password'";  
$result = mysql_query($sql, $connect);
    
//Check if query failed
if(!$result)
{
    //Query failed
    $_SESSION['error1']= "There was a problem running the authentication query";
    echo "There was a problem running the authentication query";
//    header("Location: change_pass.php");
    die();
}
    
//Check if there was a match
if (mysql_num_rows($results) == 0)
{
    //No match for username password
    $_SESSION['error1']= "Icorrect Username Or Password";
    echo "Icorrect Username Or Password";
//    header("Location: change_pass.php");
    die();	
}
    
//Check that new passwords match and are valid
if ($npass != $cnpass)
{
    $_SESSION['error2']= "Passwords Do Not Match";
    echo "Passwords Do Not Match";
//    header("Location: change_pass.php");
    die();
}
       
//Create/run query to change password
$sql = "UPDATE login_details  
        SET password='$npass' 
        WHERE username='$username'";
$result = mysql_query($sql, $connect);
    
//Check if query failed
if(!$result)
{
    //Query failed
    $_SESSION['error1']= "There was a problem running the update query";
    echo "There was a problem running the update query";
//    header("Location: change_pass.php");
    die();
}
    
//Password was updated successfully
echo "Password was updated successfully";
//header("Location: secure_page.php");
die();
    
?>

[Mat_dennison]

Oh your brillaints, thanks very much, ill give it a go and let u know

[ChemicalBliss]

Just for clarity:

 

Validating user input is done usually using a PCRE regular expression to check that the characters/bits of input are the same - and in the same format - as what you expect/what.

Example:

preg_match("#\^[a-z0-9_-]{6,16}#i", $userinput_username);

- This would return FALSE if any other character is used in the username apart from: "abcdefghijklmnopqrstuvwxyz-_0123456789"

 

Sanitizing User Input for Mysql (mentioned as escaping the user input above) is preventing mysql injection (I would google that).

	$username = mysql_real_escape_string($_POST['uname']); 

 

Hashing of a password is most commonly done by using md5($password); - The result is what you store in the database, everytime you check or update this password, the new password/password to be checked with needs to be hashed too. Though md5() is actually insecure as there are hash-tables out there that can very quickly get any password.

This makes your user's password (which is very commonly used for most of their accounts etc and a lot of people have the same name for different accounts/sites etc) more secure, so if someone has gained access to your database (you must assume full access to everything the mysql user/pass used in your scipts has) then at least the usernames passwords are not compromised.

 

Your If Statements, When something generally doesnt work the way you expect, you need to find the reason or bug in your code. For small, simple projects this can be done with simple echo/exit/print_r statements. for ex;

	$numofrows = mysql_num_rows($results);
if ($numofrows == 0) {
exit("Do Password Change... Rows: ".$numofrows);
if ($npass == $cnpass) {

 

Same applies to the query you never run, although these errors are just inexperience imo, things you forget or don't notice, just try to follow your code through in your head to make sure it makes sense to you in a logical way.

 

Redirect/infinite loop problems can be traced by following the code in your head with a set of "default values" to use, like a username/password etc. eventually you will spot the loop, debugging is essential .

 

hope this helps

[Psycho]

Sanitizing User Input for Mysql (mentioned as escaping the user input above) is preventing mysql injection (I would google that).

 

Um, yeah, I "mentioned" it that way because that is the correct terminology. From the manual:

mysql_real_escape_string — Escapes special characters in a string for use in an SQL statement

[ChemicalBliss]

That is good to know .

No offense intended.

 

Sanitization is a term used in many fields of programming and even boyond, whereas escaping is specific term used for specific sanitization of certain specific programming languages .

 

hope this helps

[Mat_dennison]

thanks for both ur advice and help and its got it to work!

its been educational, always good to learn more when it comes to this.

thank you!