Page 1 of 1
How many security problems does this login script have?
Posted: Fri Dec 22, 2006 11:07 am
by matthijs
As was discussed in
this thread, a big problem with PHP security is that many articles don't pay enough attention to security. So I just found this snippet of code for a login-script, and as an exercise I'd like to put it to the test here. I could have placed it in the coding critique section, but as it's so much security related I thought it would fit in here.
Code: Select all
<?
// Use session variable on this page. This function must put on the top of page.
session_start();
////// Logout Section. Delete all session variable.
session_destroy();
$message="";
////// Login Section.
$Login=$_POST['Login'];
if($Login){ // If clicked on Login button.
$username=$_POST['username'];
$password=$_POST['password'];
// Connect database.
$host="localhost"; // Host name.
$db_user=""; // MySQL username.
$db_password=""; // MySQL password.
$database="tutorial"; // Database name.
mysql_connect($host,$db_user,$db_password);
mysql_select_db($database);
// Check matching of username and password.
$result=mysql_query("select * from admin where username='$username' and password='$password'");
if(mysql_num_rows($result)!='0'){ // If match.
session_register("username"); // Craete session username.
header("location:main.php"); // Re-direct to main.php
}else{ // If not match.
$message="--- Incorrect Username or Password ---";
}
} // End Login authorize check.
?>
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Untitled Document</title>
</head>
<body>
<? echo $message; ?>
<form id="form1" name="form1" method="post" action="<? echo $PHP_SELF; ?>">
<table>
<tr>
<td>User : </td>
<td><input name="username" type="text" id="username" /></td>
</tr>
<tr>
<td>Password : </td>
<td><input name="password" type="password" id="password" /></td>
</tr>
</table>
<input name="Login" type="submit" id="Login" value="Login" />
</form>
</body>
</html>
So I'll start. First thing I see: missing db escape, therefore risking
sql injection.
Code: Select all
$result=mysql_query("select * from admin where username='$username' and password='$password'");
Second thing I see, missing
escape for html output:
There's more of course, but I'll leave the rest to you guys. How many flaws can we spot?
Posted: Fri Dec 22, 2006 12:40 pm
by DaveTheAve
All my login scripts have a captcha and a login monitoring system. Basically all the monitoring system does is you can only login once from an ip once every 30 seconds, if you fail to enter the right password three times or you submit your account & password without waiting 30 between trails, you get banned for one hour. Complex I know.
Re: How many security problems does this login script have?
Posted: Fri Dec 22, 2006 4:16 pm
by timvw
ini_set('error_reporting', E_ANAL); // Going to mention all the issues i can come up with (not only security related).
matthijs wrote:Code: Select all
// Use session variable on this page. This function must put on the top of page.
session_start();
This one bits if you include it from another file where you've already called session_start... I usually prefer to write:
Code: Select all
if (!isset($_SESSION)) { session_start(); }
matthijs wrote:Code: Select all
////// Logout Section. Delete all session variable.
session_destroy();
Why do you logout by default? Anyway, you might want to read the documentation on the first example at
http://be.php.net/session_destroy (about cookies propagating the session id).
matthijs wrote:Code: Select all
////// Login Section.
$Login=$_POST['Login'];
if($Login){ // If clicked on Login button.
$username=$_POST['username'];
$password=$_POST['password'];
Use array_key_exists (or a friend) in order to verify the values exist...
matthijs wrote:Code: Select all
// Connect database.
$host="localhost"; // Host name.
$db_user=""; // MySQL username.
$db_password=""; // MySQL password.
$database="tutorial"; // Database name.
mysql_connect($host,$db_user,$db_password);
mysql_select_db($database);
What happens if the connection could not be established?
matthijs wrote:Code: Select all
$result=mysql_query("select * from admin where username='$username' and password='$password'");
$username and $password are not prepared for use in a MySQL query (
http://www.php.net/mysql_real_escape_string).
Since you're only interested in the number of found rows, not in the data that exists in the rows i would recommend to SELECT COUNT(username) AS count instead..
matthijs wrote:Code: Select all
session_register("username"); // Craete session username.
That's old-style php. Read the documentation:
http://www.php.net/session_register.
I presume that the setting of the username in the setting leads to a 'higher' access level.. In this case i would recommend to call session_regenerate_id in order to avoid session stealing attacks (i believe the correct name is 'stealth attacks').
matthijs wrote:Code: Select all
header("location:main.php"); // Re-direct to main.php
Code: Select all
[/quote]
Afaik the HTTP protocol only supports absolute URIs for redirection... you also may want to call session_write_close to make sure the session data is flushed to disk... (And perhaps php should output a 303 status code instead of 302... but that's another matter )
[quote="matthijs"]
Code: Select all
<form id="form1" name="form1" method="post" action="<? echo $PHP_SELF; ?>">
Code: Select all
[/quote]
$PHP_SELF is oldstyle for $_SERVER['PHP_SELF']... Anyway this variable has not been prepared for use in a html context... And is known as a variable that can be easily exploited for XSS attacks... Using the action='#' instead seems like a good suggestion.
Posted: Fri Dec 22, 2006 4:27 pm
by matthijs
Great list Tim. Didn't know about ini_set('error_reporting', E_ANAL); but I might use it in the future
To be clear:
I did not write this script, I just got it from a tutorial today as an example of what we discussed in the other thread about lack of security awareness. Sorry if I wasn't clear.
One other thing I noticed about the code is the use of shorttags <? instead of <?php.
Furthermore, no hashing is used on username /password. Use of at least an md5() would be a good idea, in case the db gets hacked.
Posted: Fri Dec 22, 2006 4:31 pm
by timvw
matthijs wrote:To be clear: I did not write this script, I just got it from a tutorial today as an example of what we discussed in the other thread about lack of security awareness. Sorry if I wasn't clear.
It's just easier to talk to someone when i'm commenting on something

(I was aware you didn't write that code... I think most regulars at phpdn don't write code like that

)
Posted: Fri Dec 22, 2006 4:39 pm
by matthijs
It's ok

Thanks for your input.
Posted: Sat Dec 23, 2006 5:05 am
by Oren
Very poorly written code, so sad

Posted: Sat Dec 23, 2006 6:44 am
by timvw
But very common... (And yes, there used to be a time that i was proud if i wrote code like that...)
Posted: Sat Dec 23, 2006 4:24 pm
by Z3RO21
With my login systems I monitor users, log actions, and use a security system that checks for sql injection like queries, monitor login attempts (prevent brute force), and monitor for forgery in script elements (inserting certain data).
Posted: Sat Dec 23, 2006 5:21 pm
by timvw
And what do you do with the logs?
Posted: Sun Dec 24, 2006 1:15 pm
by Z3RO21
Store them, I needed to add the logs because I am doing a CMS for my school. The IT person wants to be able to see what people are doing.
Re: How many security problems does this login script have?
Posted: Thu Jan 04, 2007 6:25 am
by Mordred
timvw wrote:
matthijs wrote:Code: Select all
////// Logout Section. Delete all session variable.
session_destroy();
Why do you logout by default?
To avoid session fixation.
Re: How many security problems does this login script have?
Posted: Thu Jan 04, 2007 12:09 pm
by John Cartwright
Mordred wrote:timvw wrote:
matthijs wrote:Code: Select all
////// Logout Section. Delete all session variable.
session_destroy();
Why do you logout by default?
To avoid session fixation.
Typically when any account actions are taken,
session_regenerate_id() is used to avoid fixation.
Posted: Fri Jan 05, 2007 2:45 am
by Mordred
Oh, nice, thanks

Posted: Fri Jan 05, 2007 6:25 am
by timvw
For a second i thought i forgot to talk about that
timvw wrote:
I presume that the setting of the username in the setting leads to a 'higher' access level.. In this case i would recommend to call session_regenerate_id in order to avoid session stealing attacks (i believe the correct name is 'stealth attacks').