HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Possible improvements on admin login area?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
adminloginpossibleimprovementsarea

Problem

I've created an admin login area for an application I am planning to code, and I've used the following login.html page to let the user type in his data (I left out parts like "id", "placeholder" etc. to make it shorter):


  
  
  


The form is processed as is, no JavaScript interfering here. The login.php then looks like this:

";
    } else {
      echo "Wrong username and/or password! You are set back in 3 seconds";
      echo "";
    }
  } else {
    echo "This is a restricted area, you have to log in!";
    echo "";
  }
?>


The admin_area.php file, which the user is then being redirected to, has the following short placeholder code:

";
  } else {
    echo "";
    echo "";
    echo "Hello World!";
    echo "";
    echo "";
  }
?>


The essential part of the session_destroy.js file on this page contains this code snippet:

$(window).on("beforeunload", function() {
  $.get("session_destroy.php");
  return "You have left the page. Your session has been terminated.";
});


And finally, the session_destroy.php, which should terminate all session data, contains the following lines (taken from the PHP manual):

";
  } else {
    $_SESSION = array();
    if (ini_get("session.use_cookies")) {
      $params = session_get_cookie_params();
      setcookie(session_name(), '', time() - 42000,
        $params["path"], $params["domain"],
        $params["secure"], $params["httponly"]
      );
    }
    session_destroy();
    header('Location: login.html');
  }
?>


Now this "construct" seems to be working just fine. I am wondering if there are some improvements on these lines, as I have researched everything myself and I am not sure how "secure" and correct these methods might be.

I am thinking of possible security improvements to the form-processing and session-handling / destroying. Are there any areas in which I should do more research?

Solution

Tha major security issue I see is that you do not have any limiting or logs for wrong/success attempts. One can use automated brute force attacks trying all day/night long to guess your username and password. If lack of success, at least you might have your app slowed down, because of the billion requests.

As you requested, I tried to add some code examples around my suggestion:

escape($_POST['userName']);
    $pass = $db->escape($_POST['password']);
    $session_id = session_id();
    if (!in_array($success, array(0, 1))) {
        return false;
    }
    $db->query("INSERT INTO `log_attempts` 
                    (`user`, `pass`, `ip`, `session_id`, `success`, `tried_on`)
                VALUES
                    ('$user', '$pass', '{$_SERVER['REMOTE_ADDR']}', '$session_id', '$success', NOW());
            ");
    return $db->affected_rows > 0;
}

function isAttemptsLimitExceeded() {
    $max_attempts = 5;
    $session_id = session_id();
    $res = $db->query("SELECT 
                            COUNT(*) AS cnt
                        FROM 
                            `log_attempts`
                        WHERE
                            `session_id` = '$session_id' AND
                            `tried_on` > DATE_SUB(NOW(), INTERVAL 24 HOUR) AND
                            `success` = '0';
                    ");
    $row = $db->fetch($res);
    if ($row['cnt'] >= $max_attempts) {
        return true;
    }
    return false;
}

function banFailedLogins() {
    if(isAttemptsLimitExceeded()) {
        $db->query("INSERT INTO `blacklist`
                        (`ip`, `banned_on`)
                    VALUES
                        ('{$_SERVER['REMOTE_ADDR']}', NOW()");
    }
    return $db->affected_rows > 0;
}

function clearBans() {
    $db->query("DELETE from `blacklist` WHERE `ip` = '{$_SERVER['REMOTE_ADDR']}';");
    return $db->affected_rows > 0;
}

//check for expired bans?
function canAcess() {
    $ban_expire = 24; // hours
    $res = $db->query("SELECT 
                            COUNT(*) AS cnt 
                        FROM 
                            `blacklist` 
                        WHERE 
                            `ip` = '{$_SERVER['REMOTE_ADDR']}' AND
                            `banned_on` > DATE_SUB(NOW(), INTERVAL $ban_expire HOUR);
                        ");
    $row = $db->fetch($res);
    if ($row['cnt'] > 0) {
        return false;
    }
    else {
        clearBans();
        return true;
    }
}

if (!canAcess()) {
    die("You are banned for failed login attempts");
}
  if (isset($_POST['password']) && isset($_POST['userName'])) {
    if ($_POST['password'] == "myPassword" && $_POST['userName'] == "myUsername") {
      if (!session_id()) {
        session_start();
        $_SESSION['logon'] = true;
        logAttempt(1); //log successfull attempt
        header('Location: admin_area.php');
        die();
      }
    } else if ($_POST["password"] == "" || $_POST["userName"] == "") {
      echo "Please enter both a username and a password! You are sent back in 3 seconds";
      logAttempt(0); //log failed attempt
      banFailedLogins();
      echo "";
    } else {
      echo "Wrong username and/or password! You are set back in 3 seconds";
      logAttempt(0); //log failed attempt
      banFailedLogins();
      echo "";
    }
  } else {
    echo "This is a restricted area, you have to log in!";
    echo "";
  }

?>

Code Snippets

<?php

//the db interaction is pseudo code, because it depends on the API you are using
/**
 * 
 * @param int $success : 0 for fail, 1 for success
 */
function logAttempt($success) {
    $user = $db->escape($_POST['userName']);
    $pass = $db->escape($_POST['password']);
    $session_id = session_id();
    if (!in_array($success, array(0, 1))) {
        return false;
    }
    $db->query("INSERT INTO `log_attempts` 
                    (`user`, `pass`, `ip`, `session_id`, `success`, `tried_on`)
                VALUES
                    ('$user', '$pass', '{$_SERVER['REMOTE_ADDR']}', '$session_id', '$success', NOW());
            ");
    return $db->affected_rows > 0;
}

function isAttemptsLimitExceeded() {
    $max_attempts = 5;
    $session_id = session_id();
    $res = $db->query("SELECT 
                            COUNT(*) AS cnt
                        FROM 
                            `log_attempts`
                        WHERE
                            `session_id` = '$session_id' AND
                            `tried_on` > DATE_SUB(NOW(), INTERVAL 24 HOUR) AND
                            `success` = '0';
                    ");
    $row = $db->fetch($res);
    if ($row['cnt'] >= $max_attempts) {
        return true;
    }
    return false;
}

function banFailedLogins() {
    if(isAttemptsLimitExceeded()) {
        $db->query("INSERT INTO `blacklist`
                        (`ip`, `banned_on`)
                    VALUES
                        ('{$_SERVER['REMOTE_ADDR']}', NOW()");
    }
    return $db->affected_rows > 0;
}

function clearBans() {
    $db->query("DELETE from `blacklist` WHERE `ip` = '{$_SERVER['REMOTE_ADDR']}';");
    return $db->affected_rows > 0;
}

//check for expired bans?
function canAcess() {
    $ban_expire = 24; // hours
    $res = $db->query("SELECT 
                            COUNT(*) AS cnt 
                        FROM 
                            `blacklist` 
                        WHERE 
                            `ip` = '{$_SERVER['REMOTE_ADDR']}' AND
                            `banned_on` > DATE_SUB(NOW(), INTERVAL $ban_expire HOUR);
                        ");
    $row = $db->fetch($res);
    if ($row['cnt'] > 0) {
        return false;
    }
    else {
        clearBans();
        return true;
    }
}

if (!canAcess()) {
    die("You are banned for failed login attempts");
}
  if (isset($_POST['password']) && isset($_POST['userName'])) {
    if ($_POST['password'] == "myPassword" && $_POST['userName'] == "myUsername") {
      if (!session_id()) {
        session_start();
        $_SESSION['logon'] = true;
        logAttempt(1); //log successfull attempt
        header('Location: admin_area.php');
        die();
      }
    } else if ($_POST["password"] == "" || $_POST["userName"] == "") {
      echo "Please enter both a username and a password! You are sent back in 3 seconds";
      logAttempt(0); //log failed attempt
      banFailedLogins();
      echo "<meta http-equiv='refresh' content='3;url

Context

StackExchange Code Review Q#38129, answer score: 2

Revisions (0)

No revisions yet.