PHP and MYSQL: Is my system safe?

Question

I’ve heard that people can edit cookies and change their name and access level (etc.), so I coded a simple PHP code to prevent them from editing their user’s properties (such as access level). Here it is:

doConnect();

$currentIP = $_SERVER['REMOTE_ADDR'];

$page = "geton";

$AHQuery = mysql_query("SELECT * FROM users WHERE user='{$_SESSION['uName']}' ORDER BY id DESC");

while($AHLine = mysql_fetch_array($AHQuery)) {

    $trueIP    = $AHLine['ip'];
    $trueLevel = $AHLine['acesslevel'];  

}

if($currentIP != $trueIP || $_SESSION['uAcessLevel'] != $trueLevel) {

    echo "<script>alert('Please, login again.'); location.href='{$page}'</script>";
    exit;
}

The code above checks if the session user (X USER) is a valid name and if it equals to the latest X USER’s ip (when you login your ip is gathered and saved in the users table), if it doesn’t then the session is destroyed and the user is forced to login again. Anyway, my question is: is this method safe, does it really prevent people from viewing private pages and commenting as another user? (this function is in every single page of my php forumblog) Is there a better and safer way to do this?

I tried to be as clear as possible, hope you guys understand it, thanks for your attention.

Any time you use `mysql_query` it is NOT safe. Use [PDOStatement](http://www.php.net/manual/en/class.pdostatement.php) instead — Matt Dodge, Aug 30 '13 at 18:01
Session data is stored on the server, not in cookies, so the user can't change it. On the other hand, IPs can change during a session. — Barmar, Aug 30 '13 at 18:02
-Barmar Thanks, I didn’t know that! @mattedgod can you give me an example? — John Vergara, Aug 30 '13 at 18:03
What happens when two users share the same IP address (i.e, they're coming from behind a firewall)? — Dan Pichelman, Aug 30 '13 at 18:04
@JohnVergara he already gave you check the link on this comment. — Prix, Aug 30 '13 at 18:08
@mattedgod: Don't be so dramatic. You can use `mysql_query` and be safe. — Lightness Races in Orbit, Aug 30 '13 at 18:20
@LightnessRacesinOrbit and then we come across the problem of the function actually being deprecated. We're never going to rid the streets of that though, I still see people using `ereg` — Jordan Doyle, Aug 30 '13 at 18:24
@Jordan: Okay, but `mysqli::query` is not deprecated, is not going to be at any point in the future, has largely identical semantics to `mysql_query` and is perfectly safe _in its own right_. If we're going to make arguments against `mysql_query` let's make it based on the deprecation, else make arguments against failing to sanitize inputs. Note that I am _not_ claiming anyone should prefer mysqli to PDO with prepared statements: that is of course the optimal approach! :) — Lightness Races in Orbit, Aug 30 '13 at 20:20
I'm not starting an argument here, I was just pointing out it was deprecated, I agree with your statement about @mattedgod being dramatic — Jordan Doyle, Aug 30 '13 at 20:40

Jordan Doyle · Accepted Answer · 2013-08-30T19:01:13.077

Your script is secure and protects against basic session hijacking attempts, too.

You should consider checking by a /24 mask (and maybe add some more checks, HTTP_USER_AGENT checking, cookie checking, etc) - this will allow people with dynamic IPs to access your site, too.

You do not need to check if the $_SESSION variable has been modified by the user as it's not possible (unless they have access to the server, and you're not doing mass-assignment from $_POST or something stupid like that)

Note: mysql_* functions are deprecated since PHP 5.5. You can switch to MySQLi or PDO.

In reply to your comment, it's not entirely impossible for two users to have the same IP, dynamic IPs (which has been getting more and more popular in ISP-provided routers due to the IPv4 exhaustion) and NAT (which has also been getting more and more popular due to the same reason) can be two causes of this.

I've gone out of my way to provide an example PDO statement for you to work from:

$pdo = new PDO('mysql:dbname=session_test;host=127.0.0.1', 'root', '');

$sth = $pdo->prepare("SELECT * FROM users WHERE username = ? LIMIT 1");
$sth->bindParam(1, $_SESSION['username']);
$sth->execute();

$user = $sth->fetch(PDO::FETCH_OBJ);

if($user->ip !== $_SERVER['REMOTE_ADDR']) {
    exit("Session hijacking attempt found");
}

As Cloudflare is getting increasingly popular I'll throw in a recommendation to use $_SERVER['CF_CONNECTING_IP'] on the condition that $_SERVER['REMOTE_IP'] is one of Cloudflare's IPs.

Not to mention many homes nowadays have wireless routers, which means it could be one person with 3+ devices, or 10 people with 10+ devices sharing the same external IP. — aynber, Aug 30 '13 at 18:21
Just adding a bit more of a note, since it's not always about IPv4 exhaustion. :-D — aynber, Aug 30 '13 at 18:29

score 0 · Answer 2 · edited Aug 30 '13 at 19:07

Your script and logic looks safe and secure.

You still need to verify if you are able to get the actual IP using $_SERVER['REMOTE_ADDR']. If your server is behind a loadbalancer or proxy the remote add will be the loadbalancer or proxy IP and in that scenario the script will fail to do its intended job.

For this script to work as designed the environment should be configured to always get the right IP address.

PHP and MYSQL: Is my system safe?

2 Answers2