Creating a secure PHP login

A-GB

I have created what I believe to be a secure PHP login form. I won't paste all the code here but basically it runs all post data though the clean function shown below.

function clean($str) {
$str = @trim($str);
if(get_magic_quotes_gpc()) {
$str = stripslashes($str);
}
return mysql_real_escape_string($str);
}

The following SQL query is used the check if the login data are correct...

$qry="SELECT * FROM members WHERE login='$login' AND passwd='".md5($_POST['password'])."'";

If the data is valid, a session is created...

$_SESSION['MID'] = $member['mid'];

For all the secure member only pages, the code checks the see if the session if valid

if(!isset($_SESSION['SESS_MEMBER_ID']) || (trim($_SESSION['SESS_MEMBER_ID']) == '')) {

Does the above script look ok and secure? I mean have I left any vulnerabilities anywhere?

Secondly, is setting a session and checking it a good source of authentication? I have noticed several other php scripts such as messages board store the session in the database along with the username and IP address. Why do they do this? I assume the only reason for this is to prevent sessions be hijacked? If so, the IP check would prevent this?

Bjom

you could store the session in the DB instead of the default storage in temp folder on disk.

you could also improve security by switching to mysqli and prepared statements.

bradgrafelman

A-GB;10981111 wrote:

function clean($str) {
$str = @trim($str);
if(get_magic_quotes_gpc()) {
$str = stripslashes($str);
}
return mysql_real_escape_string($str);
}

Few comments:

Why do you have the '@' error suppressor in there? IMHO, this should never be used (since there is no good reason to do so).
Why are you [man]trim/man'ing all data? Can you honestly say that you will never want to allow any whitespace on any piece of data anywhere in your database? If so, why?
Note that your "clean" function should actually be called "clean_string" or something similar, since it isn't generic and instead is specific only to strings. Numeric (or any non-string) data shouldn't be sanitized with [man]mysql_real_escape_string/man (hint: look at the last word in the name of that function 😉).

A-GB;10981111 wrote:
The following SQL query is used the check if the login data are correct...
$qry="SELECT * FROM members WHERE login='$login' AND passwd='".md5($_POST['password'])."'";

Few comments/questions:

How is $login defined?
Are you aware that MD5 is considered to be fairly weak (e.g. quite easily broken) nowadays? Consider using at least SHA1, and also note that it's wise to use a salt when hashing passwords as well.
Don't do a 'SELECT ' - instead, only SELECT the columns from which you actually need data (even if that's all columns in the table, I still consider it 'better' to name all of them rather than doing a 'SELECT ').

A-GB;10981111 wrote:
If the data is valid, a session is created...
$_SESSION['MID'] = $member['mid'];
For all the secure member only pages, the code checks the see if the session if valid
if(!isset($_SESSION['SESS_MEMBER_ID']) || (trim($_SESSION['SESS_MEMBER_ID']) == '')) {

Er.. is that a typo? You set $SESSION['MID'], and yet you're checking for $SESSION['SESS_MEMBER_ID']?

A-GB;10981111 wrote:
Does the above script look ok and secure? I mean have I left any vulnerabilities anywhere?

Are you doing all of this over a secure (e.g. HTTPS) connection?

A-GB;10981111 wrote:
Secondly, is setting a session and checking it a good source of authentication?

Seems good enough to me; session data can only be modified by someone who has access to the server. Otherwise, the only things you have to worry about are session fixation and/or session hijacking attacks.

A-GB;10981111 wrote:
I have noticed several other php scripts such as messages board store the session in the database along with the username and IP address. Why do they do this? I assume the only reason for this is to prevent sessions be hijacked? If so, the IP check would prevent this?

Yes, some applications are poorly written and attempt to prevent session fixation/hijacking by checking to see if the user's IP address ever changes from one request to another. Some reasons this is 'poorly written' IMHO would be:

Hundreds or thousands of users could share the same IP address.
One user could utilize multiple IP addresses (think: smartphone, home PC, work PC, etc.).
One user's device could utilize multiple IP address across consecutive requests (think AOL proxy servers, where every page load might come from a different proxy server than what was last used, meaning your IP would be different each time).
An IP address is meant to identify a specific networked device, not a "user."

Bjom;10981117 wrote:
you could also improve security by switching to mysqli and prepared statements.

How does using MySQLi and prepared statements "improve" security at all? (Hint: It doesn't.)

laserlight

bradgrafelman wrote:
How does using MySQLi and prepared statements "improve" security at all? (Hint: It doesn't.)

If the relevant code is already secure, then the use of prepared statements would not improve security. However, whether or not the relevant code is already secure, prepared statements increase the margin of security by separating the data involved from the statement logic, assuming that they are not merely emulated.

A-GB

Thanks all!

Can you honestly say that you will never want to allow any whitespace on any piece of data anywhere in your database?
Yes

How is $login defined?
$login is the email address/username and therefore is POST data.

Consider using at least SHA1, and also note that it's wise to use a salt when hashing passwords as well. Yes I am aware of MD5 issues, I will now use SHA1 instead. Plus, I am using a site-wide salt. Maybe to improve the security I could create a pre-user salt in MySQL?

Don't do a 'SELECT ' - instead, only SELECT the columns from which you actually need data (even if that's all columns in the table, I still consider it 'better' to name all of them rather than doing a 'SELECT ').
I will update my code with this.

Er.. is that a typo? You set $SESSION['MID'], and yet you're checking for $SESSION['SESS_MEMBER_ID']?
yes, Typo erro, Fixed now 🙂

Are you doing all of this over a secure (e.g. HTTPS) connection?
When we go live I will add HTTPS but until then no.

How does using MySQLi and prepared statements "improve" security at all? (Hint: It doesn't.)
Good because I don't understand MySQLi 😛

Are you suggesting I stick with good old setting SESSION as authentication?

A-GB

One other thing... What is the best way is filtering POST data before adding it into MySQL?

function clean($str) { 
$str = @trim($str); 
if(get_magic_quotes_gpc()) { 
$str = stripslashes($str); 
} 
return mysql_real_escape_string($str); 
}

Does this do the job?

As you pointed out this is for strings only, and I will need to add numeric POST data into MySQL at some point. How would I go about doing that?

A-GB

FYI... Here is some later code which checks the POST data.

// Validate First name
if ($_POST['firstname'] != "") {
	$san_firstname = filter_var($_POST['firstname'], FILTER_SANITIZE_STRING);  

	if ($san_firstname == "") {  

		$error_array[] = 'The first name is not valid.';
		$error_flag = true;
	}  

} else {
    $error_array[] = 'The first name is not valid.';
	$error_flag = true;
}

Does this look ok?