Page 1 of 1

Sessions Are Being Hijacked... By Default?

Posted: Wed May 20, 2009 7:48 pm
by jonofalltrades666
On a site I'm running it seems that users will end up in each others' sessions, as in you can click on the home page and see "You're logged in as <someone else>"! I can't for the life of me understand why. I've tried specifying session IDs and names, and I call session_regenerate_id() on every page. I must be missing something terribly obvious. I can log on and off just fine as long as I'm the only user, but simultaneous users have been causing this problem.

Each page has this in a universal header:

Code: Select all

 
session_start();
session_regenerate_id();
 
And the login page has this (after the above code, since it also includes that .inc file):

Code: Select all

 
$userName    = $_REQUEST["userName"];
$shaPassword = $_REQUEST["shaPassword"];
$res         = Data::signIn($userName, $shaPassword);  // Verify the pwd in MySQL
if ($res instanceof User)
    {
    session_regenerate_id(true);
    $_SESSION["msg"]  = "<p>You've been signed in.  Welcome back!</p>";
    $_SESSION["user"] = $res;
    }
 
I'm sure I deserve to be laughed at, but I've gone over the PHP session handling many times, I'm reading The Truth About Sessions and trying to understand it... maybe I should stick to client-server apps.

Any help is appreciated! My temporary desperate hack is to verify the user's browser and IP address in the .inc file, which helps but is no panacea.

Thanks,

Jonathan

Re: Sessions Are Being Hijacked... By Default?

Posted: Thu May 21, 2009 1:09 am
by jaoudestudios
Try to get it working without session_regenerate_id. So remove it and see what happens and take it from there.

Start simple and build it up.

Re: Sessions Are Being Hijacked... By Default?

Posted: Thu May 21, 2009 3:49 am
by Paul Arnold
My guess is that your actual code that checks the user and password against the databse is returning the wrong id.

Re: Sessions Are Being Hijacked... By Default?

Posted: Thu May 21, 2009 9:38 am
by kaisellgren
It has probably something to do with Data:SignIn or the $res variable.

Re: Sessions Are Being Hijacked... By Default?

Posted: Thu May 21, 2009 8:52 pm
by jonofalltrades666
Wow, three replies in 24 hours! Thanks for posting, it's great to get some ideas.

session_regenerate_id() is actually a new addition, based on some of what I've been reading about security. I've tried disabling it, but only temporarily; I'll turn it off indefinitely until I get this resolved. Like you said, eliminate each possibility.

I've added a couple of extra checks to the sign in verification process, though I can't imagine how it would come up (that's exactly the point, of course!). I just now added the userName == eMailAddress comparison, maybe it will help, but adding the others have not stopped the problem.

Code: Select all

$userName    = $_REQUEST["userName"];
$shaPassword = $_REQUEST["shaPassword"];
$res         = Data::signIn($userName, $shaPassword);
if ($res instanceof User && $userName == $res->getEMailAddress())
    {
    // Store information about the user's connection; if this changes, it ain't him!
    $userBrowser = $_SERVER['HTTP_USER_AGENT'] . "uniquesalt";
    $userIP      = getenv("REMOTE_ADDR") . "uniquesalt";
    $_SESSION["userBrowser"] = md5($userBrowser);
    $_SESSION["userIP"]      = md5($userIP);
    
    $_SESSION["msg"]  = "<p>You've been signed in.  Welcome back!</p>\n";
    $_SESSION["user"] = $res;
...
This is the SignIn function, in its entirety:

Code: Select all

static public function signIn($eMailAddress, $shaPassword)
    {
    if (self::$users == null)  self::loadUsers();
    
    $eMailAddress = addslashes(strtolower($eMailAddress));
    $shaPassword  = addslashes($shaPassword);
    $sql          = "CALL SignIn('$eMailAddress', '$shaPassword', @user)";
    $outParams    = self::$db->query_sp_out($sql, array("@user"));
    $userID       = $outParams["@user"];
    if ($userID > 0)
        {
        $user = self::$users[$userID];
        if ($user->getEMailAddress() != $eMailAddress)  throw new Exception("User e-mail address does not match: found ". $user->getEMailAddress() . "; expected $eMailAddress.");
        return $user;
        }
    // Else
    return false;
    }
The SignIn sproc in MySQL is also very simple:

Code: Select all

CREATE PROCEDURE SignIn(_EMail VARCHAR(100), _SHAPassword BINARY(40), OUT _User INT)
BEGIN
    SELECT User FROM Users WHERE EMailAddress = _EMail AND SHAPassword = _SHAPassword INTO _User;
    IF (_User IS NOT NULL) THEN
        INSERT INTO UserLogIns (User, Date) VALUES (_User, NOW());
    END IF;
END$$
I thought adding this to my universal header would help, but no such luck:

Code: Select all

if (isset($_SESSION["user"]))
    {
    $userBrowser = $_SERVER['HTTP_USER_AGENT'] . "uniquesalt";
    if (getValue($_SESSION, "userBrowser") != md5($userBrowser))
        {
        // The user's web browser has changed; suspicious!
        unset($_SESSION["user"]);
        $_SESSION["msg"] = "<p>Your Web browser has changed, please <a href=\"SignIn.php\">sign in</a> again.</p>";
        }
    
    $userIP = getenv("REMOTE_ADDR") . "uniquesalt";
    if (getValue($_SESSION, "userIP") != md5($userIP))
        {
        // The user's IP address has changed; suspicious!
        unset($_SESSION["user"]);
        $_SESSION["msg"] = "<p>Your IP address has changed, please <a href=\"SignIn.php\">sign in</a> again.</p>";
        }
    }
Thanks for the replies, and if anything else comes to mind please share!

Jonathan

Re: Sessions Are Being Hijacked... By Default?

Posted: Thu May 21, 2009 9:39 pm
by Christopher
Have you gone through the code and checked each step of the way if values are what you expect they are? And I assume that you SHA in Javascript, because it is not done here (assuming the name means something).

PS - you should use the database function to escape, not addslashes().

Re: Sessions Are Being Hijacked... By Default?

Posted: Fri May 22, 2009 6:36 am
by kaisellgren
Do all users have a different ID? (Of course they should, but I'm asking you to make sure this is the case)

Type something like var_dump($user) before line 14 of your signIn function to see if the $user is always different.

Like already said, you should use mysql(i)_real_escape_string() instead of addslashes().

Bear in mind that the way you block an access for variying IPs is kind of cruel. Many people may not be able to stay logged in, because their IP changes. There are several ISPs that cycle IPs for each of their customers (AOL, some big Chinese ISPs), because they have so many users and not so many IPs. You probably should just compare the difference of the new and the old IPs. If the IP has changed too much, require re-authentication.

Re: Sessions Are Being Hijacked... By Default?

Posted: Fri May 22, 2009 8:13 am
by jonofalltrades666
Absolutely, User.User is defined as INT PRIMARY KEY AUTO_INCREMENT. I've adjusted line 14 to be more explicit:

Code: Select all

$_SESSION["msg"]  = "<p>You've been signed in as \"$res\".  Welcome back!</p>\n";
It produces the expected result (right now anyway).

I've replaced all database-related instances of addslashes() with a cleanForSQL() function abstracted in my database layer and implemented in the MySQL DB layer with mysqli_real_escape_string().

I agree regarding the IP check, it's really a desperate measure to try to eliminate this session overlap problem - though if the issue is with signing in rather than sessions, I should be able to safely drop it. I'll leave it in for now, then back it out when the bigger problem goes away. You say "If the IP has changed too much;" is it the case, then, that an AOL user may change saw, the second or third byte but that I should expect the first two to remain unchanged? Or perhaps I should see if the IP falls in a range assigned to AOL, and be more lenient if so?

I've posted these various tweaks, but the problem remains. Just now I pulled up the site, found that I'm not logged in (of course). I signed in, and line 14 above reported "You've been signed in as Jonathan, welcome back". Clicked on a members-only page, got "Hello, Judith...!" I've signed me/her out, signed back in, and got the same result. Adding the browser, IP, and user name to the page footer, temporarily, it reports my correct information but not my correct user name. Even in another browser, where the browser check should sever my session, I still get this Judith user's info immediately after signing in.

That seems to put the problem somewhere in this stretch:

Code: Select all

//SignInVerification.php
$_SESSION["msg"]  = "<p>You've been signed in as \"$res\".  Welcome back!</p>\n";  // $res is Jonathan
$_SESSION["user"] = $res;
    
$return = getValue($_SESSION, "bookmark", "Inventory.php");
if ($return == "Main.php")  $return = "Inventory.php";
// Verified $_SESSION["user"] is still as it should be here
header("location:$return");
exit;
// On the $return page, in this case Main.php, $_SESSION["user"] is "Judith", or rather a User object representing that user
I can verify $_SESSION["user"] is correct at the beginning of the Main.php page, but by the time it gets to the end it has somehow changed? The only other reference to $_SESSION anywhere else in the page uses another key: $_SESSION["msg"]. Hmm, it's a lead, I'll investigate further this evening. Thanks for your help, this has been and remains a real puzzler! I'm slightly relieved to hear that I haven't made some obvious newbie mistake... though maybe it's too soon to say that.

Jonathan

Re: Sessions Are Being Hijacked... By Default?

Posted: Fri May 22, 2009 8:33 am
by kaisellgren
Well you probably have a stupid mistake somewhere in your code... I am not spotting anything obvious, you just have to continue digging...
jonofalltrades666 wrote:You say "If the IP has changed too much;" is it the case, then, that an AOL user may change saw, the second or third byte but that I should expect the first two to remain unchanged?
Something like that. There is no specific rule that you should apply. The two first parts of IPv4 should not change... and if you want to be more paranoid you can make it tighter or even let the user decide.

You could convert the two IPs to numeric display with ip2long() and then compare like if (abs($ip-$ip2) > 255*100) where the right side represents the tightness of the protection you (or your user) choose(s).

Re: Sessions Are Being Hijacked... By Default?

Posted: Fri May 22, 2009 12:31 pm
by jonofalltrades666
I think I've found it, and indeed as you said it was a stupid mistake. One which Java or C# would have caught for me, but PHP is looser and I wasn't looking for it. I don't know that this will help anyone, it's so specific, but just in case here's my confession:

The problem had nothing to do with sessions or security, but rather the main page of the site has a segment that lists new users, and in so doing used a loop like this:

Code: Select all

for ($newUsers as $user)...
And of course, I'd been grabbing the User object from $_SESSION["user"] and storing it in a variable named $user. The loop overwrote this, though of course it did not overwrite $_SESSION["user"], so it appeared to be inconsistent. This had been rare at first, because signing in sent you to an inventory page. I never went to the main page after signing in, but other users did and got their lines crossed. Recently, I posted a change where signing in sent you back to whatever page you'd been on, meaning the main page generally, so users were being mishandled much more often.

Thanks, Kai, for helping to get me on the right track here. I owe you a beer!

Re: Sessions Are Being Hijacked... By Default?

Posted: Fri May 22, 2009 1:22 pm
by kaisellgren
jonofalltrades666 wrote:I owe you a beer!
:drunk:

Re: Sessions Are Being Hijacked... By Default?

Posted: Fri May 22, 2009 1:32 pm
by pickle
jonofalltrades666 wrote:I owe you a beer!
http://www.foamee.com/