PHP Login/cookie authentication

I have an authetication api for an intranet site but I'm a little worried that my design of the authentication is bad and unsafe.

Below is the basic part of the authetication process and I hope I can get some overall feedback on my flaws.

```
if($_POST['authType'] == 'login'){
login($_POST['authUser'],$_POST['authPsw']);
}

if($_POST['authType'] == 'logout'){
logout($_POST['hash']);
}

if($_POST['authType'] == 'cookie'){
login('','',$_POST['hash']);
}

function login($user,$psw, $hash){

//Cookie hash login
if($hash){

//Check if hash and IP resides in authed db table

if(dbHash('CHECK', $hash)){
//IF OK set session variables with OK and DB data, return $cookie
//Expire hashes older than 30 days and return "Session expired"
$cookie['login'] = 1;
$cookie['hash'] = $hash;
dbCart('LOAD');
}
else{
//IF NOT
$cookie['login'] = 0;
$cookie['error'] = 'Session expired';
}

header('Content-type: application/json');
echo json_encode($cookie, JSON_PRETTY_PRINT);

}

//User/password login
else{
$ldapObj = authUser($user,$psw,"domain");

if($ldapObj['error']){
header('Content-type: application/json');
echo json_encode($ldapObj['error'], JSON_PRETTY_PRINT);
exit();
}

if($ldapObj['user']['samaccountname'] and !$ldapObj['error']){

$_SESSION['auth']['login'] = 1;
$_SESSION['auth']['hash'] = bin2hex(openssl_random_pseudo_bytes(16));
$_SESSION['auth']['ip'] = $_SERVER['REMOTE_ADDR'];
$_SESSION['auth']['user'] = $ldapObj['user'];

$cookie['hash'] = $_SESSION['auth']['hash'];
$cookie['user'] = $_SESSION['auth']['user'];

//Save session in DB with hash, IP and session_id.
dbHash('ADD');
dbCart('LOAD');

header('Conten

Security

You have an auth type that is called cookie, but which doesn't work on cookies, which is odd. Apart from the added complexity (you have to add it to every form yourself), this is also bad as you can't make POST data httpOnly. It also doesn't make any sense to me (it doesn't work as a remember me token or anything).

It's also very unclear to me how you match a "cookie" to a specific user, as it's just random data.

If you deviate from the standard approach of sessions and remember me cookies, you should really properly document why and how exactly your mechanism works.


There is no proper CSRF at the moment but site is only available on our internal network until I cant trust the authentication and arrange an ssl certificate. It is one reason why I added the requirement to match both cookie hash and client ip.

As long as you have access to it, it doesn't matter that it is internal, it can still be exploited. The IP binding also does not prevent CSRF exploitation (as the request is send by you, not by the attacker).

But seeing that the cookie hash isn't actually a cookie (or a hash), it seems to serve as a CSRF token (if it needs to be send for each request, which I'm not quite sure if it is).

Structure

I like that you have designated functions for login and logout, it makes the code nicer to read. But I wouldn't stop there. At the very least I would split login into loginWithCookie and loginWithCredentials. You might also want to think about using some generic function/class to send json, to avoid the duplication of header / json_encode / die.

dbHash

Your dbHash method isn't very well named: the name doesn't really tell me what it is doing (also, it isn't a hash; token might be more fitting).

It is also not all too well designed: I have to pass a string like ADD to it to tell it what to do, which means I cannot use it without reading the documentation or looking at the implementation. Instead, it would be better to have some HashDAO class, which then has methods like add(), delete(), etc.

It is also very confusing that I don't have to pass additional data when ADDing. dbHash('ADD') doesn't tell me anything. $hashDAO->add($hash, $ip, $session_id) would tell me exactly what is happening.

Misc

• I already mentioned it above, but your naming really hurts readability (which makes it more difficult to look for security and other issues). The main problem is with hash (which isn't a hash, but a token) and cookie (which isn't a cookie, but also a token). Some other names are also not ideal such as data (too generic, might be username) or output (better: isAuthenticated).

• You always assign your variables after binding them. I think your code would be more readable the other way around.

• Your auth table contains values which you do not seem to use such as sid, which adds unnecessary complexity.

StackExchange Code Review Q#132726, answer score: 3