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"));
}
?>
1 Answer 1
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.
Explore related questions
See similar questions with these tags.
md5()
tohash('sha256', key)
. Also, I don't understand why he usedbase64_encode()
, it adds no additional security... If you go with passwords in DB, then take a look atpassword_hash()
andpassword_verify()
functions in PHP. \$\endgroup\$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\$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 usebase64_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 thepassword_hash()
andpassword_verify()
for added security. \$\endgroup\$