Page 1 of 2
Please nitpick my login script!
Posted: Mon Jan 26, 2009 5:09 pm
by andym01480
Code: Select all
<?php
session_start();
function loginform()
{
echo "<h2>You need to login to access this page</h2>";
echo "<form action=\"{$_SERVER['REQUEST_URI']}".setUrlVariables()."\" method=\"post\">";//setUrlVariables appends variables!
echo '<table>';
echo '<tr><td>Email Address</td><td><input type="text" name="email"></td></tr>';
echo '<tr><td>Password</td><td><input type="password" name="password"></td></tr>';
echo '<tr><td> </td><td><input type="radio" value="y" name="remember">Remember me</td></tr>';
echo '<tr><td> </td><td><input type="submit" name="submit" value="Submit"></td></tr>';
echo '</table>';
echo '</form>';
echo '<p><a href="forgotten.php">Forgotten password?</a></p>';
exit();
}
//check if cookies set or session indicates login
if (($_SESSION['userlevel']>=$loginlevel)||(isset($_COOKIE['remember'])&&($_COOKIE['remember']=='1')&&($_COOKIE['userlevel']>=$loginlevel))) $loggedin='1';
//check form overide values
if($loggedin!='1')
{
if($_POST['email']=='admin' && $_POST['password']=='password') //obviously not the real password!
{
$loggedin='1';
$userlevel='2';
$email=$_POST['email'];
}
}
//check form values
if($loggedin!='1')
{
$email=$_POST['email'];
$password=md5($_POST['password']);
include("../includes/email.inc.php");
//check for valid email address
if (!emailchecker($email)) loginform();
//check database
$sql="SELECT email,userlevel,password FROM address";
$result=mysql_query($sql);
while($row=mysql_fetch_assoc($result))
{
if($row['email']==$email && $row['password']==$password && $row['userlevel']>=$loginlevel)
{
$loggedin=1;
$userlevel=$row['userlevel'];
//set cookies if wanted
if(($_POST['remember']=='y')&& $loggedin=='1')
{
setcookie('userlevel',$userlevel,time()+60*60*24*30,'/','.domainname');
setcookie('remember','1',time()+60*60*24*30,'/','.domainname');
setcookie('emailaddress',$email,time()+60*60*24*30,'/','.domainname');
}
}
}
}//end check form values
$log='1';
if($loggedin=='1') $log='0';
//acceslog
if(!isset($email))
{
if (isset($_COOKIE['emailaddress'])) $email=$_COOKIE['emailaddress'];
if (isset($_SESSION['email'])) $email=$_SESSION['email'];
}
$sql="INSERT into accesslog (ip,email,page,date,failure,accesslog_id) VALUES ('{$_SERVER['REMOTE_ADDR']}','{$email}','{$_SERVER['REQUEST_URI']}',NOW(),'{$log}','')";
$result=mysql_query($sql);
if($loggedin!='1') loginform();
if ($loggedin=='1')
{
$_SESSION['userlevel']=$userlevel;
$_SESSION['email']=$email;
}
?>
The above login.php is called at the beginning of every protected file...
Code: Select all
<?php
session_start();
include("../includes/config.inc.php");
$loginlevel='2'; //require admin level access
include("login.php");
//Logged in
I've tested it as best I can. Are there any holes?
Re: Please nitpick my login script!
Posted: Mon Jan 26, 2009 5:29 pm
by kaisellgren
Code: Select all
if (($_SESSION['userlevel']>=$loginlevel)||(isset($_COOKIE['remember'])&&($_COOKIE['remember']=='1')&&($_COOKIE['userlevel']>=$loginlevel))) $loggedin='1';
Cookie forging.
Why are you loading this twice?
Code: Select all
if (!emailchecker($email)) loginform();
Admins (or even other users) are not allowed to have invalid or empty email addresses?
Code: Select all
$sql="SELECT email,userlevel,password FROM address";
$result=mysql_query($sql);
while($row=mysql_fetch_assoc($result))
{
if($row['email']==$email && $row['password']==$password && $row['userlevel']>=$loginlevel)
Did you know that you can make this perform tens or hundreds of times faster by doing a SQL WHERE clause?
Code: Select all
$sql="INSERT into accesslog (ip,email,page,date,failure,accesslog_id) VALUES ('{$_SERVER['REMOTE_ADDR']}','{$email}','{$_SERVER['REQUEST_URI']}',NOW(),'{$log}','')";
SQL injection.
Your application is also potentially vulnerable to SQL and other attacks in case Register Globals is enabled.
Re: Please nitpick my login script!
Posted: Mon Jan 26, 2009 5:51 pm
by andym01480
Okay removed cookie remember bit until I can get my head round that.
Why is this vulnerable to sql injection?
Code: Select all
$sql="INSERT into accesslog (ip,email,page,date,failure,accesslog_id) VALUES ('{$_SERVER['REMOTE_ADDR']}','{$email}','{$_SERVER['REQUEST_URI']}',NOW(),'{$log}','')";
$email can only be a valid email address, $log is given a value by the script. Are you saying that a user can change server variables?
Re: Please nitpick my login script!
Posted: Mon Jan 26, 2009 6:30 pm
by kaisellgren
andym01480 wrote:$email can only be a valid email address
I create a cookie that has emailaddress containing SQL injection then I pass directly to login.php
EDIT: I swear I saw link to your website somewhere? It has lots of faces in the header, white background, ... did you take it away?

Re: Please nitpick my login script!
Posted: Mon Jan 26, 2009 7:31 pm
by aschlosberg
Register Globals can be off and SQL injection will still be possible by sending cookies with the following values:
remember = 1
userlevel = 999999
This will cause $loggedin to be 1 on line 20. Everything can then be ignored until line 63 and then if no session cookie is passed then anything passed in the 'emailaddress' value of a cookie will be injected.
Install the Firefox extension "Cookie Editor" and give it a try.
Re: Please nitpick my login script!
Posted: Tue Jan 27, 2009 1:43 am
by andym01480
Thanks guys
I removed the hint of where the site was as soon as I realised how vulnerable I was.
Will do some beefing up today.
Removed the cookie stuff and changed the email vulnerability!
Re: Please nitpick my login script!
Posted: Tue Jan 27, 2009 4:25 am
by andym01480
I've added a throttle and rewritten taking on board suggestions
Code: Select all
<?php
session_start();
include("../includes/email.inc.php"); //emailchecker
$sql=array();
$now=time();
$max=$now-15;
$sql['ip']=mysql_real_escape_string($_SERVER['REMOTE_ADDR']);
$sql['uri']=mysql_real_escape_string($_SERVER['REQUEST_URI']);
function loginform()
{
echo "<h2>You need to login to access this page</h2>";
echo "<form action=\"{$_SERVER['REQUEST_URI']}".setUrlVariables()."\" method=\"post\">";
echo '<table>';
echo '<tr><td>Email Address</td><td><input type="text" name="email"></td></tr>';
echo '<tr><td>Password</td><td><input type="password" name="password"></td></tr>';
echo '<tr><td> </td><td><input type="radio" value="y" name="remember">Remember me</td></tr>';
echo '<tr><td> </td><td><input type="submit" name="loginsubmit" value="Submit"></td></tr>';
echo '</table>';
echo '</form>';
echo '<p><a href="forgotten.php">Forgotten password?</a></p>';
exit();
}
//check form submission
if($_POST['loginsubmit']=='Submit')
{
//validate form
if (!emailchecker($_POST['email'])) //not a valid email address so reprint form
{
loginform();
exit();
}
$sql['email']=mysql_real_escape_string($_POST['email']);
$sql['password']=mysql_real_escape_string(md5($_POST['password']));
//check for user
$query="SELECT email,password,last_failure,userlevel FROM address WHERE `email`='{$sql['email']}'";
$result=mysql_query($query);
if(!$result) //no email in database
{
loginform();
exit();
}
//email in database
$row=mysql_fetch_assoc($result);
//check for throttle
if ($row['last_failure']>=$max)
{
//update failure
$failuresql="UPDATE `address` SET `last_failure`=$now WHERE `email`='{$sql['email']}'";
mysql_query($failuresql);
//update access log
$sql="INSERT into accesslog (ip,email,page,date,failure,accesslog_id) VALUES ('{$sql['ip']}','{$sql['email']}','{$sql['uri']}',NOW(),'0','')";
$result=mysql_query($sql);
//o/p login form
loginform();
exit();
}
//check for correct password
if($row['password']!=$sql['password'])
{
//update failure
$failuresql="UPDATE `address` SET `last_failure`=$now WHERE `email`='{$sql['email']}'";
mysql_query($failuresql);
//update access log
$accesssql="INSERT into accesslog (ip,email,page,date,failure,accesslog_id) VALUES ('{$sql['ip']}','{$sql['email']}','{$sql['uri']}',NOW(),'0','')";
$result=mysql_query($accesssql);
//o/p login form
loginform();
exit();
}
//check for high enough user permission
if($row['userlevel']<$loginlevel)
{
//update failure
$failuresql="UPDATE `address` SET `last_failure`=$now WHERE `email`='{$sql['email']}'";
mysql_query($failuresql);
//update access log
$accesssql="INSERT into accesslog (ip,email,page,date,failure,accesslog_id) VALUES ('{$sql['ip']}','{$sql['email']}','{$sql['uri']}',NOW(),'0','')";
$result=mysql_query($accesssql);
//o/p login form
loginform();
exit();
}
//passed all check so valid login
$_SESSION['loggedin']=$loginlevel;
//update access log with successful login
$accesssql="INSERT into accesslog (ip,email,page,date,failure,accesslog_id) VALUES ('{$sql['ip']}','{$sql['email']}','{$sql['uri']}',NOW(),'1','')";
$result=mysql_query($accesssql);
}
//check cookie
if($_SESSION['loggedin']<$loginlevel) loginform();
//Logged in
?>
Two questions
What vulnerabilities are there now?
How can improve it further?
Re: Please nitpick my login script!
Posted: Tue Jan 27, 2009 7:17 am
by kaisellgren
If the data is never submitted with POST, then it skips the huge IF -block and comes to:
Code: Select all
if($_SESSION['loggedin']<$loginlevel) loginform();
Just wanted to say that could generate errors (warnings) for you, because you have never even defined $loginlevel (or even the session var). Make sure you won't get errors or warnings.
Code: Select all
if(!$result) //no email in database
I recommend using this instead:
As far as I can see the "login level check", "throttle" and "invalid password" are similar - so you could melt them together and use:
Code: Select all
if ($row['last_failure']>=$max || $row['password']!=$sql['password'] || $row['userlevel']<$loginlevel)
One thing is that you do not define $loginlevel anywhere. Do it.
Re: Please nitpick my login script!
Posted: Tue Jan 27, 2009 7:56 am
by superdezign
Also, I prefer not to trust $_SERVER variables, as that array is the most vulnerable to XSS injection. Such as in your <form> element's "action" attribute, instead of using $_SERVER['REQUEST_URI'], just trust the browser to realize that ' action = "?url=variables&are=here" ' is the same page, or set a $URL_ROOT variable of some sort in an universally included PHP file (as I do :3).
Re: Please nitpick my login script!
Posted: Tue Jan 27, 2009 9:59 am
by andym01480
I used $_SERVER['REQUEST_URI'] as the login script is always an include of the Script that is calling it.
But I just tested a quick form with "" as the action and discovered it does the same thing in IE7, Chrome and FF!
How would
Code: Select all
action="<?php echo $_SERVER['REQUEST_URI']?>"
be vulnerable to XSS? Can you give an example of how that could happen in say my login script?
Re: Please nitpick my login script!
Posted: Tue Jan 27, 2009 10:28 am
by Mordred
The "throttle" code is useless and moreover it can be used for DOS: just try to login with the someone's account every 14 seconds and he won't be able to login himself.
Re: Please nitpick my login script!
Posted: Tue Jan 27, 2009 10:46 am
by andym01480
I got that from Chris Shifflet's book!
Re: Please nitpick my login script!
Posted: Tue Jan 27, 2009 11:21 am
by kaisellgren
Mordred wrote:The "throttle" code is useless and moreover it can be used for DOS: just try to login with the someone's account every 14 seconds and he won't be able to login himself.
Damn I missed that

- @andym01480: This is the reason why you have to understand security and not ask people to fix a script at forums, because you often make mistakes and people are not willing to spend all time on fixing your script so people often miss vulnerabilities as they do not concentrate 100% on your script.
Btw, this kind of attacks are called Account Lockout Attacks:
http://www.owasp.org/index.php/Account_lockout_attack
I'm not sure what Chris's book says as I have never read it, but as for the REQUEST_URI, one can insert custom text on your site like login.php?anything_you_want_to_be_on_your_site
Re: Please nitpick my login script!
Posted: Tue Jan 27, 2009 12:03 pm
by andym01480
This is the reason why you have to understand security and not ask people to fix a script at forums, because you often make mistakes and people are not willing to spend all time on fixing your script so people often miss vulnerabilities as they do not concentrate 100% on your script.
One way to "understand security" is to have people point out holes - I have learnt a lot in the last 24 hours!
Re: Please nitpick my login script!
Posted: Tue Jan 27, 2009 12:16 pm
by kaisellgren
andym01480 wrote:This is the reason why you have to understand security and not ask people to fix a script at forums, because you often make mistakes and people are not willing to spend all time on fixing your script so people often miss vulnerabilities as they do not concentrate 100% on your script.
One way to "understand security" is to have people point out holes - I have learnt a lot in the last 24 hours!
As long as there are people to point out the holes you have
