Database update from drop down menus

[Damond]

I have created some little links that are coded to pop up during certain situations such as being in jail or the hospital... Just a little quick link where the user doesn't have to go the their inventory every time to use and item. That was the easy part. Next I created some drop down menus in the preferences page that will allow the users to pick which item from their own inventory they would like to set to this quick link. And this is where I am running in to an SQL problem. I know it is just a little bit of something that I am missing to get the DB to update. I have been going over this code for three days now and can't see what it is that I am missing.

 

function item_change()
{
global $db,$ir,$c,$userid,$h;
print "<h3> Set your healing, feeding, and jail quick menu items here</h3>
<form action='preferences.php?action=itemchange2' method='post'>
<table>
<tr>
	<th>Healing</th>
	<th>Feeding</th>
	<th>Jail</th>
</tr>
<tr>
	<td>";
	print "".healitem_dropdown(NULL, "healitem")."";
	print "</td><td>";
	print "".fooditem_dropdown(NULL, "fooditem")."";
	print "</td><td>";
	print "".jailitem_dropdown(NULL, "jailitem")."";
	print "</td>
</tr>
<tr>
	<td colspan=3 align='center'><input type=submit value=Change></td>
</tr>
</table>
";
}
function do_item_change()
{
global $db,$ir,$c,$userid,$h;

$db->query("UPDATE users SET heal_item='{$_POST['healitem']}', feed_item='{$_POST['fooditem']}', jail_item='{$_POST['jailitem']}' WHERE userid=$userid");
print "Items changed"; 
}

 

All three menus are set up in the global functions page with all the rest of my drop downs and work perfectly. I'm sure someone is going to look at this and say "What is this idiot doing!?" The answer is simple. I'm trying to learn PHP and MySQL as I go. I think I am not doing to bad so far.

[SHPXLBH]

Can I point out that someone can do whatever they like with this. Like; give themselves unlimited money, unlimited crystals, make themselves admin, put everyone in hospital, and so much more damage (basically do whatever they want with the users table).

Now the above has been mentioned, let's refactor!

We know each input must apply to one rule; values must be numerical (as item id's are numerical). With this, we can do a simple sanitise.

 

function do_item_change() {
   global $db,$userid;

   //Maybe check they have this item in their inventory... or is this done when they use on each page?
   $_POST['healitem'] = (int) sprintf("%u", $_POST['healitem']);
   $_POST['fooditem'] = (int) sprintf("%u", $_POST['fooditem']);
   $_POST['jailitem'] = (int) sprintf("%u", $_POST['jailitem']);

   $db->query("UPDATE users 
               SET heal_item=". $_POST['heal item'] .", 
                   feed_item=". $_POST['food item'] .", 
                   jail_item=". $_POST['jail item'] ." 
               WHERE userid=". $userid);

   print "Items changed";  //Maybe put this in a nice div?

}

 

An example of input and various techniques to sanitise input: https://eval.in/238625

Edited January 1, 2015 by SHPXLBH

[Damond]

[MENTION=70600]SHPXLBH[/MENTION] Thanks so much. I was WAY off on this one. Lucky for me I have good people like you to come along and point out my errors. I did know about the security risk the way that it was I just wanted to get it to work before I worried about that part.

As soon as I get home from this little new years gathering with my family I will change the code around and maybe show a nice screen shot if I can figure out how to post one. LOL

[SRB]

Damond, inbox me your Skype ID

[Jax]

[MENTION=70600]SHPXLBH[/MENTION] Your so-called ""sanitization"" code leaves a lot to be desired. Lack of type checking can lead to potentially unexpected values; for example:

Given $_POST['var'] = "-1"; echo (int)sprintf('%u', $_POST['var']); yields 9223372036854775807 assuming a 64-bit machine, while echo sprintf('%u', $_POST['var']); yields 18446744073709551615.

Passing an array instead of an string again yields unexpected values:

Given $_POST['var'] = array(); echo (int)sprintf('%u', $_POST['var']); yields 0, while $_POST['var'] = array(1,2,3) yields 1. Not unexpectedly, $_POST['var'] = array(array()); yields 1;

Within any program, you should always know the type of expected input; with PHP, $_GET and $_POST elements are either strings or arrays. If it's a string, and you are expecting an integer, then surely ctype_digit() is the optimal check, though obviously range checking is required.

I would have thought that something more akin to:

// make sure the expected verb has been used

if ($_SERVER['REQUEST_METHOD'] == 'POST')
{
// setup the range limits

   $var_min = 0;
   $var_max = 100;

   // check the given variable has been provided, is the correct type
   // AND fits within a sane range

   if (array_key_exists('var', $_POST) && ctype_digit($_POST['var']) &&
      ($_POST['var'] >= $var_min) && ($_POST['var'] <= $var_max))
   {
       $var = $_POST['var'];

       // do something with $var - no real need to cast it to an int here
   }
   else
   {
       // bad data

       header('HTTP/1.1 400 Bad Request');
       exit;
   }
}

Of course, this type of problem is always open to interpretation, a lot of games developed through these pages as you have pointed out in other topics are easily inject-able, however lack of proper type checking as in this instance is equally a recipe for disaster. While I'm sure your code would correctly handle the case of say, transferring $9223372036854775807 between accounts, I know a lot of the code presented here fails this most basic of check.

[Coly010]

[MENTION=70600]SHPXLBH[/MENTION] Your so-called ""sanitization"" code leaves a lot to be desired. Lack of type checking can lead to potentially unexpected values; for example:

Given $_POST['var'] = "-1"; echo (int)sprintf('%u', $_POST['var']); yields 9223372036854775807 assuming a 64-bit machine, while echo sprintf('%u', $_POST['var']); yields 18446744073709551615.

Passing an array instead of an string again yields unexpected values:

Given $_POST['var'] = array(); echo (int)sprintf('%u', $_POST['var']); yields 0, while $_POST['var'] = array(1,2,3) yields 1. Not unexpectedly, $_POST['var'] = array(array()); yields 1;

Within any program, you should always know the type of expected input; with PHP, $_GET and $_POST elements are either strings or arrays. If it's a string, and you are expecting an integer, then surely ctype_digit() is the optimal check, though obviously range checking is required.

I would have thought that something more akin to:

// make sure the expected verb has been used

if ($_SERVER['REQUEST_METHOD'] == 'POST')
{
// setup the range limits

   $var_min = 0;
   $var_max = 100;

   // check the given variable has been provided, is the correct type
   // AND fits within a sane range

   if (array_key_exists('var', $_POST) && ctype_digit($_POST['var']) &&
      ($_POST['var'] >= $var_min) && ($_POST['var'] <= $var_max))
   {
       $var = $_POST['var'];

       // do something with $var - no real need to cast it to an int here
   }
   else
   {
       // bad data

       header('HTTP/1.1 400 Bad Request');
       exit;
   }
}

Of course, this type of problem is always open to interpretation, a lot of games developed through these pages as you have pointed out in other topics are easily inject-able, however lack of proper type checking as in this instance is equally a recipe for disaster. While I'm sure your code would correctly handle the case of say, transferring $9223372036854775807 between accounts, I know a lot of the code presented here fails this most basic of check.

You have a lot of code there, surely this will do the same to make sure you have a positive integer? if you want negative, just remove the abs()..

function test_num($var){
  $no = abs((int) $var)+0;
  return $no;
}
if(isset($_POST['var']){
  $no = test_num($_POST['var']);
  if($no >= $min && $no <= $max){
     // do something 
  } else {
     // do not
  }
} 

[Sim]

Wouldnt it be is_int() as ctype_digit() allows for decimals?

[Coly010]

Wouldnt it be is_int() as ctype_digit() allows for decimals?

No, take a look: http://php.net/manual/en/function.ctype-digit.php

the period ( . ) is not considered a digit so it will return false apparently

However, if your variable is already an integer, then using ctype_digit() will be extremely unreliable. The example given on php.net:

 

<?php

$numeric_string = '42';
$integer        = 42;

ctype_digit($numeric_string);  // true
ctype_digit($integer);         // false (ASCII 42 is the * character)

is_numeric($numeric_string);   // true
is_numeric($integer);          // true
?>

Edited December 31, 2014 by Coly010

[Sim]

I prefer this one: http://php.net/is_int Its just a personal preference.

[Script47]

is_int() is the same as is_integer(), the difference between ctype_digit() and is_int() the latter does not take numerical strings unlike the first one which does.

Side Note!

Didn't even know I hit my 1000+ post. I wanna check my 1000th. :(

Edited January 1, 2015 by Script47

[Coly010]

I prefer this one: http://php.net/is_int Its just a personal preference.

if your using is_int() with input then your need to convert your input string to an integer before hand, in which case you'll know its an int?

[Script47]

if your using is_int() with input then your need to convert your input string to an integer before hand, in which case you'll know its an int?

If you're asking a question, then yes, because you'll have to convert otherwise is_int() will fail.

THANK YOU [MENTION=64603]Sim[/MENTION] I NOW HAVE A 2 BAR REP! :D

Edited January 1, 2015 by Script47

[Sim]

? I didn't even know there was Rep on this forum =]

IRC [MENTION=69670]Script47[/MENTION]

[Script47]

? I didn't even know there was Rep on this forum =]

IRC [MENTION=69670]Script47[/MENTION]

The green bars underneath your "rank".

[Jax]

You have a lot of code there, surely this will do the same to make sure you have a positive integer

No, it doesn't - However if you think that is the same ... I'll accept that. What's your game URL ? :D

[Sim]

As far as I am aware, $_GET[] variables are mixed variables. So if I am expecting a number, and its $_GET['id']

then is_int($_GET['id']) is false, well it does it's job if it's not a whole #.. ;]

[Damond]

Thank you guys for all the great input. As it turns out not only was a missing something in the second function but I also forgot to add the second function to my switch.

Everything is working great now. My database is being updated with the selections users make and because of your guys at least this part of my coding is well protected. When my site is all said and done I might have to look into hiring out the task of securing my whole site.

Thanks again and happy new year.

[Coly010]

No, it doesn't - However if you think that is the same ... I'll accept that. What's your game URL ? :D

So tell me, why does it not?

If you try input a string into that function, it'll return 0. Eg, "hello" returns 0

if you input a number you get a number. Eg "90" will return 90

if you input a decimal you get an int. Eg "45.6" will return 45

I'd like to know your reasoning behind saying that this will not sanitise any form on input when you are expecting a positive input

[Jax]

You state that you are expecting a positive input, and the discussion this far seems to assume we are also talking about integers, yet you state that both "hello" and 45.6 are valid integers. How can "hello" be a valid input? The initial point I made pointed out the flaw of using (int)sprintf('%u', $var); as while the result is an integer, it can be a highly unlikely value.

Testing your code suggests that you perhaps need to look a little closer at it. After all what is the desired effect here? Users will generally fill out forms correctly, though some mistakes must be prevented. Hackers will try to subvert your form by pushing highly unusual data into your system. Surely the latter of these must be blocked rather than blindly accepted.

Example code: https://node86.com/pastebin/5b0s72b

And output: https://node86.com/pastebin/c43q6

While your code is not wrong, it simply makes assumptions about the input which ~may~ come back and bite you. Correct type checking, type conversion and range checking is a necessary fact of almost all applications, unfortunately, with PHP this can be made tricky as people fall into common basic mistakes and worse make blind assumptions about internal functions accepting their documentation as-is without running sufficient checks. A good hacker or pen tester will understand the flaws in the language and how people make simple mistakes with sanitization enabling them to circumvent your system.

BTW, the 075 test caught me out, I'm still not 100% convinced on how to handle that one cleanly outside of using regex to check the input, however then you run into a range of other interesting problems.

[Damond]

I'm sorry but you guys seem to be really over complicating this. There is a set of three drop down menus. Healing, Jail, and Feed. These lists are populated only from what a user has in their inventory. There is no actual user input other then picking an item from menus to be set as a quick link and hitting the submit button. IF they where to somehow change the id of the item to be set in the user table they would still run into an invalid use of file as the quick links use a inventory id not the actual item id. And even at that it still checks again to insure that you do have this item in your inventory before letting you use it.

[KyleMassacre]

I'm sorry but you guys seem to be really over complicating this. There is a set of three drop down menus. Healing, Jail, and Feed. These lists are populated only from what a user has in their inventory. There is no actual user input other then picking an item from menus to be set as a quick link and hitting the submit button. IF they where to somehow change the id of the item to be set in the user table they would still run into an invalid use of file as the quick links use a inventory id not the actual item id. And even at that it still checks again to insure that you do have this item in your inventory before letting you use it.

Ahhh, that's where you are wrong. The dropdowns create an "input" correct? Just because the user doesn't have a textbox to type in doesn't mean that it's not user data. All these people are doing is telling you where and how exploits in your code exists. You see, someone can easily open up the dev tools in there browser and change a view values around to get "their" desired effect if they so wish. Please keep in mind that any $_POST or $_GET value may be altered. That is where 99.9% of all vulnerabilities in games tend to exist.

Also so since im on the subject of changing data in the source code I don't see any type of checking that the user actually owns the item. Your dropdown(s) just scan their inventory at that given moment to display statically on the screen but when the submit happens there is no checking of their inventory what so ever.

Edited January 1, 2015 by KyleMassacre

[Damond]

My mistake.

[KyleMassacre]

My mistake.

This all comes with learning my friend

[MENTION=70654]Jax[/MENTION],

You sound mighty familiar. Are you sure you are new to the forum? :p

Edited January 1, 2015 by KyleMassacre

[Damond]

So all this said, is there a "best" way of protecting yourself with out having to add lines and lines of extra coding for every user input? Understand most of what has been said in this thread is way over my head. I have done so much reading on SQL injection security and CSRF security in the last two days that I am completely at a loss as to what actually needs to be done.

[Coly010]

So all this said, is there a "best" way of protecting yourself with out having to add lines and lines of extra coding for every user input? Understand most of what has been said in this thread is way over my head. I have done so much reading on SQL injection security and CSRF security in the last two days that I am completely at a loss as to what actually needs to be done.

"best" way, no. What [MENTION=70654]Jax[/MENTION] has been saying is correct. 99.9% of the time you do need to type check. You can never 100% fully secure a site, but that in mind you can take precautions. The way to avoid writing lines and lines of code over and over is to create a function then pass your input to it.

However be aware that with programming there are a lot of times you need to write that bit extra to secure your code.

[MENTION=70654]Jax[/MENTION] , I understand what you're saying, the point I'm trying to make is that if a hacker was attempting to input a malicious query into the request then my function just converts it to an int, which is no longer as malicious. So in the case of a string "abcd" being entered when a user transfers money to another user, it'll simply send them a value of 0 money. Not too bad. If the game is set up so that money is an int, then a user trying to send 99.999999999 money will end up sending 99 money. not too bad again. Your array example throws me a bit, because they could have a string in the first element and a number in the second. my function as far as i'm aware will always return 1 for an array, which again isn't too malicious but still not what I would want. I have checks if someone tries to send 0 of anything and prevents them of doing it. I also have checks to prevent a user from sending more than they have so if the user has 0 money and a hacker inserts an array it'll still fail.

To me personally the function works for my needs as I have the other checks that are needed.

Your octal value example, returning 75 again in a game situation isn't overly malicious as most users wont understand the difference between decimal and octal and will most likely be looking to actually input 75.