Login Page - Advice/Comments on security and speed?

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
User avatar
Trenchant
Forum Contributor
Posts: 291
Joined: Mon Nov 29, 2004 6:04 pm
Location: Web Dummy IS

Login Page - Advice/Comments on security and speed?

Post by Trenchant »

I've recently started a new online webgame. I would like to start with a very strong foundation(index, login, registration, and database).

Does anyone have any comments on this code? This is the login page.

pro.login.php

Code: Select all

<?php
/* --------------------------------------------------
					Login Page
	By: Luke Shaheen
	
	PRO. prefix - Process Only File
	
	This is the general portal for users to login to
	the site.
	
	Feature List:
	
	- "remember me"
	- MD5 Authentication
	- Detailed logging
	- Double form protection(car verification)
	
	This page will only respond to login attempts.  The actual form should be on the main page.
	
   --------------------------------------------------- */
include('inc.include.php');
//$layout->header('Login','');

// Log Login Function
function log_login($playername, $status, $reason, $system) {
	include('inc.include.php');
	// Connect to database.
	$system->db_connect();
	
	// INSERT report.
	$query = mysql_query("INSERT INTO `login_attempts` (player, ip, date, status, reason)
							VALUES ('$playername', '$ip', '$date', '$status', '$reason')");
}

// Check to see if a player was sent.
if (strip_tags($_POST[login_player])) {
	// Login request was sent(username)
	// Upload username as a variable and clean.
	$login_player = strip_tags($_POST[login_player]);
	$login_password = strip_tags($_POST['login_password']);
	echo "<center><b>".$login_player."</b></center><br>";
	
	
	//  Login Steps:
	//    1. Check if account exists
	//    2. Check if a password was posted
	//         - If it was then goto step 4
	//    3. Display users current car and ask for password
	//         - On SUBMIT send to step 2
	//    4. Validate password
	//         - If invalid:  Log event(IP, username, time)
	//    5. Verify account status(jailed or banned?)
	//    6. Check for "Remember Me"
	//    7. Detailed Logging of login
	//    8. Load the session
	//    9. Redirrect User
	
	// Pre-Login: Check if the account exists.
	$system->db_connect();
	
	// Count usernames matched.
	$pre_login_sql = mysql_query("SELECT * FROM `users` WHERE `player` = '$login_player' LIMIT 1");
	$pre_login = mysql_num_rows($pre_login_sql);
	$pre_login_sel = mysql_fetch_array($pre_login_sql);
	
	// Load the players ID number for later car identification
	$login_car_id = $pre_login_sel['cur_car'];
	
	// Close DB for security & speed
	mysql_close();
	
	// Compare results to 0
	if ($pre_login == '0') {
		// Username is NOT there.
		echo "The player you specified does NOT exist.";
		
		// Show footer and end page.
		$layout->footer();
		exit;
	}
	
	// Username does exist so continue.
	
	// Step #2 - Check if a password was posted
	if ($login_password) {
		// The user has posted a password so continue.
		
		// Step #4 - Validate Password
		// Connect to database.
		$system->db_connect();
		
		// Database is connected.  Select users info.
		$player_sql = mysql_query("SELECT * FROM `users` WHERE `player` = '$login_player' LIMIT 1");
		$player = mysql_fetch_array($player_sql);
		$player_db_password = $player['password'];
		$player_db_rank = $player['rank'];
		$player_db_id = $player['id'];
		// Close DB connection
		mysql_close();
		
		// Check if the passwords match.
		if (sha1($login_password) == $player_db_password) {
			// The passwords match!  Continue...
			
			// Step #5 - Verify account status(jailed[2] or banned[3]?)
			if ($player_db_rank == '2') {
				// The player's account is "jailed"
				log_login($login_player, 'failed', 'Account Jailed', $system);
				echo "<b>This account is 'Jailed' and cannot be used.</b><br><br>  The reason it was 'Jailed'(or locked/closed):";
		
			} else {
				// The player is legit!  Continue...
				
				// Step #6 - Check for "Remember Me"
				if ($_POST['remember_me']) {
					// The user does want their info to be remembered.
					// Upload cookie - to expire in 30 days
					setcookie('player_id', $player_db_id, time()+60*60*24*30, '/www', 'theextremegarage.com');
					setcookie('player_pass', $player_db_password, time()+60*60*24*30, '/www', 'theextremegarage.com');
					echo "'Remember Me' feature used.  You will be automatically logged in on this computer for the next 30 days.<br>";
				}
				
				// Step #7 - Detailed Logging of login
				log_login($login_player, 'Success', 'none', $system);
					
				// Step #8 - Load the session
				$_SESSION['player'] = $login_player;
				$_SESSION['player_id'] = $player_db_id;
				$_SESSION['rank'] = $player_db_rank;
					
				// Step #9 - Redirect the user and report succes!
				echo "You have been logged in as, ".$login_player."(PID: ".$player_db_id.")<br>";
				echo "You will be redirected to your garage in 3 seconds.  If you don't wish to wait please <a href='./game/?welcome'>click here</a>";
				
				
			}
		} else {
			// The passwords don't match.  Tell the user. (remember to log attempt.
			echo "The password you entered was incorrect.  Attempt logged.";
			log_login($login_player, 'failed', 'Bad Password', $system);
		}
		
	} else { // No login password was sent.  Show car and ask for password.
	
		// Step #3 - Display users car.
		// load car as an image
		$car_img = display_car($login_car_id, $system);
		
		// Display car image
		echo "<center><img src='".$car_img."' alt='Player Car'></center><br>";
		
		// Check car warning.
		echo "Please verify that the above car is yours.  If it is continue by entering your password below.<br><br>
				<center>
				<form action='pro.login.php?LOGIN2' method='post'>
				<b>Player:</b> ".$login_player."<br>
				<b>Password:</b> <input type='password' size='10' name='login_password'><br>
				<input type='hidden' name='login_player' value='".$login_player."' />
				<input type='hidden' name='remember_me' value='".$_POST['rememberme']."'>
				<input type='submit' value='Login'></form></center><br>";
	
	}
	
} // End of loop (if POST[login_username])
	

//$layout->footer();
?>
Here is a demo of the system:
http://dev.theextremegarage.com/
Player: demo
Password: demo

Features yet to make:
- Cookie recognition for returning users
- automatic redirrect after login(what would you suggest here? Normally I use JS, I'm thinking header())
- Possible conversion to Feyd's Sha256

Thanks,
Luke
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post by Maugrim_The_Reaper »

You seem to use two identical include() calls - include('inc.include.php'). One in the file body, the second in the log_login() function. strip_tags() is the wrong way of cleaning user input. You should check the data meets the required criteria, and insert it into a new array (maybe call it $clean so its easy to remember). All user input must also be escaped prior to use in SQL queries.

Example:

Code: Select all

<?php

$clean = array();

// assume login_player must be set, not empty, and contain only alphanumeric characters [a-zA-z0-9]
if(isset($_POST['login_player']) && !empty($_POST['login_player']) && ctype_alnum($_POST['login_player']))
{
	$clean['login_player'] = $_POST['login_player'];
}
else
{
	echo 'Your submitted username is invalid, please return to login page and try again.';
	exit();
}

$sql = array();

$sql['login_player'] = mysql_real_escape_string($clean['login_player']);

// do something with mysql_query using the $sql values (which are escaped) not $clean.

// if wanting to print to screen such...

$html = array();
$html['login_player'] = htmlentities($clean['login_player'], ENT_QUOTES, 'UTF-8');

echo 'The user name you submitted is: ', $html['login_player'];

?>
Other points:

- use quotes around array keys always
- filter ALL user data input
- Anything which fails input filtering is invalid and should be discarded - it's the user's fault, don't try to fix it, make them fix it.
- escape ALL user data for SQL queries
- use $clean/$sql to separate the two if it helps organise them more effectively
- escape all user variables for HTML output
- Don't confuse the two escaping methods - one is for SQL, one for HTML. These are different contexts.
- Be careful about storing passwords to cookies - they persist so the password hash is accessible conceivably by anyone with every request or even physically at the PC. It's a hash though, so not quite as bad as using a plaintext password (happens).

Rest of the code looks ok - you should if possible read up on security practice in PHP, it'll fill in the gaps between my short comments.

Feyd's sha256 class works fine for me - if you have control of the actual server to be used for this you could alternatively utilise the mcrypt library, or the PECL hash library for PHP. Both offer sha256 functions that are a bit faster than a PHP implementation. Feyd's class however will definitely work anywhere, without needing any such librarys.

Definitely use header() for redirects unless you make JS a requirement for your app. The typical redirect is (using a full URL if at all possible rather than a partial):

Code: Select all

$uri = 'http://forums.devnetwork.net';
header('Location: ' . $uri);
User avatar
Trenchant
Forum Contributor
Posts: 291
Joined: Mon Nov 29, 2004 6:04 pm
Location: Web Dummy IS

Post by Trenchant »

Thanks for all the information.

One question though. Is the $HTML part required? Earlier all entries are rejected unless they are only numbers and letters.
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post by Maugrim_The_Reaper »

In a close knit 1-page procedural script?...no. But in practice you should utilise these security practices consistently - i.e. everywhere and all the time. As an application gets more complex - you may have one script handling input, and a separate one handling output or SQL. A good practice therefore is Security in Depth (or Defense in Depth) where you put in place layers of security even if one is apprently not required. The idea is that if one line of defence fails (maybe someone introduces a bug in your filtering logic and suddenly username is not properly cleaned) you have redundant backup lines which reduce the overall risk of any single failure compromising the entire system.
User avatar
Trenchant
Forum Contributor
Posts: 291
Joined: Mon Nov 29, 2004 6:04 pm
Location: Web Dummy IS

Post by Trenchant »

Maugrim_The_Reaper wrote:In a close knit 1-page procedural script?...no. But in practice you should utilise these security practices consistently - i.e. everywhere and all the time. As an application gets more complex - you may have one script handling input, and a separate one handling output or SQL. A good practice therefore is Security in Depth (or Defense in Depth) where you put in place layers of security even if one is apprently not required. The idea is that if one line of defence fails (maybe someone introduces a bug in your filtering logic and suddenly username is not properly cleaned) you have redundant backup lines which reduce the overall risk of any single failure compromising the entire system.
Good point :lol:

I've added a bit of additional security to the script. I made a function that checks a users cookie against the database for an automatic login. I've also added padding for cookie logins so the passwords sent are more secure.
Post Reply