[RESOLVED] Simple auth script

[Megahertza]

Just a simple auth script, Any suggestion on improvents or problems found.

<?php
session_start();
$_SESSION['username'] = "NULL";
include 'includes/connection.php';
$result = mysql_query("SELECT * FROM users")
or die(mysql_error());

while($row = mysql_fetch_array($result)){
    If (($row[username] === $username) and ($row[password] === md5($password))) {
   		$_SESSION['username'] = $username;
	} else {
		header("Location: index.php?pid=bad_logon&usern=".$username);
	}
}
?>

[bbreslauer]

There's no reason to retrieve all of the users from the database; simply use a WHERE clause to only get the user whose username is equal to what was entered, and you'll no longer need to go through the while loop. In addition, you'll only need to retrieve the password.

md5 could be insecure, you might want to consider using sha1 as the password hashing function.

Headers could cause problems, as they're known to be picky in where they're placed. You'll probably want to exit() right after you place the header call, to make sure secret data doesn't get accidentally shown.

If you have the secret data coming right after the login is validated, you might want to switch the if statement around, so it checks to see if the passwords are not identical, and then send the header and exit. This way, you wouldn't actually need an else statement in there.

If you want to make sure the session is empty at the start (and this is only being run once per login, not on every script), then run session_destroy() right after session_start().

[Weedpacket]

And if only the username and password are being used in the rest of the page, then "SELECT username, password [WHERE...]" would be more efficient than selecting everything in the row.

[Megahertza]

Thanks for the hints bbreslauer

<?php
session_start();
session_destroy();
include 'includes/connection.php';
$result = mysql_query("SELECT * FROM users WHERE username = '$username'")
or die(mysql_error());

$row = mysql_fetch_array($result);
    If ($row[password] !== md5($password)) {
        header("Location: index.php?pid=bad_logon&usern=".$username);
		exit();        

	}
$_SESSION['username'] = $username;
?> 

Weedpacket, I'm somewhat new at mysql and i'm confused by it at the best of time, could you give me a example.

[bbreslauer]

If you're actually able to use $username as a valid variable, then that means you have register_globals() on. This can be a security risk, and if you can change that in the php.ini config file, you should turn it off. If not, you can still minimize your risk by explicitely defining the variable, using:

$username=$POST['username'];
or
$username=$GET['username'];

though you should probably be using POST.

What Weedpacket is talking about is, instead of saying

SELECT * FROM users

you could say

SELECT username,password FROM users

if you only need the username and password. If you only need the password, you can take the username, out of that.

[Weedpacket]

Megahertza wrote:

could you give me an example

Um, I did. Yeah, what bbreslauer said.

[Megahertza]

Originally mu $POST variables where retrieved on a parent file and this script was included. The other page doesn't use them if the page id is pid="auth".

<?php
$username = $_POST[user];
$password = $_POST[pass];
session_start();
session_destroy();
include 'includes/connection.php';
$result = mysql_query("SELECT username,password FROM users WHERE username = '$username'")
or die(mysql_error());

$row = mysql_fetch_array($result);
    If ($row[password] !== md5($password)) {
        header("Location: index.php?pid=bad_logon&usern=".$username);
		exit();        

	}
$_SESSION['username'] = $username;
?>

Now that I'm only asking for username and password colunms, can I ask mysql to
"SELECT username,password FROM users WHERE username = '$username', password = 'md5($password)'" to remove the need for a if statement. but i would need another if statement checking for a result correct?

[bbreslauer]

You could change to that select statement, but you would need to check if you retrieved any rows from the database, probably using mysql_num_rows().

[Megahertza]

Thanks guys, i think my script is the best it can really be. marking as resolved.