Password_verify / PDO - Login Form Handler issues

[Skorpio]

I have been working on a login form, I have completed the registration side but the login form is proving to be fighting back.  I have just jumped into the world of PDO and only recently PHP in a serious way.

 

I have been trying to use the password_verify(); function but I have spent so long on it now trying to get it working I have made it more difficult than it should be and probably is.

 

I would be grateful if someone could take a look at my code and just tell me what I am doing wrong.  I have tested it with the username and password hard coded in and it returns an array however if I comment out the hard coded username and password I get an empty array.

 

I dare say that someone will see the issue straight away but I cannot get my head round it.

<?php
	session_start();
	error_reporting(0);
	require '../php_inc/connection/connect.php';
	require_once '../php_inc/functions.php';
	
	
	
	$error = ''; // all error messages will use this variable
	$msg = 'Please fill in both fields and answer the captcha, they are all required to log in.';
	
	if(isset($_POST['submitted'])){
		$dbuname = 'dashby'; // As if check with DB - If I comment these 2 out and try to get data from DB I get empty array
		$hashed = '$2y$12$7hcyfm7UjboYGaNLF7vK1.qroo3YkvhKAR8EfxG1byEMkNB0oSQgi'; // As if check with DB - same password
	
		require 'Captcha.php';	
		$username = escape_in($_POST['username']); // Username
		$captcha = escape_in($_POST['captchaResult']); //Captcha
		$unhashed = escape_in($_POST['password']); //Password b4 hashing takes place
		
		//$submittedPassword = password_hash($unhashed, PASSWORD_DEFAULT, ['cost' => 12]);
		
				// connect to the database so the checks can be done.
				if($pdo){
					$stmt = $pdo->prepare("select * from users where username = :username && password = :password");
					$stmt->bindParam(":username", $username);
					$stmt->bindParam(":password", $unhashed); // If $hashed is the variable I get an array returned, as $unhashed I get an empty array
					echo '<pre>';
					if($stmt->execute()){
						$rows = $stmt->fetchAll(PDO::FETCH_ASSOC);
						print_r($rows);
					}
					
				}
				echo '</pre>';
		
		if($total == $getCaptchaResults){ 
			//Capcha OK
			if(password_verify($unhashed, $hashed)){
				//$msg = '';
				//$error .= 'Password match';
				if($username == $dbuname){
					//$msg = '';
					//$error .= 'Captcha, username and password ok'; // working to this point
					$_SESSION['username'];
					//header('Location: welcomelogged.php');
				} else {
					$msg = '';
					$error .= 'Denied wrong username and/or password';
				}
				
			} else {
				$msg = '';
				$error .= 'Denied wrong password and/or username';
			}
			
		} else { 
			if(($total != $getCaptchaResults)){
				$msg = '';
				$error .= 'Captcha Wrong'; 
			}
		}
		
	}// post submitted brace
	

?>

The if statements all work bar the password_verify when I comment out the hard coded variables out, directly under if(isset($_POST['submitted'])) {}

 

I would be grateful if someone could steer me in the right direction.

 

Thanks in advance.

Edited March 29, 2016 by Skorpio

[Jacques1]

The code has multiple problems.

 

First off, you must not use SQL-escaping together with prepared statements. The whole point of a statement is that its input gets treated as raw data and not as SQL, so any backslashes you've added through your escape functions will end up as literal characters (e. g. “O\'Reilly”).

 

You're also searching your database for the plaintext password, but of course that will never be found when you've stored hashes (which you should). You need to search for the username only, retrieve the hash and then verify the password against the hash using password_verify().

[Skorpio]

Thanks for the quick response. So can I take it that this is starting to look better.  I was under the wrong assumption that for POST data you were still meant to take precautions by escaping the data, i.e. script tags etc.

<?php
	session_start();
	//error_reporting(0);
	require '../php_inc/connection/connect.php';
	require_once '../php_inc/functions.php';
	
	
	$hashed = '';
	$error = ''; // all error messages will use this variable
	$msg = 'Please fill in both fields and answer the captcha, they are all required to log in.';
	
	if(isset($_POST['submitted'])){
		//$dbuname = 'dashby'; // As if check with DB - If I comment these 2 out and try to get data from DB I get empty array
		//$hashed = '$2y$12$7hcyfm7UjboYGaNLF7vK1.qroo3YkvhKAR8EfxG1byEMkNB0oSQgi'; // As if check with DB - same password
	
		require 'Captcha.php';	
		$username = $_POST['username']; // Username
		$captcha = $_POST['captchaResult']; //Captcha
		$unhashed = $_POST['password']; //Password b4 hashing takes place
		
		//$submittedPassword = password_hash($unhashed, PASSWORD_DEFAULT, ['cost' => 12]);
		
				// connect to the database so the checks can be done.
				if($pdo){
					$stmt = $pdo->prepare("select username, password from users where username = :username");
					$stmt->bindParam(":username", $username);
					
					echo '<pre>';
					if($stmt->execute()){
						$rows = $stmt->fetchAll(PDO::FETCH_ASSOC);
						print_r($rows);
					}
					
				}
				echo '</pre>';
				
				if(password_verify($unhashed, $hashed)){
					$msg .= 'Password match';
					$error .= '';
				} else {
					$msg .= 'Password don\'t match';
					$error .= '';
				}

// other ifs commented out until I get this functioning.

[Jacques1]

You do have to HTML-escape outgoing data, but this happens in the rendering part and has nothing to do with SQL queries. When you perform database operations, you need the original, unaltered data.

[Skorpio]

Right, thanks. So am I going in the right direction with the code above?  I am getting the array results from the database now thanks to you, I was going round in circles before.  I am just now trying to finish off the PDO so that I can actually do something with the data.

 

Forgive the daft question but outgoing as from the database you mean?

 

I am currently getting the following output, image, yet I am getting the else message from my if statement.

Edited March 29, 2016 by Skorpio

[requinix]

You aren't setting $hashed. Get the value from $rows, or do a bindValue+fetch instead of a fetchAll.

[Skorpio]

I have changed to fetch rather than fetchAll as only want 1 result. As for the $rows and bindValue I am trying to work that out.  I have initialized hashed only because I was getting error but can I take it that this is why it is triggering the else rather than the if?

 

Thanks

Edited March 29, 2016 by Skorpio

[Skorpio]

UPDATE: I had left the opening pre tag in.

 

Ok, I have the following code, it works as far as logging in with the correct details, username and password however if the incorrect details are put in I do not get the else message now.

if($pdo){
     $stmt = $pdo->prepare("select username, password from users where username = :username");
     $stmt->bindValue(":username", $username);
     echo '<pre>';
     $stmt->execute();
     $results = $stmt->fetch(PDO::FETCH_ASSOC);
     if(count($results) > 0 && password_verify($unhashed, $results['password'])){
	$_SESSION['username'] = $results['username'];
	header('location: welcomelogged.php');
	exit;
     }else{
	$msg = '';
	$error .= 'Passwords do not match';//Not being triggered at all
     }

On incorrect details being submitted the form is stretched out and all over the place.  I cannot identify what I am missing.

Edited March 29, 2016 by Skorpio

[Jacques1]

Neither can we, because the code stops right before the relevant parts, and “the form is stretched” is a somewhat vague error description.

 

All I can tell you is that if the user doesn't exist at all, $results is false, counts($results) is 1, and 1 interpreted as a boolean is true. This can (should!) lead to a couple of PHP error messages when you try to do a password comparison with a nonexistent password hash.

 

Anyway, this is the perfect opportunity for you to learn debugging, one of the most important skills of all. Start analyzing your code with var_dump(). This will tell you exactly which code path is executed and which values a particular variable has.

[Skorpio]

Hi, as I said in the previous update as far as the form stretching, I had left the opening pre tag in when I was working with the array. That's sorted. I will start with the var_dump as you suggest.

Thanks

[Skorpio]

From doing the var_dump it seems that despite thinking I had solved the issue I am back to square 1.  I just have the one if else in place at the moment

if($pdo){
	$stmt->bindValue(":username", $username);
        $stmt->execute();
        $results = $stmt->fetch(PDO::FETCH_ASSOC);
        if(count($results) > 0 && password_verify($unhashed, $results['password'])){
	   var_dump($results);
	   echo 'Success';
        } else {
	   var_dump($results);
	   echo 'Try Again';
        }
}//if $pdo

On correct information being entered I get the expected output - success screenshot

 

However when entering the incorrect information I still get output - Failed screenshot

 

Finding a result when incorrect details are entered has taken me back to the start.

[Skorpio]

Tried to update last post, unable.

 

Ok so further testing with totally incorrect login details returns bool false which is correct, screenshot

 

So the issue is with entering any existing username returns a result as per failed screenshot in last post.

[Skorpio]

Surely it cannot be as easy as to force a return of false?

if($pdo){
	$stmt = $pdo->prepare("select username, password, active from users where username = :username");
	$stmt->bindValue(":username", $username);
	$stmt->execute();
	$results = $stmt->fetch(PDO::FETCH_ASSOC);
	if(count($results) > 0 && password_verify($unhashed, $results['password'])){
		var_dump($results);
		echo 'Success';
	} else {
		//var_dump($results);
		echo 'Try Again';
		return false; // Is this the correct way or ?
		
	}
}//if $pdo

[Jacques1]

According to your first post, the code is part of the main script. If you return in the middle of the script, you'll get nothing but a blank page.

 

I recommend you put the above code into a separate function like check_credentials() which returns true or false depending on whether the credentials are correct. Then the main script can either log the user in or display an error message.

 

And again: Don't use count() on a variable that can be false. This is appearently a leftover from your previous fetchAll(). Now just check $results && password_verify(...).

[Skorpio]

Starting with your last point, I have removed count and am checking 

 

 

$results && password_verify(...).

I was planning on the function but wanted to get the code working here first. As the prepared statement is only checking for the username it is returning true even if the password does not match.  

 

As I said in my original post I am just going round in circles.  If I place the above code in a function

function check_credentials ($pdo, $username, $unhashed){
    $stmt = $pdo->prepare("select username, password, active from users where username = :username");
	$stmt->bindValue(":username", $username);
	$stmt->execute();
	$results = $stmt->fetch(PDO::FETCH_ASSOC);
	
	if($results == 1 && password_verify($unhashed, $results['password'])){
		$msg = '';
		$error .= 'OK';
	} else {
		$msg = '';
		$error .= 'No entry';
		var_dump($results);
	}
         return false;
}

That will still return a result if the username is entered correctly though, won't it?

Edited March 29, 2016 by Skorpio

[Jacques1]

You need to understand the logic, not assemble random code snippets.

 

Right now, the function always returns false. That's obviously not very useful. Instead, you want it to either return true or false, depending on whether both the username and the password are correct.

 

The easiest way is to have a boolean flag $is_valid which is initialized to false and then switch to true if the condition of the innermost if statement is fulfulled. Then at the end of the function you can simply return the flag.

$is_valid = false;

// a lot of checks
if (...)
{
    if (...)
    {
        // if everything is fine, switch the value to true
        $is_valid = true;
    }
}

return $is_valid;

[Skorpio]

These are the current checks I have, everything running on xampp but when testing on live server throws a 500 after submission, does not show errors.

 

So I have this, 

if($pdo){
		$stmt = $pdo->prepare("select username, password, active from users where username = :username");
		$stmt->bindValue(":username", $username);
		$stmt->execute();
		$user = $stmt->fetch(PDO::FETCH_ASSOC);
		
		if($user === false){
			$msg = '';
			$error .= 'Incorrect Username/Password combination';
		} else {
			$validPassword = password_verify($unhashed, $user['password']);
			
			if($validPassword){
				if($user['active'] == 1){
					if($total == $getCaptchaResults){
						$msg = '';
						$error .= 'Everything working so far'; // any more ifs - when live response from $_POST[] is HTTP ERROR 500
					} else {
						$msg = '';
						$error .= 'Captcha incorrect!';
					}
				} else {
						$msg = '';
						$error .= 'You cannot log in until you click the link in the registration email we sent you.';
				}//active
			} else {
				$msg = '';
				$error .= 'Incorrect Username/Password combination';
			}
			
		}
		
	}
	
}

I will look at is_valid and see what happens.

 

Thanks

[Skorpio]

I think I have found the reason for the 500 error if nothing else, my host only supports up to php 5.4 so the verify and hash are not supported but that's another issue. but the above code is running but I am still testing the is_valid you suggested.

[Strider64]

I think I have found the reason for the 500 error if nothing else, my host only supports up to php 5.4 so the verify and hash are not supported but that's another issue. but the above code is running but I am still testing the is_valid you suggested.

You can download the password_hash/password_verify for PHP 5.3.7 and PHP 5.4 https://github.com/ircmaxell/password_compat

Edited March 29, 2016 by Strider64

[Strider64]

I personally would use true instead of false for I find it easier to comprehend and it might be helpful just to have a security access level that grants permissions to the user to do certain things based on their security level.

 

For example this is my login script for my website(s):

        $db = DB::getInstance();
        $pdo = $db->getConnection();
        /* Setup the Query for reading in login data from database table */
        $this->query = 'SELECT id, username, password, security_level, first_name, last_name, email, home_phone, cell_phone, gender, birthday FROM users WHERE username=:username';

        try {
            $this->stmt = $pdo->prepare($this->query); // Prepare the query:
            $this->stmt->execute([':username' => $data['username']]); // Execute the query with the supplied user's parameter(s):
        } catch (Exception $ex) {
            die("Failed to run query: " . $ex->getMessage()); // Do Not Use in Production Website - Log error or email error to admin:
        }

        $this->stmt->setFetchMode(PDO::FETCH_OBJ);
        $this->user = $this->stmt->fetch();

        if ($this->user) {
            $this->loginStatus = password_verify($data['password'], $this->user->password); // Check the user's entry to the stored password:
            unset($data['password'], $this->user->password); // Password(s) not needed then unset the password(s)!:
        } else {
            return FALSE;
        }

        if ($this->loginStatus) {
            $_SESSION['user'] = $this->user; // Set the session variable of user:
            return TRUE;
        } else {
            return FALSE;
        }

If the user's credentials check out then I simple put $_SESSION['user'] in my configuration file like this:

/* Use $user for sessions variable */
$user = isset($_SESSION['user']) ? $_SESSION['user'] : NULL;

Then I can simple do this I want to grant access to a certain security level like this:

if ( $user && $user->security_level === 'member") {
  /* Write code here for user with the security level here */
}

The possibilities are endless

 

another example:

if ( $user && $user_security_level !== 'member' ) {
  header('Location: index.php'); // Sorry none members not allowed:
  exit();
}

Edited March 29, 2016 by Strider64

[Skorpio]

Regarding 

 

 

You can download the password_hash/password_verify for PHP 5.3.7 and PHP 5.4 https://github.com/i...password_compat

I can run up to version 7 on the live server through cpanel however the host only supports up to 5.4 which obviously raises issues such as not renewing with them unless they update to at least 5.5 but that's another issue.

 

The levels for access is something I was thinking about for the future but I need to get my head around PDO and Php in general without being overly ambitious but you have given me a lot to think about.

 

Thanks for the suggestions and the help.