Whats wrong with this php code, keep getting Error no 2?

Question

<?php 
session_start();
// After user logged in
session_regenerate_id();
$_SESSION['logged_in'] = 1;
$_SESSION['ip'] = $_SERVER['REMOTE_ADDR'];
$_SESSION['agent'] = $_SERVER['HTTP_USER_AGENT'];


// Session Checking 
function session_check(){
    if(isset($_SESSION['logged_in']) && !empty($_SESSION['logged_in'])){
         if(isset($_SESSION['ip']) && !empty($_SESSION['ip']) && ($_SESSION['ip'] == $_SERVER['REMOTE_ADDR'])){
            if(isset($_SESSION['agent']) && !empty($_SESSION['agent']) && ($_SESSION['agent'] == $_SERVER['HTTP_USER_AGENT'])){ 
                return true;
            } else {
                echo "Not allowed to view this page. Error no: 3. You will be redrected to login page in few seconds";
                header('Refresh: 3; url=./login.php');
            }   
        } else {
            echo "Not allowed to view this page. Error no: 2. You will be redirected to login page in few seconds";
            header('Refresh: 3; url=./login.php');
        }
    } else {
        echo "You are not allowed to view this page. Error no: 1. You will be redirected to login page in few seconds";
        header('Refresh: 3; url=./login.php');
        return false;
    }
}

And I keep getting error no2 when I run:

if(session_check()){ echo "something";}

Is it because I am using dynamic IP?

Is my code good enough to protect session hijacking?

If i exclude the ($_SESSION['ip'] != $_SERVER['REMOTE_ADDR']), it works perfectly..

Important Question:

What are your anti session hijacking methods? Can share with us? Using IP-checking, user-agent checking or probably other methods??

Answer 1

I'm assuming there is more happening between the setting of the variables and the checking. That is probably what is causing the problem, but it is difficult for us to say what could be causing it when we don't see any error messages or any code that might be causing it. Try echoing out what session[ip] actually is and post it here.

Answer 2

You cannot echo anything before issuing a write to header unless you use output buffering. I suggest you return a status code instead, instead of putting the header inside the session_check function. After all, it is named session_check, not session_check_redirect() :D

From the PHP manual on header()

Remember that header() must be called before any actual output is sent, either by normal HTML tags, blank lines in a file, or from PHP. It is a very common error to read code with include(), or require(), functions, or another file access function, and have spaces or empty lines that are output before header() is called. The same problem exists when using a single PHP/HTML file.

Answer 3

I don't see what's wrong with your code. As suggested, try var_dumping the content of both $_SERVER and $_SESSION at the beginning of session_check() to see what they contain.

Even if you use a dynamic IP, it should not change between two requests (it usually changes when you unplug your network cable or disconnect your wifi card).

Your method may help against session hijacking, but would not work when the attacker is behind the same public IP address as the user.

I suggest reading OWASP recommendations for best practice in web security.

Answer 4

A agree with Marius, there's probably more going on.
I've taken the liberty of making your if..else logic more readable:

function session_check(){
    if (empty($_SESSION['logged_in'])){
        echo "Error no: 1.";
        return false;
    } 
    if (empty($_SESSION['ip']) || ($_SESSION['ip'] != $_SERVER['REMOTE_ADDR'])){
        echo "Error no: 2.";
        return false;
    }
    if (empty($_SESSION['agent']) || ($_SESSION['agent'] != $_SERVER['HTTP_USER_AGENT'])){ 
        echo "Error no: 3.";
        return false;
    }

    return true;
}

Answer 5

Yes, a dynamic IP address would cause you to get logged out as a user of this code as soon as your IP address changes. You shouldn't be using the IP address to check for session security. The user agent check you already have should be enough on its own.

Here is a great article on session security: http://phpsec.org/projects/guide/4.html. Near the bottom it shows how you can make the user agent check even more secure using md5 hashing. Also here is an excerpt concerning IP addresses:

It is unwise to rely on anything at the TCP/IP level, such as IP address, because these are lower level protocols that are not intended to accommodate activities taking place at the HTTP level. A single user can potentially have a different IP address for each request, and multiple users can potentially have the same IP address.