Another login script security crit

[EricTRocks]

I just need someone to check if this code is valid and secure. It works great so far. I don't have any errors. Let me know.

This is at the top of my index.php file:



<?php

session_start();

require "db_connect.php";
mysql_select_db("users");

if(isset($_POST['authenticate']))
{

// Variables
$xsidbuser = mysql_escape_string($_POST['xsidbuser']);
$xsidbpass = md5(mysql_escape_string($_POST['xsidbpass']));

// Query Database for User / Pass match
$auth_query = mysql_query("SELECT * FROM users WHERE uname='$xsidbuser' AND upass='$xsidbpass'");
$auth_count = mysql_num_rows($auth_query);

if($auth_count > 0)
{
session_start();
$_SESSION['logged_in'] = '1';
$_SESSION['xsidbuser'] = $xsidbuser;
unset($_SESSION['login_error']);

header ("Location: index.php");
exit();
}
else
{
$loginerror = "<p id='red'>Your login was invalid.</p>";
$_SESSION['logged_in'] = '0';
}

}

if(isset($_GET['logout']))
{
session_start();
session_unset();
session_destroy();

header ("Location: index.php");
exit();
}

?>




This is in an area for the login box:




<?php

if (isset($_SESSION['logged_in']) && $_SESSION['logged_in'] == '1')
{
// XSI DB User Variable
$xsidbuser = $_SESSION['xsidbuser'];

// Query for number of posted materials
$user_materials = mysql_num_rows(mysql_query("SELECT uname FROM users WHERE uname='$xsidbuser'"));

echo "<p id='logintext'><h1>Welcome: $xsidbuser</h1></p>\r\n<p>Posted Materials: $user_materials</p>\r\n\r\n<p><a href='" . $_SERVER['PHP_SELF'] . "?logout=1'>Logout</a> | View Profile</p>";
}
else
{
echo "$loginerror\r\n\r\n";

?>


<form action="<?php echo $_SERVER['PHP_SELF']; ?>" enctype="multipart/form-data" method="post">
<fieldset>

<label for="xsidbuser">USER:</label>
<input name="xsidbuser" type="text" size="9" />

<label for="xsidbpass">PASS:</label>
<input name="xsidbpass" type="password" size="9" />

<input type="hidden" name="authenticate" />

<label for="authenticate"><!-- --></label>
<input id="loginbutton" type="image" src="images/button_login.gif" name="submit" />

<p><a href='#'>Register Now!</a></p>

</fieldset>
</form>


<?php
}
?>




Any and all help is appreciated!

Thanks,
Eric T.

[EricTRocks]

Hey,

All I want is a quick check for security holes, even if you say looks good to me.

I appreciate it,
Eric T.

[Horizon88]

Just glancing through it, it looks okay.

[EricTRocks]

Finally a response. That took a while. But I really appreciate the look through. Makes me feel as if I do know something about PHP. :)

Eric T

[cgraz]

This isn't related to security, and probably not all that big of a deal, but it is in the critique forum :D

instead of setting logged in to '1' (the single quotes treat it as a string), why not just set it to true/false? Note, you could use 1/0 respectively, just without the quotes. Then, you could check like this:if( $_SESSION['logged_in'] )
{
// Already Logged In
}
else
{
// Not Already Logged In
}Also, I always like to use session_regenerate_id() (http://www.php.net/session_regenerate_id) whenever escalating user permissions in order to avoid session fixation.

[EricTRocks]

Thanks,

Will definitely take that into consideration. Always appreciate the crit... it makes me better at the coding. :)

Talk soon,
Eric T

[MarkR]

You should only call session_start once per page. On the top page, you're calling it twice if somebody logs in correctly.

You probably want to put session_start in a shared function and use that everywhere, and set the relevant session parameters first:

function StartupSession()
{
ini_set('session.use_only_cookies', 1);
// any other ini_set you need here
session_start();
}


There are probably a number of other session parameters you might want to set - cookie name, session handler, directory etc.

If you use sessions, the ini settings need to be the same on all pages which share a session - so it's vital that they're set the same.

session.use_only_cookies is a very good thing and I highly recommend it.

Mark

[NogDog]

Logically, this seems backwards, though it probably won't matter as long as you are consistent:

$xsidbpass = md5(mysql_escape_string($_POST['xsidbpass']));

You are hashing the password string after escaping it for use in the SQL. Theoretically, if the md5() were to output something that should not be used in SQL, then you could still have a query error. In reality, since md5() returns a 32-character hexadecimal number, it's not going to matter as far as SQL injection problems go. I just thought I'd point it out here for any future code, as logically the mysql_real_escape_string() should normally be the last thing you do to a string before outputting it to MySQL. Also, if the password has any characters in it that are escaped by mysql_real_escape_string, then there will be a difference in the hash depending on the sequence:

<pre><?php
$string = "This here's a test; y'all";
echo md5(mysql_real_escape_string($string)); // hash of mysql_escaped string
echo mysql_real_escape_string(md5($string)); // just hash of the string
?></pre>

Outputs:

34457a356e8de87c0c4247b46b0721cd
89baa6cdc51ff262bb3c55a70e7702cf