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

jmkeyes · Answer 2 · 2013-08-31T00:32:03.533

0

Using the incorrect comparison operators can also be a vulnerability in your code. As a rule, I would always suggest using the identical comparison operators instead of the equivalent comparison operators. (eg. !== instead of != or === instead of ==)

edited Aug 31 '13 at 00:32

answered Aug 30 '13 at 18:38

jmkeyes

3,739
16
19

I don't see why OP would need to micro-optimize things like this for less than a millisecond less loading time. This should be a comment too. http://stackoverflow.com/questions/16633947/in-javascript-is-it-better-to-use-two-equal-signs-or-three-when-comparing-varia/16633974#16633974 – Jordan Doyle Aug 30 '13 at 18:42
It's not about micro-optimization at all -- PHP has obscure, counter-intuitive methods of comparing for equality between types that can trip up a lot of people. – jmkeyes Aug 30 '13 at 18:44
This is a comment not an answer. – Jordan Doyle Aug 30 '13 at 18:46
How so? If I exploit his website by possible his misuse of comparison operators, it definitely shows a vulnerability in his code. – jmkeyes Aug 30 '13 at 18:47
Please explain, how are you going to exploit his website by him using a comparison operator that you don't like? There's not one exploit that comes to mind when I think about exploitation of comparison operators. Comparison operators always check that the left side is identical to the right side first, in the OP he's only checking `string = string`. – Jordan Doyle Aug 30 '13 at 18:58
It's a potential vulnerability because of PHP's type-coercing comparison operators. For example, during a pen test of a different website, I've bypassed an access restriction by using an integer-prefixed string, which PHP automatically type coerced into an integer and allowed me to access records I didn't have explicit access to. – jmkeyes Aug 30 '13 at 20:35
Which means someone isn't validating inputs which is nothing to do with equality operators. – Jordan Doyle Aug 30 '13 at 20:37
The use of incorrect comparison operators, especially in PHP, as part of validating input **is** a vulnerability. – jmkeyes Aug 30 '13 at 20:42
@JoshuaK: You should explain that in your answer. Without that explanation, Jordan is correct in that this should be a comment. – Chris Laplante Aug 30 '13 at 21:10

score 0 · Answer 3 · 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?

3 Answers3