Login System - What should I fix?

[bagofmilk]

I'm still learning PHP, so bear with me...

I have a login system here, and I would like any input on what I need to update to make this more secure.

I've been reading a lot of information about how I need to prevent SQL injection and XSS attacks, but I have yet to see some real-world examples on how to accomplish that.

the "dbcfg.php" file is my database config file - it defines the DSN, DBUSER, and DBPASS.

<?php
    require_once ("dbcfg.php");

    //Retrieve the EMAIL and PASSWORD entries from the user via POST
    $email = filter_var($_POST["email"], FILTER_VALIDATE_EMAIL);
    $password = htmlspecialchars($_POST["password"]);


    //Create an empty ERRORS array
    $err = array();

    //If the EMAIL variable is null or empty then throw an error
    if (!isset($email) || strlen($email) == 0 || $email == null) {
        array_push($err, " - 'Email Address' cannot be empty.");
    }

    //If the PASSWORD variable is null or empty then throw an error
    if (!isset($password) || strlen($password) == 0 || $password == null) {
        array_push($err, " - 'Password' cannot be empty.");
    }

    //If the array has values, then return the errors back to the user
    if (sizeof($err) > 0) {
        echo implode("<br>", $err);
    }


    //Else - TRY to check the user's credentials with what is in the database
    else {

        try {
            $pdo = new PDO(DSN,DBUSER,DBPASS);  
            $sql = "SELECT ID, FNAME, LNAME, EMAIL, PWORD FROM users WHERE (EMAIL=:email AND IS_INACTIVE = 0) LIMIT 0, 1;";
            $stmt = $pdo->prepare($sql);        
            $stmt->execute([":email" => $email]);
            $results = $stmt->fetchAll(PDO::FETCH_ASSOC);        
            if (sizeof($results) > 0) {
                $obj = $results[0];
                if (password_verify($password, $obj["PWORD"])) {
                    session_start();
                    $_SESSION['user_id'] = $obj["ID"];
                    $_SESSION['user_firstname'] = $obj["FNAME"];
                    $_SESSION['user_lastname'] = $obj["LNAME"];
                    $_SESSION['user_email'] = $email;
                    echo "success";
                }
                else {
                    echo "Invalid username and/or password";
                }    
            }
            else {
                echo "Invalid username and/or password";
            }
        } catch (PDOException $e) {
            echo $e->getMessage();
            die("Could not connect to the database:" . $e->getMessage());
        }
    }
    
?>

 

Edited January 25, 2022 by bagofmilk

[ginerjm]

- one doesn't need to user limit 0,1 if only querying for one row.  Change to limit 1.

- you should verify that everything is performing properly instead of just assuming.  Do a test on the prepare, on the execute and the number of rows returned before trying to use the query results.  Good practice at all times.  Look at examples in the manual.

- Another good practice - put your session_start at the beginning of your script.  That way it's there and you don't have to worry about it.

- If you are only looking to get back 1 row no need to do a fetchall.  Change to fetch . - if($row = $stmt->fetch(PDO::FETCH_ASSOC))

- handle the error messages instead of just continuing with your script.

- save a function call by simply adding your messages to the error array - $arr[] = 'new message';

The rest seems to me to be secure, but I have yet to use the password verify (or password_hash) so I don't know if that is proper.

[bagofmilk]

@ginerjm

For error handling - would you suggest that I use "throw"

so:

....

//CHANGE TO THIS
if (sizeof($err) > 0) {
	// --- remove: echo implode("\n", $err);
	throw new Exception(implode("\n", $err));
}

//And

...
		}
		else {
			// --- remove: echo "Invalid username and/or password";
			throw new Exception("Invalid username and/or password.");
		}    
	}
	else {
		// --- remove: echo "Invalid username and/or password";
		throw new Exception("Invalid username and/or password.");
	}

 

[ginerjm]

How about simply testing the result and putting out an error message and then either quitting or working around it with your script?

if (!$row = $stmt->fetch())
{
    echo "No rows returned from query - try again";
    exit();
}
// continue on with the row of data
	

This works since (per the manual) the fetch will either return a result or false, and results are always a 'true' value.

Edited January 25, 2022 by ginerjm

[ginerjm]

You don't need to test assignments.  Mainly things that are outside of your control such as querying (done by the db server) or  a prepare that may not like your query statement, or a query itself that fails to run properly.  Of the fetch that may not find anything to fetch.  

[ginerjm]

Here's an example of how I would do it.  Can't promise it's error free but you can see how it would look:

session_start();
require_once ("dbcfg.php");
//Retrieve the EMAIL and PASSWORD entries from the user via POST
$email = filter_var($_POST["email"], FILTER_VALIDATE_EMAIL);
$password = htmlspecialchars($_POST["password"]);

//If the EMAIL variable is null or empty then throw an error
if (!isset($email) || strlen($email) == 0 || $email == null) 
	$err[] = "Email Address cannot be empty.<br>";

//If the PASSWORD variable is null or empty then throw an error
if (!isset($password) || strlen($password) == 0 || $password == null) 
	$err[] = "Password cannot be empty.<br>";

//If the array has values, then return the errors back to the user
if (isset($err))
{
	echo $err;
	exit();
}
//**********************
//Now check the user's credentials with what is in the database
$pdo = new PDO(DSN,DBUSER,DBPASS);  
$sql = "SELECT ID, FNAME, LNAME, EMAIL, PWORD FROM users 
		WHERE EMAIL=:email AND IS_INACTIVE = 0 LIMIT 1";
if (!$stmt = $pdo->prepare($sql))
{
	echo "Could not perform Prepare on query";
	exit();
}
if(!$stmt->execute([":email" => $email]))
{
	echo "Query failed to execute:<br>";  
	echo print_r($stmt->errorInfo()) . "<br>";
}

if(!$row = $stmt->fetch(PDO::FETCH_ASSOC)) 
{
	echo "No rows found matching email<br>";
	exit();
}
if (password_verify($password, $row["PWORD"])) // I assume this is correct?
{
	$_SESSION['user_id'] = $obj["ID"];
	$_SESSION['user_firstname'] = $obj["FNAME"];
	$_SESSION['user_lastname'] = $obj["LNAME"];
	$_SESSION['user_email'] = $email;
	echo "success";
}

Just noticed that you don't check that the incoming form was even received properly.  Need to check that POST was received to avoid un-set errors on the input gathering lines.  Add this:

if ($_SERVER['REQUEST_METHOD'] == 'POST')
{
	(do your input receiving and editing)
}
// now check for any error messages

 

Edited January 25, 2022 by ginerjm

[ginerjm]

oops - out of habit I wrote my fetch line using $row but you are using $obj.  Please switch.

And - I realize that I am using $err as an array but echo ing it incorrectly.  Try:

echo "<pre>",print_r($err,true),"</pre>";

 

Edited January 25, 2022 by ginerjm

[mac_gyver]

here's a laundry list of items, repeating some already posted above -

1. for a login page, you care - 1) that the current visitor is not already logged in, 2) that the trimmed email and password are not empty strings, and 3) that the email/password matches the email/password_verify() result.
2. you don't care if the email address is properly formatted (in the registration logic, you do care.) in fact, the current code testing $email, the result of the filter_var() statement, will incorrectly report that the email was empty, when it is not properly formatted.
3. the session_start goes near the top of the code in the initialization section, both for consistency and so that you can check if the current visitor is not already logged in before allowing access to the login form and the form processing code.
4. require_once is not a function. the () are unnecessary clutter. also, the _once version takes more processing since php must check the table of already required files.
5. post method form processing code should first detect if a post method form was submitted, then trim, and validate all the input data.
6. you should trim all the post data at once, keeping it in an array variable, then operate on elements of this array variable throughout the rest of the code.
7. htmlspecialchars is an output function. it is used when you output dynamic values in a html context.
8. except for unchecked check-boxes/radio-buttons, all other form field types will be set after the form has been submitted. do not use isset() on these type of fields. the trimmed value will either be an empty string or it will contain the submitted value to use.
9. when you add the error messages to the $err array, use the field name as the array index. this will let you perform any dependent validation steps only if there is not an existing error for the field and will let you test/output the errors adjacent to the corresponding field if you so desire.
10. use the simplest logic/syntax available. boolean tests exist so that you can directly test things in conditional statements.
11. you would test if the $err array is empty before using the submitted data and run the rest of the post method form processing code.
12. at the appropriate location in the html document, you would test if the $err is not empty and display its contents.
13. only catch database statement errors in your code for things that are user recoverable, such as inserting/updating duplicate or out of range values. in all other cases, simply let php catch and handle any database statement errors.
14. if you do catch database statement errors in your code, do not unconditionally echo the raw error information onto a web page.
15. when you make the PDO database connection - 1) set the character set to match your database tables, 2) set the error mode to exceptions, 3) set emulated prepared queries to false, and 4) set the default fetch mode to assoc.
16. if you use ? prepared query place-holders, it will reduce the amount of typing and typo mistakes.
17. the semi-colon ; on the end of sql query statements is not needed/used when executing them through php.
18. if someone successfully authenticates who they are, but their account is inactive, you would want to specifically tell them that, so, you want to query for their row of data regardless of the IS_INACTIVE value.
19. after doing the above item #18, the email column should be a unique index, so there's no point in having a LIMIT clause in this query.
20. as already posted, don't use fetchAll, then use more code/variables to get/test a single row. just use fetch.
21. you can directly test the result of the fetch statement, requiring no additional code.
22. when conditional failure logic is much shorter than the success logic, invert the conditional test and put the failure logic first. this will result in clearer code.
23. the login failure message should be added to the $err array, then displayed at the appropriate location in the html document.
24. the only user value that should be stored in a session variable is the user id. you would then query on each page request to get any other user data. this will allow the other values to be edited and they will take effect on the next page request.
25. after the end of all the validation/authentication logic, if there are no errors, redirect to the exact same url of the current page to cause a get request for that page. this will prevent the browser from trying to resubmit the form data if the user reloads that page or browses away from and back to the page. provide navigation link(s) to allow the user to go to any other page.

example code showing these points -

// initialization
session_start();

// is the visitor already logged in
if(isset($_SESSION['user_id']))
{
	// either redirect or output an error that you cannot access this page if already logged in
	die; // stop code execution
}

require "dbcfg.php";

//Create an empty ERRORS array
$err = [];
// array to hold a trimmed working copy of the form data. access elements in this array throughout the rest of the code
$post = [];

// post method form processing
if($_SERVER['REQUEST_METHOD'] == 'POST')
{
	// trim all the data at once (if any of the fields are arrays, use a recursive trim call-back function here, instead of php's trim function)
	$post = array_map('trim',$_POST);

	if($post['email'] === '')
	{
		$err['email'] = " - 'Email Address' cannot be empty.";
	}

	if($post['password'] === '')
	{
		$err['password'] = " - 'Password' cannot be empty.";
	}

	// if no errors, use the submitted data
	if(empty($err))
	{
		$sql = "SELECT ID, PWORD, IS_INACTIVE FROM users WHERE EMAIL=?";
		$stmt = $pdo->prepare($sql);
		$stmt->execute([ $post['email'] ]);
		if(!$row = $stmt->fetch())
		{
			// email didn't match
			$err['login'] = "Invalid email and/or password";
		} else {
			// verify hashed password
			if(!password_verify($post['password'], $row["PWORD"]))
			{
				// password didn't match
				$err['login'] = "Invalid email and/or password";
			} else {
				// successful login
				// at this point, you would test and setup messages for things like IS_INACTIVE...
				// only store the user id in a session variable. query on each page request to get any other user data.
				$_SESSION['user_id'] = $row["ID"];
			}
		}
	}
	// if no errors, success
	if(empty($err))
	{
		// redirect to the exact same url of this page to cause a get request - PRG Post, Redirect, Get.
		die(header("Refresh:0"));
		// note: if you want to display a one-time success message, store it in a session variable, then test, display, and clear that variable in the html document
	}
}


// the html document starts here...
?>


<?php
//If the array is not empty, then return the errors back to the user
if(!empty($err))
{
	echo implode("<br>", $err);
}
?>

 

[Strider64]

        if ($user && password_verify($this->password, $user['hashed_password'])) {
            unset($this->password, $user['hashed_password']);
            session_regenerate_id(); // prevent session fixation attacks
            static::$last_login = $_SESSION['last_login'] = time();
            $this->id = $_SESSION['id'] = $user['id'];
            header("Location: ../game.php");
            exit();
        }

I run a small blog for my own personal website and I have a trivia game that I wrote, so I don't do too much error checking as I figured it's not like a banking website. If I was error checking I would log the errors in a log of some kind and never tell the user (other than to retry) what they did wrong, why give a hacker more clues than the need? 🤔😀  Sorry about my rambling, my point of the is post is session)_regenerated_id() should be used to prevent session fixation attacks.