3
\$\begingroup\$

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('Content-type: application/json');
            echo json_encode($cookie, JSON_PRETTY_PRINT);
        }
        else{
            unset($_SESSION['auth']);
        }
    }
}


function logout($hash){

    unset($_SESSION['auth']);
    unset($_SESSION['cart']);
    dbHash('DELETE', $hash);

}

Below is the dbHash function. All of the backend authentication should now be posted except my common functions such as database connection etc and the ldap library.

include "common.php";
include "ldap.php";

session_start();

/****************************************************************************
Authentication is divided in thress parts, 1=Login, 2=Logout and 3=DB management
Login can be done by HASH or user/password. For user it connects to LDAP annd for
HASH the local DB.
HASH login requires HASH code and same IP as when created. If not then its deleted.

SESSION variables decide on logged in or not in site.

DB management is set to 3 subfunctions, 1= ADD new hash, 2= CHECK if has is valid
3= DELETE hash


3= Checks for expired hashes and deletes them every time its accessed.
User LDAP information is stored in json format in DB in case site need the information.

*****************************************************************************/

function dbHash($task, $input){

    //Add hash to DB
    //Fix data checks
    if($task == 'ADD'){

        dbHash('DELETE');

        $con = local_pdo();
        $stmt = $con->prepare("INSERT INTO `auth`(`hash`, `ip`, `create`, `expire`, `data`, `session_id`) VALUES 
                                (?,?,?,?,?,?)");

        $stmt->bind_param("ssssss", $hash,$ip,$create,$expire,$data,$sid);

        $date = new DateTime();
        $date->modify('+30 days');

        $hash = $_SESSION['auth']['hash'];
        $ip = $_SERVER['REMOTE_ADDR'];
        $expire = $date->format('Y-m-d H:i:s');
        $data = json_encode($_SESSION['auth']['user']);
        $sid = session_id();

        $stmt->execute();
        $stmt->close();
    }

    //Delete hash in DB
    if($task == 'DELETE'){
        $date = new DateTime();
        $date->modify('-30 days');

        $con = local_pdo();
        $stmt = $con->prepare("delete from auth where (hash = ? or expire < ?)");
        $stmt->bind_param("ss", $hash, $expire);
        $hash = $input;
        $expire = $date->format('Y-m-d H:i:s');
        $stmt->execute();
        $stmt->close();

    }

    //Check hash in DB
    if($task == 'CHECK' && $input){
        $date = new DateTime();
        $date->modify('-30 days');

        $con = local_pdo();
        $stmt = $con->prepare("select hash,ip,data from auth where hash = ? and ip = ? and expire > ?");
        $stmt->bind_param("sss",$value,$ip,$expire);
        $value = $input;
        $ip = $_SERVER['REMOTE_ADDR'];
        $expire = $date->format('Y-m-d H:i:s');
        $stmt->execute();

        $stmt->bind_result($hash, $ip, $data);
        $stmt->fetch();

        if($hash){
            $_SESSION['auth']['login'] = 1;
            $_SESSION['auth']['hash'] = $hash;
            $_SESSION['auth']['ip'] = $_SERVER['REMOTE_ADDR'];
            $_SESSION['auth']['user'] = json_decode($data,true);

            $output = true; 
        }
        else{
            $output = false;
            dbHash('DELETE', $input);
        }

        dbHash('DELETE');
        $stmt->close();

        return $output;

    }

}

There is no proper CSRF at the moment but site is only avalible on our internal network untill 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.

I'm probably in over my head with this but you got to try :)

\$\endgroup\$
3
\$\begingroup\$

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.
\$\endgroup\$

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.