1
\$\begingroup\$

I am reviewing previous undocumented php work from a predecessor on these two scripts to manage access to an administrative dashboard, but I'm not sure about vulnerabilities and other things that usually go unnoticed. I tried to improve on his design and this is the result.

Is this login check secure?

<?php
session_start();

$check1 = (empty($_SESSION) || $_SESSION['admin'] != true);
$check2 = ($_SESSION['NO_HIJACK'] == (
        ($_SERVER['HTTP_USER_AGENT'] ?? '?') . '+' .
        ($_SERVER['REMOTE_ADDR'] ?? '?')));

if ($check1 || $check2) {
    http_response_code(401);
    header("Location: http://admin.example.com");
    exit(0);
}

?>

This is the verification script that receives the login key from the login form with a jquery post:

<?php
if (!empty($_POST) && isset($_POST['key'])) {
    $controlKey = array();
    $controlKey[] = 'token1';
    $controlKey[] = 'token2';
    $controlKey[] = 'token3';
    $hKey = hash('sha256', $_POST['key']);
    if (in_array($hKey, $controlKey)) {
        session_start();
        session_regenerate_id(true);
        $_SESSION['admin'] = true;
        $_SESSION['NO_HIJACK'] = (($_SERVER['HTTP_USER_AGENT'] ?? '?') .
            '+' . ($_SERVER['REMOTE_ADDR'] ?? '?'));
        echo json_encode(array("status" => "success"));
    } else {
        echo json_encode(array("status" => "fail"));
    }
} else {
    echo json_encode(array("status" => "empty"));
}
?>
\$\endgroup\$
10
  • 2
    \$\begingroup\$ Firstly: this code is a big NO, its so prone to session hijacking, its not even funny. For the password check itself: if you are not going to enable admins to change their passwords (by storing then in DB) in future (which would require rework of the whole code), then I would at least change md5() to hash('sha256', key). Also, I don't understand why he used base64_encode(), it adds no additional security... If you go with passwords in DB, then take a look at password_hash() and password_verify() functions in PHP. \$\endgroup\$ Commented Jan 6, 2017 at 13:34
  • 1
    \$\begingroup\$ @insertusernamehere yes, that is an option, but you would have to go through an array and check every password, and password_verify() isn't that fast. But for administration (where optimizing every little thing usually doesn't matter that much), it is an viable option. \$\endgroup\$ Commented Jan 6, 2017 at 13:56
  • \$\begingroup\$ Thanks, I'm going to fix these things and update my question. For base64 I guess he wanted to store the keys in the database. Also the key array maximum size won't exceed 10. \$\endgroup\$ Commented Jan 6, 2017 at 14:02
  • 1
    \$\begingroup\$ @beppe9000 but you do not need base64_encode() to store passwords in database... Again, it doesn't really do anything for you. And if somebody got to that password, he could just use base64_decode(). One thing with this implementation is, anybody can use any password. Are there different users, or just single user and admins themselves have different passwords for that single user? Ps.: With only 10 passwords max, I would use the password_hash() and password_verify() for added security. \$\endgroup\$ Commented Jan 6, 2017 at 14:03
  • 1
    \$\begingroup\$ @beppe9000 you are doing something similar to me on one of my websites, but I am still checking the user name (from DB) against their password (and only theirs). With this code, user123 can login using password from user456 or root and that is not good. \$\endgroup\$ Commented Jan 6, 2017 at 15:55

1 Answer 1

Reset to default
1
\$\begingroup\$

Security

First of all, the things you are doing right which are often missed: You die after a redirect (otherwise there wouldn't actually be any protection) and you regenerate the session id (which can prevent/mitigate session fixation).

Hashing

You shouldn't use sha256, as it's rather fast. Instead, use bcrypt (password_hash and password_verify).

Session Highjacking

Binding the session to the IP is a controversial measure, as it can negatively effect usability. If it makes sense in the context of your application it's a good measure though.

Binding to the user agent does not have the same downside. But note that the user agent can relatively easily be bruteforced, so you should definitely terminate the session if a violation is found.

General Approach

About the general approach: I don't like the control key mechanism with multiple keys which are independent of user accounts, as it increases the probability that a key is bruteforced. On the other hand, I assume that you can't just use one shared key, as then you couldn't revoke admin privileges of just one user.

If this is about a sort of second password to elevate to special privileges, I would suggest a proper privilege management that is bound to specific user accounts (ie you have non-admin and admin users, which may have a secondary password to access more privileged actions).

Hardcoded Credentials

I'm not a fan of hardcoding credentials like this (it makes them difficult to change by a user, source code is more likely to be shared than database content, etc), but if you need to do it, try to store them in a specific config file which doesn't contain any other code (ideally outside of the web root). That way it's easier not to leak them in version control etc.

Other

Your code doesn't contain bruteforce protection or session expiration, but I'm assuming that that is handled somewhere else.

Misc

  • check1 and check2 aren't very good variable names. isAdmin and isHijacked would be better.
  • if you reverse your first if (and maybe also the second one), it's easier to see what happens in what case.
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.