FRIHOST • FORUMS • FAQ • TOS • BLOGS • DIRECTORY
You are invited to Log in or Register a Frihost Account!

Is this code safe, or not?

   Frihost Forum Index -> Scripting -> Php and MySQL
 


Marston
Is this code in unsafe? I'm pretty new to MySQL, and I don't want my site to get hacked... (I know I haven't connected to the db in the script- that's on purpose).

Any help would be greatly appreciated.
Code:
<?
require_once('/path/to/database/connect/file.php');
mysql_select_db("marston_memsys");
$result = mysql_query("select * from members");
$r=mysql_fetch_array($result);
$userid = $r["user_id"];
$username = $r["username"];
$regdate = $r["user_regdate"];

switch($mode) {
default:
echo "Sorry! No content here...";
break;
case "view":
 switch($id){
 default:
 echo "non-existant member";
 break;
 case $userid:
 echo '<a href="'.$userid.'">'.$username.'</a> - Member since: '.$regdate;
 break;
 }
break;
}
?>

It's for a custom built member system. This particular script is to fetch and display information on members. The profile is accessed by going to profile.php?mode=view&id=64. I know that the script works, I just want to know if it's hackable.
Kaneda
I'd say it's as safe as it gets, since you're not using any user-supplied values in your SQL statements. However, are you SURE it works? Wink You're selecting all your members, but only fetching the first row (member), so your case statement would branch to non-default if the first user in your members table has the supplied user_id.

The usual way to do it would be:

Code:
mysql_query("SELECT * FROM members WHERE user_id = $id");


However, in that case, you'd need to ensure $id is a number with is_numeric (if user_id IS a number) - otherwise, the user could enter, for example

Code:
1 OR 0 = 0


... which would turn the query into:

Code:
SELECT * FROM members WHERE user_id = 1 OR 0 = 0


... which would select all users (not so bad in this case). With various trickery he could insert any kind of SQL code into your query.

If user_id is a string, you'd add singlequotes around $id in the query:

Code:
mysql_query("SELECT * FROM members WHERE user_id = '$id'");


... and you'd call mysql_real_escape_string(). Before that call, you'd have to call stripslashes(), if magic_quotes is on. When magic_quotes is on, PHP basically automatically calls addslashes() on all POST/GET variables, which is not enough - and since mysql_real_escape_string() also adds slashes before quotes, you'd get twice as many slashes as you want Wink.

You can check if magic_quotes is on with the function get_magic_quotes_gpc().

Without mysql_real_escape_string() (and magic_quotes), again, the user could enter something like:

Code:
1' OR '' = '


... escaping out of (and back into) the string, and allowing him to add arbitrary SQL code inbetween:

Code:
SELECT * FROM members WHERE user_id = '1' OR '' = ''


So, the full call would be something like:

Code:
if (get_magic_quotes_gpc()) {
  $id = stripslashes($id);
}
$id = mysql_real_escape_string($id);
$result = mysql_query("SELECT * FROM members WHERE user_id = '$id'");


Anyway, any security considerations to do with MySQL mainly enter when you include user-supplied input into your queries, which you don't in this case, whether it works or not Wink

Another way to improve security would be to enforce permissions on the database level. So, if your users are only allowed to browse data, but not modify and delete it, set up mySQL to only allow the website user to do SELECT on your tables. Or make a user allowed to DELETE/UPDATE/INSERT, which is only used in the places where it's necessary.
AftershockVibe
It looks safe to me and serge has given you a few tips which could enhance things even further if you really think it's necessary.

I have, however, found a bug in your code:
Code:
switch($mode){
default:
echo "Sorry! No content here...";
break;
case "view":
 switch($id){
 default:
 echo "non-existant member";
 break;
 case $userid:
 echo '<a href="'.$userid.'">'.$username.'</a> - Member since: '.$regdate;
 break;
 }
break;
}


Default cases must be at the end (otherwise they will always be executed) and I think you've misplaced a curly brace ("}"). I am also unsure where your $id and $mode variables are coming from - Required file?
You'll want it to look something like this;
Code:

switch($mode)
{
case "view":
     switch($id)
     {
          case $userid:
               echo '<a href="'.$userid.'">'.$username.'</a> - Member since: '.$regdate;
               break;
         
          default:
               echo "non-existant member";
               break;
     }

default:
     echo "Sorry! No content here...";
     break;
 }
hexkid
AftershockVibe wrote:
Default cases must be at the end (otherwise they will always be executed)


Sorry, that's not true.
Code:
~$ cat ex.php
<?php
$x = 2;
switch ($x) {
default: echo 'default'; break;
case 2: echo 'two'; break;
}
echo "\n";
?>

~$ php ex.php
two
If you replace $x by 1 the result is "default", as expected.
Kaneda
AftershockVibe wrote:
It looks safe to me and serge


I'm so happy I'm still Serge Wink

Quote:
I am also unsure where your $id and $mode variables are coming from - Required file?


Or the evil register_globals. Wink
Marston
Quote:
I am also unsure where your $id and $mode variables are coming from - Required file?
They don't actually exist, but it's how you make query strings in PHP... Didn't you know that?

Kaneda: Could you dumb down your post a bit for me Razz. I'm not at that level of PHP H4XX0RZ11 yet.. Wink
Kaneda
Marston wrote:
They don't actually exist, but it's how you make query strings in PHP... Didn't you know that?


Actually, they're the evil, "hacking-prone" way to access query strings in PHP Wink But if the server admin has turned support for doing it that way (register_globals) on, there's not much you can do about it. The proper way to use query strings is like this:

Code:
$id = $_POST['id']; // for POST parameters


or

Code:
$id = $_GET['id']; // for GET parameters (http://url?id=5)


The reason the transparent use of parameters (i.e., auto-converting http://url?id=5 to $id = 5) is a security liability is that the user is actually able to modify every single uninitialized variable in your script.

I.e., some developer writes a script like this:

Code:
<?php
// The following out-commented line fixes the problem with this script:
// $admin = false;

if (checkLogin($username,$password)) {
  $admin = true;
}

if ($admin) {
  doAdministrativeStuff();
}
?>


The way this script is supposed to be called is like: http://www.test.com/login.php?username=kaneda&password=mypassword

If the username and password is right (checked by checkLogin), $admin is set to true, otherwise it remains unset. So, the later function (doAdministrativeStuff) will only be called if the username and password is correct - or so the developer thinks...

However, the hacker can call it like this instead:
http://www.test.com/login.php?admin=1

And since all query variables are converted into PHP variables automatically, that actually makes the second "if" in the code above evaluate to true, and doAdministrativeStuff() is called - without supplying any username or password.

register_globals is evil. You might as well get used to the $id = $_GET['id'] way of doing things, since most PHP servers turn register_globals off for this reason Smile

On a server where register_globals is on, you have to make sure that every important variable you use is always initialized. Otherwise, the hacker will initialize it for you Wink

Quote:
Kaneda: Could you dumb down your post a bit for me Razz. I'm not at that level of PHP H4XX0RZ11 yet.. Wink


Sorry, for some reason I assumed more than I should Wink

There were two main points...

1. One was that I don't actually think your script works...

You're querying for all records from the members table (SELECT * FROM members). However, you're only fetching one of those records from the result - the first one:

Code:
$r=mysql_fetch_array($result);


So, when you're later checking $userid against $id, you're just checking if the user-supplied id is same as the userid of the first record of your members table. I.e., if the user queries for the second member in the table, the script won't work.

Then I suggested the usual way to do such a query:

Code:
SELECT * FROM members WHERE user_id = $id


That only fetches one record - the one the user asked for (by way of the query variable - http://url?id=64, for example would turn into: "SELECT * FROM members WHERE user_id = 64", and the database does the work for you - more efficient than you going through all the returned records looking for the right one - let the database do the searching, that's what it was invented for).

2. The second point was that security considerations mainly enter when you're adding user-supplied variables (like $id in my example above) to the query. Then you need to be careful testing the data. If the supplied variable is supposed to be a number, all you need to do is check that it actually is, like this:

Code:
if (!is_numeric($id)) die('Invalid userid');


If it's supposed to be a string, you need to make sure the string can't include additional SQL that will be executed. You do this by way of the mysql_real_escape_string() function.

However, PHP includes a configuration parameter - magic_quotes - which, if on, does half the job of mysql_real_escape_string() already. It adds '\' in front of all quote characters in strings passed through querystring etc. exactly in order to make their usage in database queries safer (and make innocent querystring parameters containing quotes actually WORK in SQL queries). However, "half the job" isn't enough. So magic_quotes is not really a help, but rather a nuisance - because mysql_real_escape_string() does the same - and more. An example of where the problem enters:

The user wants to search for "Soldier's Poem". In your PHP, you do this by:

Code:
mysql_query("SELECT * FROM songs WHERE title = '$searchquery'");


However, the user's query would turn this line into (not in "code" tags, because we need to make some stuff bold):

mysql_query("SELECT * FROM songs WHERE title = 'Soldier's Poem'");

Note that the bold part is actually outside the string, and that the ending singlequote (the last character in the query) has turned into a starting singlequote. In this case it will simply cause an SQL syntax error. But a hacker can use it to execute a lot of nasty code he himself made up by, for example, passing "Soldier'; DELETE FROM songs --". This turns into:

Code:
mysql_query("SELECT * FROM songs WHERE title = 'Soldier'; DELETE FROM songs --'"); // Won't actually work in PHP, just an example.


So, other than getting back a song with the title "Soldier", he wipes out all songs from your table. He could put anything after the semicolon (except, again, that won't work in PHP, which only accepts one SQL statement at a time in mysql_query for this very reason - there's other ways, though Wink). The "--" bit is the start of a comment in SQL, which makes SQL not throw up a syntax error due to the remaining singlequote at the end of the query.

The way magic_quotes tries to fix this is to add '\' in front of all quotes in strings sent in via querystrings etc.:

mysql_query("SELECT * FROM songs WHERE title = 'Soldier\'s Poem'");

That way, a singlequote won't escape out of the string and everything works. However, there are other ways to escape, and those are taken care of by mysql_real_escape_string(). But but but... If magic_quotes is on, calling mysql_real_escape_string() will add yet another set of '\' to the string:

mysql_query("SELECT * FROM songs WHERE title = 'Soldier\\'s Poem'");

... which will go horribly wrong. So, if magic_quotes is on, you first need to strip the slashes it might have added, turning the full call into:

Code:
if (get_magic_quotes_gpc()) {
  $searchquery = stripslashes($searchquery);
}
$searchquery = mysql_real_escape_string($searchquery);
$result = mysql_query("SELECT * FROM songs WHERE title = '$searchquery'");


That's the same code as supplied in the previous post for your own example. Smile

Hope this was more clear. Otherwise, ask Wink
Marston
Hmm, so how would I make the script work?
Kaneda
Marston wrote:
Hmm, so how would I make the script work?


Assuming you want the info on a single user, based on the id supplied through the querystring, and assuming the user_id field is numeric, I'd do something like this:

Code:
<?
require_once('/path/to/database/connect/file.php');
mysql_select_db('marston_memsys');

// Get querystring variables:
$mode = empty($_GET['mode']) ? 'view' : $_GET['mode']; // default: view
$id = empty($_GET['id']) ? -1 : $_GET['id']; // default: -1

switch($mode) {
  case 'view':
    // Ensure numeric, positive user-id:
    if (!is_numeric($id) || $id < 0) {
      die('invalid user-id');
    }

    $result = mysql_query("SELECT * FROM members WHERE user_id = $id");

    // Check if we got any rows back:
    if (mysql_num_rows($result) < 1) {
      die('non-existant member');
    }

    $r=mysql_fetch_array($result);
    $username = $r['username'];
    $regdate = $r['user_regdate'];
    echo '<a href="' . $id . '">' . $username . '</a> - Member since: ' . $regdate;
    break;

  default:
    echo 'Sorry - no content here';
    break;
}
?>


(untested)
Marston
Ah, works like a charm! Thanks so much.

Embarassed I don't suppose you'd explain how the script works? (Just incase I need to replicate it for something else)...

If you don't want to explain, you don't have to, but I just wanted to know.

Thanks again.
Marston
I made something based on your code, kandea:

Code:
<?
define( 'parentFile' , 1 );
include ("/home/marston/domains/xzact.frih.net/public_html/assets/includes.php");

mysql_connect("localhost", "xxxx", "xxxx") or die("cannot connect");
mysql_select_db('marston_memsys') or die("");

    // Ensure numeric, positive key:
/*    if (!is_numeric($key)) {
      die(redirect("http://xzact.frih.net/error.php?id=wrong-activation"));
    }
    elseif (strlen($key) > 18 ) {
      die(redirect("http://xzact.frih.net/error.php?id=wrong-activation"));
    }
    elseif (strlen($username) < 6) {
      die(redirect("http://xzact.frih.net/error.php?id=wrong-activation"));
    }
    elseif (strlen($username) > 25) {
      die(redirect("http://xzact.frih.net/error.php?id=wrong-activation"));
    }
    else { */

$key = $_GET['key'];
$username = $_GET['username'];

$query = mysql_query("SELECT user_activationnumber FROM members WHERE username = $username") or die("Error, cannot select user_activationnumber");
$row = mysql_fetch_array($query);

if ($row['activation_code'] == $key)
{
    mysql_query("UPDATE members SET active=1 WHERE username = $username LIMIT 1") or die("Error cannot update db");

    echo 'You have successfully activated your account!';
}
else
{
    echo 'That is the wrong activation code.';
}
?>
But it gets caught up when it tries to select user_activationnumber....

Don't suppose you could help me again? Pretty pretty please?

Last edited by Marston on Sat Jul 08, 2006 9:33 pm; edited 1 time in total
Kaneda
Marston wrote:
But it gets caught up when it tries to select user_activationnumber....

Don't suppose you could help me again? Pretty pretty please?


Since username is likely a string, the query should have quotes around that bit:

$query = mysql_query("SELECT user_activationnumber FROM members WHERE username = '$username'") or die...

... and then you should also remember to make sure to make the string safe:

Code:
if (get_magic_quotes_gpc()) {
  $username = stripslashes($username);
}
$username = mysql_real_escape_string($username);
$query = mysql_query("SELECT ...


As for explaining, I'd rather not Wink There's lots of explanation in the previous two posts, and the rest is just a copy of your own script Wink
Marston
Smile Thanks for all the help.
snowboardalliance
If you want to know more about php, I would recommend this

http://www.hudzilla.org/phpbook/index.php

Since you are obviously a beginner, this will walk you through everything you need to know. I mean from the basics to security and performance.
Reply to topic    Frihost Forum Index -> Scripting -> Php and MySQL

FRIHOST HOME | FAQ | TOS | ABOUT US | CONTACT US | SITE MAP
© 2005-2007 Frihost, forums powered by phpBB.