Help - Search - Members - Calendar
Full Version: Code Check: Login() Function
Zymic Webmaster Forums > Web Design & Development > Server Side Scripting > PHP
Archadium
Hi. I am new to Zymic, and I thought I would post some code here to see if anyone can spot any major security holes. I apologize for ugly code. smile.gif

CODE
<?php
/* name: login.php
description: Contains the login function
written by: Brian Kittrell, Archadium
created: 12/18/2008
modified: 12/27/2008
*/

$username = $_POST['username']; // checks to see if post variables set from a form
$password = $_POST['password'];

if ($_POST['login']) // if the user just submitted the login form, run the code
{
if (ereg("^[a-zA-Z0-9]{1,16}$",$username)) { $nlogin++; } // prevent SQL injection
if (ereg("^[a-zA-Z0-9]{1,16}$",$password)) { $nlogin++; } // prevent SQL injection
if ($nlogin == 2) // if not trying to hack, allow part 2
{
$password = arch_encrypt($password); // encrypt that which the user has provided
$luser_query = "SELECT * FROM archadium_users WHERE username='$username' AND password='$password'"; // check against the database
$luser_result = mysql_query($luser_query); // run it
$lusercheck = mysql_num_rows($luser_result); // detect if the user exists
if ($lusercheck == 1) // if the user is in the database and the info matches
{
$login_query = "SELECT uid, username, password, nickname, active FROM archadium_users WHERE username='$username'"; // get all the info
$login_result = mysql_query($login_query); // run it
while ($row = mysql_fetch_assoc($login_result)) { // assign the variables to the session
$_SESSION['uid'] = $row['uid'];
$_SESSION['username'] = $row['username'];
$_SESSION['password'] = $row['password'];
$_SESSION['nickname'] = $row['nickname'];
$_SESSION['active'] = $row['active'];
$user_pm_query = "SELECT * FROM archadium_pm WHERE id_to = '$uid' AND new = '1'"; // Check to see if user has new private messages
$user_pm_result = mysql_query($user_pm_query); // run it
$user_pm_count = mysql_num_rows($user_pm_result); // Counts number of new messages in message box
$_SESSION['pmcount'] = $user_pm_count; // Sets session variable for pm counter
printf("<script>location.href='" . $current_location . "'</script>"); // kicks the user over to the members page
}
}
else { echo "<CENTER><FONT COLOR=red>Invalid login, please try again. MSG2</CENTER>"; } // if none of the above worked, you must try again.
}
else { echo "<CENTER><FONT COLOR=red>Invalid login, please try again. MSG1</CENTER>"; } // you must have an invalid password/username
}

if (isset($_SESSION['username']) and isset($_SESSION['password'])){ // checks to see if a session is active
$user_pm_query = "SELECT * FROM archadium_pm WHERE id_to = '$uid' AND new = '1'"; // get a count of new private messages
$user_pm_result = mysql_query($user_pm_query); // run it
$user_pm_count = mysql_num_rows($user_pm_result); // Counts number of new messages in message box
$_SESSION['pmcount'] = $user_pm_count; // Sets session variable for pm counter
echo "<p><FONT SIZE=2>Logged in as<B>"," ",$_SESSION['nickname'],"</B> | ","<A HREF=$logout>Logout</A></FONT><BR>You have <A HREF=$pm_url>",$user_pm_count,"</A> new messages.";} // return a formatted display of details for the user
else { // if they didn't have a session, allow them to login
echo "<form action=".$_SERVER['PHP_SELF']." method=\"post\">
<TABLE><TR><TD></TD><TD><p><b>Username:</TD><TD><INPUT TYPE=TEXT NAME=username></TD>
<TD><p><b>Password:</TD><TD><INPUT TYPE=password name=password></TD></TR>";
echo "<TR><td align=center colspan=4><FONT COLOR=white>Registration is free! <A HREF=$base_dir" . "register.php>Sign up</A> now!<TD colspan=2><CENTER><INPUT TYPE=submit name=login value=login></form></TD></TD></TR></TABLE>";
}
?>


Not as ugly as I thought, but some of the formatting got messed up. But, prettier than I expected.
Bogey
Why are you putting the PM count into a session?
"$_SESSION['pmcount'] = $user_pm_count; // Sets session variable for pm counter"?

So, the check is case-sensitive?

Also, why are you storing these in a session?
CODE
                $_SESSION['username'] = $row['username'];
                $_SESSION['password'] = $row['password'];
                $_SESSION['nickname'] = $row['nickname'];
                $_SESSION['active'] = $row['active'];

I usually do that when it is required, but store the user ID only... but that's probably because I use phpBB3 integration for my site and it's easy to set up variables there.

As far as security goes... I would strip the username and password using mysql_real_escape_string(); function, this way, you don't need to do the "prevent SQL injection" tests (Those ereg's in the if statement.

Instead of the word 'and' in the if statement you can use the operators created especially for this reason.

&& = and
|| = or
== = equals

There are much more operators... (http://www.w3schools.com/PHP/php_operators.asp)

When you put variables (or arrays) in strings that are used in double-quotes " " ", you don't have to do (" . $blah . "), you can use "{$blah}" instead...

I also like to user lower cased letters for tag names, instead of uppercase.

CODE
<?php
/* name: login.php
description: Contains the login function
written by: Brian Kittrell, Archadium
created: 12/18/2008
modified: 12/27/2008
*/

$username = mysql_real_escape_string($_POST['username']); // checks to see if post variables set from a form
$password = mysql_real_escape_string($_POST['password']);

if ($_POST['login']) // if the user just submitted the login form, run the code
{

    $password = arch_encrypt($password); // encrypt that which the user has provided
    $luser_query = "SELECT * FROM archadium_users WHERE username='$username' AND password='$password'"; // check against the database
    $luser_result = mysql_query($luser_query); // run it
    $lusercheck = mysql_num_rows($luser_result); // detect if the user exists
    if ($lusercheck == 1) // if the user is in the database and the info matches
    {
        $login_query = "SELECT uid, username, password, nickname, active FROM archadium_users WHERE username='$username'"; // get all the info
        $login_result = mysql_query($login_query); // run it
        while ($row = mysql_fetch_assoc($login_result)) // assign the variables to the session
        {
            $_SESSION['uid'] = $row['uid'];
            $_SESSION['username'] = $row['username'];
            $_SESSION['password'] = $row['password'];
            $_SESSION['nickname'] = $row['nickname'];
            $_SESSION['active'] = $row['active'];
            $user_pm_query = "SELECT * FROM archadium_pm WHERE id_to = '$uid' AND new = '1'"; // Check to see if user has new private messages
            $user_pm_result = mysql_query($user_pm_query); // run it
            $user_pm_count = mysql_num_rows($user_pm_result); // Counts number of new messages in message box
            $_SESSION['pmcount'] = $user_pm_count; // Sets session variable for pm counter
            printf("<script>location.href='{$current_location}'</script>"); // kicks the user over to the members page
        }
    }
    else
    {
    echo "<CENTER><FONT COLOR=red>Invalid login, please try again. MSG2</CENTER>"; // if none of the above worked, you must try again.
    }
}

if (isset($_SESSION['username']) && isset($_SESSION['password'])) // checks to see if a session is active
{
    $user_pm_query = "SELECT * FROM archadium_pm WHERE id_to = '$uid' AND new = '1'"; // get a count of new private messages
    $user_pm_result = mysql_query($user_pm_query); // run it
    $user_pm_count = mysql_num_rows($user_pm_result); // Counts number of new messages in message box
    $_SESSION['pmcount'] = $user_pm_count; // Sets session variable for pm counter ( ? )
    echo "<p><FONT SIZE=2>Logged in as<B> {$_SESSION['nickname']}</B> | <A HREF={$logout}>Logout</A></FONT><BR>You have <A HREF={$pm_url}>{$user_pm_count}</A> new messages."; // return a formatted display of details for the user
}
else
{ // if they didn't have a session, allow them to login
    echo "<form action=\"{$_SERVER['PHP_SELF']}\" method=\"post\">
        <TABLE><TR><TD></TD><TD><p><b>Username:</TD><TD><INPUT TYPE=TEXT NAME=username></TD>
        <TD><p><b>Password:</TD><TD><INPUT TYPE=password name=password></TD></TR>";
    echo "<TR><td align=center colspan=4><FONT COLOR=white>Registration is free! <A HREF={$base_dir}register.php>Sign up</A> now!<TD colspan=2><CENTER><INPUT TYPE=submit name=login value=login></form></TD></TD></TR></TABLE>";
}
?>
This is a "lo-fi" version of our main content. To view the full version with more information, formatting and images, please click here.
Invision Power Board © 2001-2010 Invision Power Services, Inc.