5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review, glad you decided to post here! I hope you get some good reviews! \$\endgroup\$ – Phrancis Apr 3 '15 at 2:07
  • \$\begingroup\$ Can you post the code for this: (directly queried/set, Trusted)? \$\endgroup\$ – tim Apr 3 '15 at 8:28
  • \$\begingroup\$ done! I gave an example in the commented out code \$\endgroup\$ – Justin Apr 3 '15 at 20:32
  • \$\begingroup\$ Does anyone else have a second thought on this? I appreciate your insightful help Tim. just wanted to get a second voice on it. \$\endgroup\$ – Justin Apr 7 '15 at 15:42
4
\$\begingroup\$

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)
\$\endgroup\$
  • \$\begingroup\$ The useracctid is nothing set by the client. basically, when the login.php page checks the POST'd credentials against our login db, on success, the login db directly returns the account id. that accountid (from the credential db) is then set to the session variable. \$\endgroup\$ – Justin Apr 3 '15 at 20:36
  • \$\begingroup\$ @Justin ok then (i'm assuming it's a database managed id that can't be set by anyone, as otherwise second order injection still seems possible). I agree that if that's the case, it's probably unlikely that it changes to something dangerous in the future, but on the other hand, you never know, and prepared statements are really simple to use (and the code looks nice as well), so I don't see the downside of using them. It might also simplify (automatically) searching for injections later on if you use prepared statements (less false positives). \$\endgroup\$ – tim Apr 3 '15 at 20:53
  • \$\begingroup\$ Thanks tim, that sounds great. Yes I'm fairly certain accountid will be safe from injection. Thanks for your help with this, I am truly grateful. \$\endgroup\$ – Justin Apr 6 '15 at 21:18

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.