Login security check - your advice

[krystof78]

Hi there,

 

I would like to know what you think about these security checks for a login page leading to an admin area:

 

// START FORM PROCESSING
if (isset($_POST['submit'])) { // Form has been submitted.
$errors = array();

// perform validations on the form data
$required_fields = array('username', 'password');
$errors = array_merge($errors, check_required_fields($required_fields, $_POST));

$fields_with_lengths = array('username' => 30, 'password' => 30);
$errors = array_merge($errors, check_max_field_lengths($fields_with_lengths));

// clean up the form data before putting it in the database
$username = trim(mysql_prep($_POST['username']));
$password = trim(mysql_prep($_POST['password']));
$hashed_password = sha1($password);

// Database submission only proceeds if there were NO errors.
if (empty($errors)) {
	// Check database to see if username and the hashed password exists there.
	$query = "SELECT id, username ";
	$query .= "FROM users ";
	$query .= "WHERE username = '{$username}' ";
	$query .= "AND hashed_password = '{$hashed_password}' ";
	$query .= "LIMIT 1";
	$result_set = mysql_query($query);
	confirm_query($result_set);
	if (mysql_num_rows($result_set) == 1) {
		// username/password authenticated
		// and only 1 match
		$found_user = mysql_fetch_array($result_set);
		$_SESSION['user_id'] = $found_user['id'];
		$_SESSION['username'] = $found_user['username'];
		redirect_to("staff.php");
	} else {
		// username/password combo was not found in the database
		$message = "Username/password combination incorrect.<br/>
		Please make sure your caps lock key is off and try again.";
	}		
} else {
	// Errors occured
	if (count($errors) == 1) {
		$message = "There was 1 error in the form.";
	} else {
		$message = "There were " . count($errors) . " errors in the form.";
	}
}
}
// END FORM PROCESSING

 

Is there any security holes in this code? I am starting to get a bit paranoid after reading a few things about security issues so I'd be glad if you could help me on that... Thanks in advance for your input...

 

Cheers,

 

Krys

[ignace]

Why you never will beat hackers:

 

http://www.engadget.com/2010/03/09/1024-bit-rsa-encryption-cracked-by-carefully-starving-cpu-of-ele/

http://www.sektioneins.com/en/advisories/advisory-032009-piwik-cookie-unserialize-vulnerability/index.html

http://www.sektioneins.com/en/advisories/index.html

 

[premiso]

@Op, your code looks fine, and is probably as secure as you can make it. As long as mysql_prep escapes the string data, I think you are safe.

 

You are not secured from all attacks, such as session hi-jacking, which is a huge issue mainly on Shared Servers. But if you are not on a shared server, that should be secure.

 

The main issue is going to come from, not the validation, but the rest of your code. As long as you validate that the user is allowed and on your main pages that people can view are not doing elementary mistakes, such as including a url in include, or passing unknown data into an include call, you should be fine. Just make sure that you validate any data coming in from forms / get. If you do not do that, your site can more or less easily be exploited.

[krystof78]

Thanks premiso for your answer. Here is the function I use to escape the string data:

 

function mysql_prep( $value ) {
$magic_quotes_active = get_magic_quotes_gpc();
$new_enough_php = function_exists( "mysql_real_escape_string" ); // i.e. PHP >= v4.3.0
if ( $new_enough_php ) { //PHP v4.3.0 or higher
	// undo any magic quote effects so mysql_real_escape_string can do the work
	if ( $magic_quotes_active ) { $value = stripslashes( $value ); }
	$value = mysql_real_escape_string ( $value );
} else { // before PHP v4.3.0
	// if magic quotes aren't already on then add slashes manually
	if ( !$magic_quotes_active ) { $value = addslashes ( $value ); }
	// if magic quotes are active, then the slashes already exist
}
return $value;
}

 

I feel that it's doing the job required. What do you think about it?

 

Thanks again.

 

Cheers,

 

Krys