For a login portal, what security measures are needed to prevent unauthorized access?

Question

I'm designing a login portal that has one angularjs page that displays/processes data queried from a database. I'm relying on a few php pages (a loginpage.php [verifies credentials/loads session variables], a check.php [page that is included in all php pages, verifies session variables are 'okay'], and a getter.php [queries database/returns json] page) to do handle the getting/setting of data for my angularjs SPA.

I did a lot of reading into session variables, security, sql-injection, etc. to prevent users from gaining access to additional unauthorized data from my database.

While I know it is hard to say that methods are 100% secure, I was wondering if someone could give me some insight into whether or not there are any severe gaps/holes in my session security logic.

login.php (abbreviating code, obv session_start is in the beginning of all php pages)

after verifying credentials, the following session variables are set:

//example of (directly queried/set, Trusted) - I directly query a login db,          
//which returns a data set of fields. I take the $response and directly set
//the session variables like so: $_SESSION['useremail']= $query->record[0]->Email;

$_SESSION['user'] = $username;
$_SESSION['useracctid'] = (directly queried/set, Trusted)
$_SESSION['userisauthenticated'] = true;
$_SESSION['timeactive'] = date("Y-m-d H:i:s", strtotime("+20 minutes"));
$_SESSION['ip'] = $_SERVER['REMOTE_ADDR'];
$_SESSION['count'] = 5;

check.php

<?php 
session_start();
function checksession() {
    if(isset($_SESSION['user']) && $_SESSION['userisauthenticated']) {
        if($_SESSION['ip'] != $_SERVER['REMOTE_ADDR']) {
            session_destroy();  //die;
            header("Location: https://www.myloginpage.com");
            exit;
        }               
        //rotate the session_id on each 5 requests. That way, the session_id theoretically doesn't stay long enough to be hijacked.
        if(($_SESSION['count'] -= 1) == 0) {
            session_regenerate_id();
            $_SESSION['count'] = 5; 
        }
        //times out after 20 minutes of inactivity.
        if($_SESSION['timeactive'] < date("Y-m-d H:i:s")) {
            session_destroy(); //die; is exit needed after header?
            header("Location: https://www.myloginpage.com");
            exit;
        }
    } else {
        //should I do more?     
        session_destroy();
        header("Location: https://www.myloginpage.com");        
        exit;
    }
    //adds 20 to the timeout
    $_SESSION['timeactive'] = date("Y-m-d H:i:s", strtotime("+20 minutes"));

}   ?>

getter.php (abrv. code, verifies session credentials, then queries db and returns data for angularjs SPA) The angularjsview.php page passes the $_SESSION['useracctid'] as a ng-init to the controller, and the controller posts (via angularjs' $http) the useracctid to the getter.php page.

<?php 
session_start();
require_once(check.php)
checksession();

//gets post data, which has acctid
$data = json_decode(file_get_contents('php://input'),true);
if(!empty($data)) {
    checksession();

    $useracctid = $data['useracctid'];

    // makes sure that id is 10 characters with no special cases
    $useracctid = clean($useracctid);
    $useracctid = substr($useracctid, 0, 10);

    if($_SESSION['useracctid'] != $useracctid) {
        session_destroy(); //die;
        //if user tries to get access to this page without posting the account data, it destroys session 
        header("Location: https://www.myloginpage.php");
        exit;
    } else {
        echo getjson();
    }
}
function getjson();
    $query = SELECT * FROM STUFF WHERE Acct = $_SESSION['useracctid'];
    //code here that queries db, structures it and returns json
?>

With my security logic, can I 'assume' that even if a malicious user authenticates, he/she cannot get more information than the jeopardized user account? Am I missing a security check or have any huge exploits to this logic? Any help would be appreciated, this has been very fun to learn! If this is not how SOverflow questions should be asked, please let me know.

Answer 1

Your session logic seems good to me.

Prepared Statements

Use prepared statements.

It's unclear where $_SESSION['useracctid'] comes from, but it is placed directly inside the query, which is a bad idea. It might have been cleaned previously (currently, it's cleaned indirectly, via substr and clean on $useracctid, but you didn't give that function), or it might not have, or it currently is, but your code changes, and it's not cleaned in the future.

Or, as it's stored in a function, it is currently cleaned by the caller, but a different script also needs this function and doesn't clean it. You just don't know, and prepared statements are the recommended solution to prevent SQL injection, so they should be used.

Misc

• use session_regenerate_id(true); to not only change the session id, but to destroy the previous session file.
• It's good to regenerate the session id periodically, but you should also always call session_regenerate_id(true); when the session state changes, eg when logging in (less important today, as use_trans_sid is set to 0 by default, and thus session fixation is less of a problem, but still).
• binding the session to the IP is a pretty good security mechanism, but it could possibly inconvenience users whose IP changes often for legitimate reasons (eg using a cellphone). A less secure, but more user oriented approach might be to bind it to the user agent.
• call checksession() directly in check.php. There doesn't seem to be a situation when it will not be called, and when including check.php, it is easy to forget that call.
• is exit needed after header?: Yes, it is. see also here for consequences of not doing it (although it's mostly a lesson about following REST and having CSRF protection)