How Secure Is This Login?

[CrimsonDemon]

I'm rewriting some of my scripts using sessions and removing the need for global variables. However, since I've never done this before, I'm having some concerns about whether or not I'm actually doing this right. Here is my code:

This is the main page, where you actually type in your login information. I'm looking for a way to slightly simplify this:

<?php 
session_start();
header("Cache-control: private"); // IE 6 Fix.
require("header.php");
require("functions.php");

if (isset($_POST['user'])) {
	if (authenticate($_POST['user'],$_POST['pass'])) {	
		dispmenu();
	}
}
elseif (isset($_SESSION['username'])) {
	if (authenticate($_SESSION['username'],$_SESSION['password'])) {
		dispmenu();
	}
}
else {
	login();
}

?>

Here is the function authenticate. This is where the security comes into play, and where I hope most of the suggestions refer to:

function authenticate($usernew, $passnew) {
	global $myrow;
	$cryptpass=crypt($passnew,$usernew);
	if (isset($usernew) and isset($passnew)) {
		$result = mysql_query("SELECT * FROM members WHERE username = \"$usernew\"");
		$myrow = mysql_fetch_array($result);

	if ($myrow != NULL) {
		extract($myrow);
	}

	if ($usernew == $myrow['username'] and $cryptpass == $myrow['password']) {
		$_SESSION['username'] = $usernew; 
		$_SESSION['password'] = $passnew; 
		return 1;
	}
}	
if ($passnew !=  $myrow['password']) {
		echo("Password incorrect.");
}
}

Thanks in advance.

[Shrike]

crypt() only performs on the first 8 characters of a string IIRC. So if your passwords are longer than this, it may be worth going for md5() or similar.

[designguy79]

I would definitely convert and/or escape any quotes from all the _POST vars, especially the ones that are included directly in your SQL statement. You don't want a malicious user adding their own SQL code on the end. For example, what if they submitted the following as the $usernew?

; SELECT "blank" AS password WHERE username="admin"

I haven't tested that before, but definitely watch out whenever you use variables from a form in the SQL queries.

Here are some useful links regarding PHP securuty issues:

http://www.webmasterworld.com/forum88/924.htm

http://www.onlamp.com/pub/a/php/2003/03/20/php_security.html

[CrimsonDemon]

@: Very good idea. I've changed this and it looks much more secure because it doesn't use a salt and it much longer.

@: Thanks a lot for those links, they really made me think of some things that could use changing. About your suggestion, I don't quite understand it, as I'm still fairly new to PHP. Would you mind showing me an example using my script? I'd appreciate it a lot.

[designguy79]

I am afraid that I don't have a lot of time to explain it extremely in-depth. Here is the section that covers it from

http://www.onlamp.com/pub/a/php/2003/03/20/php_security.html

Escape characters in SQL statements
------
A common mistake is to use a variable value supplied by the user or the URL in an SQL query without escaping special characters. Consider the following fragment of code from a script designed to authenticate a username and password entered in a HTML form:

$query = "SELECT * FROM users WHERE username='" . $username . "' 
          AND password='" . $password . "'";

// the record exists function is defined elsewhere
if (record_exists($query)) {
	echo "Access granted";
} else {
	echo "Access denied";
}

This code would work when accessed using check.php?username=admin&password=x. However, if the code were accessed using check.php?username=admin&password=a%27+OR+1%3Di%271 (and if magic_quotes_gpc were disabled) then the password condition becomes Password='a' or 1='1' so that the admin user record would always be returned regardless of the password it contained.

This problem is partly avoided when the magic_quotes_gpc variable is on in the php.ini file, meaning that PHP will escape quotes in GET, POST, and cookie data using the \ character. However, magic_quotes_gpc is frequently disabled because it could make other code behave strangely. Given a line containing echo $username in the above code fragment, any occurrences of ' would be replaced by \'). Furthermore the magic_quotes_gpc variable does not protect against variable values obtained from sources such as database records or files which a malicious user may have already modified during normal program operation.

What to Look For
----
Search for the query functions for your database. For example, if you are using MySQL, search for usage of the mysql_db_query function. 

Possible Fixes or Improvements
----
Use the built-in addslashes function or a similar function to escape quotes and backslashes in SQL statements with backslashes.

Enabling magic_quotes_gpc may help, but don't rely upon it. (Enabling this setting and using addslashes will produce errors).

If you are using variables which you expect to contain numbers in your SQL statement, ensure that they really do contain numbers. You can use various built-in PHP functions including sprintf, ereg and is_long, to perform this check.

I am not saying that your code is vulenarble or not, but you will want to test it and be aware of these issues.

Hope that helps!

[sinus_]

here is something that i found out...

	if (!get_magic_quotes_gpc()) {
    $user = addslashes($_POST['user']);
	$pass = addslashes($_POST['pass']);
	$email = addslashes($_POST['email']);
	} else {
    $user = $_POST['user'];
	$pass = $_POST['pass'];
	$email = $_POST['email'];
	}