4
\$\begingroup\$

I made this admin login script a while ago. It works fine in terms of checking the database for username and password and logs me in. What I don't quite understand is how to secure my login once in.

Currently using:

  • Cookie and Session check to show admin pages

    if(!isset($_COOKIE['Cckiuas']) && !isset($_SESSION["admin"]) &&
    !isset($_SESSION["session"]) && !isset($_SESSION["active"])){ 
         dont show page 
    }
    else {
        show page
    } 
    

If I create a cookie in my browser with the same name with any value in it, it lets me in.

What would be the best way to check if I'm "logged in" or not?

Feel free to suggest improvements, I'm guessing there will be a lot.

Login.php

<?php
// create 2 random numbers used for our sum later
$min = 1;
$max = 50;
$sum1 = rand($min,$max);
$sum2 = rand($min,$max);
?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<link href="css/login.css" rel="stylesheet" type="text/css" />
<title>WebCodex - Please Login</title>
</head>

<body>
<!-- CONTACT FORM STARTS HERE --> 
<div id="loginformwrap">
<div style="color: #666; font-family: Lato; font-size: 36px; text-align: center;"></div>
<div id="loginformwrapper">       
<form name="loginform" method="post" action="<?php $_SERVER['PHP_SELF']; ?>">
<table class="adminlogin">
<tr>
<td valign="top">
<input type="text" class="default" required="required" name="username"  value="" placeholder="Username" id="adminusername" maxlength="80" style="text-align: center; width:280px; color: #333;" onblur="this.style.color='#333'" onfocus="this.style.color='#000'"/> 
</td>
</tr>
<tr>
<td valign="top">
<input type="password" class="default" required="required" name="password"  value="" placeholder="Password" id="adminpassword" maxlength="80" style="text-align: center; width:280px; color: #333;" onblur="this.style.color='#333'" onfocus="this.style.color='#000'"/> 
</td>
</tr>
<tr>
<td colspan="2" style="text-align:center" >
<div class="antispammessage">
Please verify you are Human
<div class="antispamquestion">
Using only numbers, what is <span class="sum"><?php echo $sum1; ?></span> plus <span class="sum"><?php echo $sum2; ?></span>? &nbsp; 
<input type="text" name="securityA" required="required" id="securityA" value="" placeholder="?" maxlength="100" style="color: #333;width:30px; text-align: center;" onblur="this.style.color='#333'" onfocus="this.style.color='#000'"/> 
<input type="hidden" name="sum" id="sum" value="<?php echo $database->filter($sum1 + $sum2); ?>" />
</div>
</div>
</td>
</tr>
<tr>
<td colspan="2">
<input id="adminloginbutton" type="submit" name="login" value="LOGIN">
</td>
</tr>
</table>
</form>
     <!-- CONTACT FORM ENDS HERE -->
</div>
<?php 
if(isset($_SESSION['errors']) != '' && is_array($_SESSION['errors'])) {
    $error = $database->filter($_SESSION['errors']);
    $error = array_pop($error);
    foreach($error as $errors) {
    echo '<center><span class="adminerror" id="message">'.$errors.'</span></center>';
    }
    $_SESSION['errors'] = '';
}
elseif(isset($_SESSION['errors'])) {
    echo '<center><span class="adminerror" id="message">'.$_SESSION['errors'].'</span></center>';
    $_SESSION['errors'] = '';
}
 ?>
</div>


</body>
</html>

<?php
require_once("config.php");

// log user in 
if (isset($_POST['login'])) {
    $username = $database->filter($_POST['username']);
    $password = $database->filter($_POST['password']);
    $securityA = $database->filter($_POST['securityA']);
    $sum = $database->filter($_POST['sum']);

    $login = $backend->loginCheck($username, $password, $securityA, $sum);


        // IF loginCheck returns True
        if ($login === true) {
            $active = '1';
            $lastlogin = date('d / m / y - H:ia');
            $session = uniqid(mt_rand(), true);
            $update = array('session' => $session, 'lastlogin' => $lastlogin, 'active' => $active);
            $where = array('username' => $username, 'password' => $password);
            $database->update('wcx_admin', $update, $where, 1);

            // REGISTER SESSIONS 
            $_SESSION["admin"] = $username; // Bad Idea?
            $_SESSION["active"] = $active;
            $_SESSION["session"] = $session;

            // CREATE COOKIE
            $cckiuas = $username + $active + $session;
            $expire = time()+3600;

            // SET COOKIE AND REDIRECT BACK TO INDEX.php
            setcookie("Cckiuas",$cckiuas,$expire,'/');
            header("Location: index.php");
        }
}


?>

login function

public function loginCheck($username, $password, $securityA, $sum) {
    $active = '';
    $errors = '';
    $salt = '';
    $session = '';
    $dbpass = '';


    if (!$securityA) {
        $errors[] = 'Please enter the Security Answer';
    }

    if ($securityA != $sum && $securityA != '') {
        $errors[] = 'Security Answer is wrong';
    }

    if (!$username) {
        $errors[] = 'Please enter a Username';
    }

    else{
        $query = "SELECT username FROM wcx_admin WHERE username='$username'";
        $count = $this->num_rows($query);
        if ($count === 0) {
            $errors[] = 'Incorrect Username or Password';
        }
    }

    if (!$password) {
        $errors[] = 'Please enter a Password';
    }
    else {
        $query = "SELECT username, password, salt FROM wcx_admin WHERE username='$username'";
        $result = $this->get_results($query);
        foreach( $result as $row ) {
            $salt = $this->filter($row['salt']);
            $dbpass = $this->filter($row['password']);
        }
            $password = hash("sha512", $password . $salt);
        if  ($password !== $dbpass) {
            $errors[] = 'Incorrect Username or Password';

        }
    }

    $query = "SELECT active FROM wcx_admin WHERE username='$username'";
    $result = $this->get_results($query);
    foreach( $result as $row ) {
        $activecheck = $this->filter($row['active']);
    }
    if($activecheck === '1') {
        $errors[] = 'You are already Logged in!';
    }

    if ($errors) {
        $_SESSION['errors'] = array($errors);
        header("Location: index.php");
    }
    else {
        return true;    
    }
}

filter function

// SANITISE INPUT DATA
public function filter( $data ) {
    if( !is_array( $data ) ) {
        $data = trim( htmlentities( $data ) );
        $data = mysqli_real_escape_string( $this->link, $data );
    }
    else {
        // SANITISE ARRAY FUNCTION
        $data = array_map( array( 'DB', 'filter' ), $data );
    }
    return $data;
}
\$\endgroup\$
2
\$\begingroup\$

Basic login idea

The easiest way is to set a username and password cookie and to check each time if the details are valid. This is, however, not very safe, as one could simply set their cookies as the password's hash and be logged in, without needing to know the clear text password. It's definitely better than just checking for the existence of a cookie, which should be avoided even in trivial scripts.

You can get a quick improvement in security by creating a random ID upon login, storing it and then setting a $_SESSION or cookie parameter to that secret value. It's not bank-grade safety and this will restrict your users to only be logged in at one place only (since logging in a second time will generate a new ID -> old one is not valid anymore).

My advice at this point would be: think big, but do little steps. You probably want to use an iterative process for the development of your script, i.e. start with something basic, make it work, and then add to it bit by bit.

Here are a few concerns I have.

The admin check page

if(!isset($_COOKIE['Cckiuas']) && !isset($_SESSION["admin"]) && 
   !isset($_SESSION["session"]) && !isset($_SESSION["active"])){

You are doing four negative checks and connecting them with && (i.e. the AND operator). That means as soon as the cookie is set, or any one of the session parameters, the admin page will be shown. Did you perhaps mean to use ||?

SQL Injection

$query = "SELECT username, password, salt FROM wcx_admin WHERE username='$username'"; 

You're adding variables directly into SQL code, which is a big no-no. If you're using MySQLi to do your SQL things, then mysqli_real_escape_string() is your best friend in that context.

Fetching user data from database

$result = $this->get_results($query);
foreach( $result as $row ) {
    $salt = $this->filter($row['salt']);
    $dbpass = $this->filter($row['password']);
}
    $password = hash("sha512", $password . $salt);

(Awkward indentation on the last line), but the foreach() puzzles me here. You probably just want to do

$result = $this->get_results($query);
if(isset($result[0])) {
    $salt = $result[0]['salt'];
    $dbpass = $result[0]['password'];
}
else {  /* Handle login error */ }

Best would be to fetch the row you need from wcx_admin once, and only once you're sure you'll need the data (i.e. the user has supplied a user name and password, and he has positively solved the anti-spam problem).

Output all errors and warnings

At least during development, I strongly suggest you use error_reporting(E_ALL) so that you can be informed about certain inconsistencies. For example, you set $error = '' at the top of loginCheck, but then you add to $error as if it were an array, e.g. $error[] = 'Please enter a Username';. You probably mean to do $error = array();

Flow

  1. I would check for the validity of the user input password before fetching things from the database. I find your check: if (!$password) to come at a very late time.
  2. Some people are against using more than one return statement, but I personally like to get out of a function as soon as possible. Note that even when you add "Incorrect Username or Password" to your errors, you will still fetch for the user name and password and validate the password. If an error occurs, you want to stop checking things.
  3. You're fetching twice the same data from wcx_admin.
  4. The function loginCheck only has one return true statement. I would suggest return false in the other cases for consistency.

Conceptual questions

  1. I'm not really sure if the error of "You are already Logged in!" makes sense. What if your database says I'm logged on but I somehow am not logged in anymore – will I just be blocked? What if I switch to another device (or browser)?
  2. I would suggest separating your check for $sum into another function. This way, you can focus in loginCheck on the login mechanism only and handle the sum elsewhere. This makes it separate and your functions a little slimmer.
  3. You're using your filter method on data fetched from the database but not on $query (as mentioned above)? I think it should be the other way around.

Good points

Good on you for using salts — clap clap! Also happy to see that you're not giving a user information whether an account exists or not, but only an "incorrect username or password" error for all cases. That's nice.

\$\endgroup\$
  • \$\begingroup\$ I was under the impression that || / OR would mean one of the values and && would mean all of the values were SET, I will do a full review of your answer in 9 hours time as it's late here and I'm on mobile right now \$\endgroup\$ – CodeX Jun 27 '14 at 23:20
  • \$\begingroup\$ Let's replace isset($_COOKIE['Cckiuas']) to "I know the location" and isset($_SESSION["admin"]) to "I know the time". The code mentioned basically says "if I do not know the location and I do not know the time, I will not go to the party" (but if I know only one, I will go anyways). Probably best to make a check for something positive, along the lines of "if I know the place and I know the time, I will go" (show the admin page) \$\endgroup\$ – ljacqu Jun 28 '14 at 0:02
  • \$\begingroup\$ How would I check the password without querying the password in the database? \$\endgroup\$ – CodeX Jun 28 '14 at 9:06
  • \$\begingroup\$ @CodeX Hmm? You need to query the database once for information, but in the above script information is being fetched from the database at three different times: SELECT username, SELECT username, password, salt and SELECT active. Better to fetch everything once – SELECT username, password, salt, active – and if there's no result, you know the user doesn't exist, otherwise proceed to validate the password and the active field. :) \$\endgroup\$ – ljacqu Jun 28 '14 at 9:14
  • \$\begingroup\$ That makes sense, fetch the data, compare the data, if !== output an error else set the session and log them in, i think I've really over thought the process, time for a complete re-write, thanks again \$\endgroup\$ – CodeX Jun 28 '14 at 9:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.