First attempt at securing files.

Eruondo · July 8, 2009

I've read some sites and posts about securing files, and now I've tried to secure my own files. Now, this looks good to me, but I wonder if any of the more experienced coders on here could take a look and see how it looks? Is there anything I have missed? Anything I could have done better?

The file is just a simple bank which came with my MCcodes, :-P

<?php
include "globals.php";
print "

<font color='#FFFFFF'><hr width='70%'><h3>Bank</h3></font><hr width='70%'/>
";

if($ir['bankmoney']>-1)
{
	switch($_GET['action'])
		{
			case "deposit":
			deposit();
			break;

			case "withdraw":
			withdraw();
			break;

			default:
			index();
			break;
		}

}
else
{
	$sql3 = $db->query("UPDATE users SET money=money-50000,bankmoney=0 WHERE userid=$userid");
	if(isset($_GET['buy']))
		{
			if($ir['money']>49999)
				{
					print "<font color='#FFFFFF'>Congratulations, you bought a bank account for \$50,000!
";
					$db->query(sql3);
				}
			else
				{
					print "<font color='#FFFFFF'>You do not have enough money to open an account.";
				}
		}
	else
		{
			print "<font color='#FFFFFF'>Open a bank account today, just \$50,000!

			[url='bank.php?buy']Yes, sign me up![/url]";
		}
}

function index()
{
global $db, $ir,$c,$userid,$h;

print "<font color='#FFFFFF'>\n[b]You currently have \${$ir['bankmoney']} in the bank.[/b]

			At the end of each day, your bank balance will go up by 2%.




<table width='70%'>
<tr>
	<td width='50%' align='center'>
		<font color='#FFFFFF'>
			[b]Deposit Money[/b]


			It will cost you 15% of the money you deposit, rounded up.




		<form action='bank.php?action=deposit' method='post'>
			Amount: 
			<input type='text' name='deposit' value='{$ir['money']}' />


			<input type='submit' value='Deposit' />
		</form>
	</td>
	<td align='center'>
		<font color='#FFFFFF'>
			[b]Withdraw Money[/b]


			There is no fee on withdrawals.




		<form action='bank.php?action=withdraw' method='post'>
			Amount: 
			<input type='text' name='withdraw' value='{$ir['bankmoney']}' />


			<input type='submit' value='Withdraw' />
		</form>
	</td>
</tr>
</table>";
}

function deposit()
{

global $db,$ir,$c,$userid,$h,$myDeposit;
$myDeposit = abs((int) $_POST['deposit']);
$myDeposit = mysql_real_escape_string(htmlentities($myDeposit));

if($myDeposit > $ir['money'])
{
	print "<font color='#FFFFFF'>You do not have enough money to deposit this amount.";
}
else
{
	$fee = ceil($myDeposit*15/100);
	$gain = $myDeposit-$fee;
	$ir['bankmoney']+=$gain;
	$sql = mysql_real_escape_string(htmlentities("UPDATE users SET bankmoney=bankmoney+$gain, money=money-$myDeposit where userid=$userid"));
	$db->query($sql);
	print "
	<font color='#FFFFFF'>
		You hand over \$$myDeposit to be deposited, 




		After the fee is taken (\$$fee), \$$gain is added to your account. 


		[b]You now have \${$ir['bankmoney']} in the bank.[/b]

";
}
}

function withdraw()
{

global $db,$ir,$c,$userid,$h;
$myWithdraw = abs((int) $_POST['withdraw']);
$myWithdraw = mysql_escape_string(htmlentities($myWithdraw));

if($myWithdraw > $ir['bankmoney'])
{
	print "<font color='#FFFFFF'>You do not have enough banked money to withdraw this amount.";
}
else
{

	$gain=$myWithdraw;
	$ir['bankmoney']-=$gain;
	$sql2 = mysql_escape_string(htmlentities("UPDATE users SET bankmoney=bankmoney-$gain, money=money+$gain where userid=$userid"));
	$db->query($sql2);
	print "
	<font color='#FFFFFF'>
		You ask to withdraw $$gain, 




		The banking lady grudgingly hands it over. 


		[b]You now have \${$ir['bankmoney']} in the bank.[/b]

";
}
}

print "

<hr width='70%'>[url='explore.php']Back[/url]
<hr width='70%'>

";
$h->endpage();
?>

Karlos · July 8, 2009

Re: First attempt at securing files.

//Few bits..
$myDeposit = abs((int) $_POST['deposit']);
$myDeposit = mysql_real_escape_string(htmlentities($myDeposit));

$sql = mysql_real_escape_string(htmlentities("UPDATE users SET bankmoney=bankmoney+$gain, money=money-$myDeposit where userid=$userid"));

Floydian · July 9, 2009

Re: First attempt at securing files.

//Few bits..
$myDeposit = abs((int) $_POST['deposit']);
$myDeposit = mysql_real_escape_string(htmlentities($myDeposit));

$sql = mysql_real_escape_string(htmlentities("UPDATE users SET bankmoney=bankmoney+$gain, money=money-$myDeposit where userid=$userid"));

Wow, that's like trying to dance with three left feet...

It's like using four fire extinguishers to put out a candle.

It's like eating pie with five spoons.

Overkill? Yes

Would said overkill lead to security flaws or bugs? Yes

Zeggy · July 9, 2009

Re: First attempt at securing files.

//Few bits..
$myDeposit = abs((int) $_POST['deposit']);
$myDeposit = mysql_real_escape_string(htmlentities($myDeposit));

$sql = mysql_real_escape_string(htmlentities("UPDATE users SET bankmoney=bankmoney+$gain, money=money-$myDeposit where userid=$userid"));

Once you've cast a variable to an int, you don't need to escape it or convert it to html entities.

You really don't want to convert a mysql query to html entities.

mysql_real_escape_string should just be used on single values/parameters, not on entire sql queries or it will also escape the single quotes around your string values:

UPDATE ... SET username='bla' WHERE...

->

UPDATE ... SET username=\'bla\' WHERE...

Eruondo · July 9, 2009

Re: First attempt at securing files.

Well, that makes sence. So all I really need is

$myDeposit = abs((int) $_POST['deposit']);

and

sql = ("UPDATE users SET bankmoney=bankmoney+$gain, money=money-$myDeposit where userid=$userid")
$db->query(sql);

And I'm good to go?

Eruondo · July 9, 2009

Re: First attempt at securing files.

I've had a new go, :-D

(It took a while, but I've tried to learn sprintf() in the meanwhile.)

crystaltemple.php, secured with a cleanfunction.

(I use bribes instead of crystals :wink:)

<?php
include "globals.php";
print "<font color='#FFFFFF'>[b]

<hr width='70%'><h3>Bribes Exchange</h3><hr width='70%'>[/b]</font>
";
$mySpend = clean($_GET['spend']);
$bribes = (int) $_POST['crystals'];
if(!$mySpend)
{
	print "
	<font color='#FFFFFF'>Welcome to the Bribes Exchange!

		You have [b]{$ir['crystals']}[/b] bribes.


		What would you like to spend your bribes on?




	[url='bribesexchange.php?spend=refill']Energy Refill - {$set['ct_refillprice']} Bribes[/url]


	[url='bribesexchange.php?spend=IQ']IQ - {$set['ct_iqpercrys']} IQ per bribe[/url]


	[url='bribesexchange.php?spend=money']Money - \$".number_format($set['ct_moneypercrys'])." per bribe[/url]

";
}
else
{
	if($mySpend == 'refill')
		{
			if($ir['crystals'] < $set['ct_refillprice'])
				{
					print "<font color='#FFFFFF'>You don't have enough bribes!";
				}
			else if($ir['energy'] == $ir['maxenergy'])
				{
					print "<font color='#FFFFFF'>You already have full energy.";
				}
			else
				{
					$maxenergy = clean($ir['maxenergy']);
					$price = clean($ir['crystals'])-($set['ct_refillprice']);

					$sql1 = sprintf("UPDATE users SET energy = ('%u'), crystals = ('%u') WHERE (userid = %u)", 
								$maxenergy,
								$price,
								$userid);
					$db->query($sql1);
					print "<font color='#FFFFFF'>You have paid {$set['ct_refillprice']} bribes to refill your energy bar.";
				}
		}
	else if($mySpend == 'IQ')
		{
			print "
			<font color='#FFFFFF'>Type in the amount of bribes you want to swap for IQ.


				You have [b]{$ir['crystals']}[/b] bribes.


				You gain {$set['ct_iqpercrys']} IQ from one bribe.
			<form action='bribesexchange.php?spend=IQ2' method='post'>
				<input type='text' name='crystals' />


				<input type='submit' value='Swap' />
			</form>";
		}
	else if($mySpend == 'IQ2')
		{
			if($bribes <= 0 || $bribes > $ir['crystals'])
				{
					print "<font color='#FFFFFF'>Error, you either do not have enough bribes or did not fill out the form.
";
				}
			else
				{						
					$iqgain = clean($bribes*$set['ct_iqpercrys']);
					$IQ = clean($ir['IQ'] + $iqgain);
					$maxenergy = clean($ir['maxenergy']);
					$price = clean($ir['crystals'] = $ir['crystals'] - $bribes);

					$sql2 = sprintf("UPDATE users SET crystals = ('%u') WHERE (userid = %u)", 
								$price,
								$userid);
					$db->query($sql2);

					$sql3 = sprintf("UPDATE userstats SET IQ = ('%u') WHERE (userid = '%u')",
								$IQ,
								$userid);
					$db->query($sql3);
					print "<font color='#FFFFFF'>You traded $bribes bribes for $iqgain IQ.";
				}
		}
	else if($mySpend == 'money')
		{
			print "
			<font color='#FFFFFF'>Type in the amount of bribes you want to swap for money.


				You have [b]{$ir['crystals']}[/b] bribes.


				You gain \$".number_format($set['ct_moneypercrys'])." from one bribe.
			<form action='bribesexchange.php?spend=money2' method='post'>
				<input type='text' name='crystals' />


				<input type='submit' value='Swap' />
			</form>";
		}
	else if($mySpend == 'money2')
		{
			if($bribes <= 0 || $bribes > $ir['crystals'])
				{
					print "<font color='#FFFFFF'>Error, you either do not have enough bribes or did not fill out the form.
";
				}
			else
				{
					$moneygain = clean($bribes * $set['ct_moneypercrys']);
					$money = clean($ir['money'] + $moneygain);
					$price = clean($ir['crystals'] - $bribes);

					$sql4 = sprintf("UPDATE users SET crystals = ('%u'), money = ('%u') WHERE (userid = '%u')",	
						$price,
						$money,
						$userid);
					$db->query($sql4);

					print "<font color='#FFFFFF'>You traded $bribes bribes for \$".number_format($moneygain).".";
				}
		}
}

print "

<hr width='70%'>[url='bribesexchange.php']Back[/url]
<hr width='70%'>

";
$h->endpage();
?>

Here is the function I use:

function clean($clean)
{
  if (get_magic_quotes_gpc()) 
  		{
  		   $clean = stripslashes($clean);
  		}
  return htmlentities(mysql_real_escape_string(trim($clean)));
}

Kudos to Floydian and SomeRandomGuy on the function and sprintftutorials. As I gathered, sprintf is more secure, so I've written the codes with it. How'd I do?

Floydian · July 9, 2009

Re: First attempt at securing files.

                 $money = clean($ir['money'] + $moneygain);
                 $price = clean($ir['crystals'] - $bribes);

You're still doing way to much without any sort of specific goal in mind here. It's like having 10 pad locks on a cardboard door. You might feel safe, but you haven't understood the door you put the padlocks on.

There's two ways I see this going.

One is: You might be the kind of person that just does this as a hobby, tosses the pasta and sees if it sticks to the ceiling, and hopes for the best. That kind of person won't ever dissect all the functions and all of the code they use in order to understand what is happening. They rely on "forum code reviews" to ensure that their code doesn't have exploits in the hopes that the forum folks will find bugs before their players exploit them.

The second kind of person you might be, is the kind of person that really does want to understand this stuff.

If you are person one, I have nothing for you. You're a hobbyist, so if you get bad consequences from your code, then at least hopefully you had fun with your dabblings.

If you are person two, then I highly suggest you study up on each of the functions you're using and try to understand exactly why they're being used. You're never going to get a good and full review of your code for free. So if you're serious, study up.

Eruondo · July 9, 2009

Re: First attempt at securing files.

I do this as a hobby, it's nothing I get payed for doing. But that does not mean that I don't want to learn and understand, because I really do.

$money = clean($ir['money'] + $moneygain);
                 $price = clean($ir['crystals'] - $bribes);

You say this is overkill, but I don't understand why, even after reading about these functions for the last six hours. In my head, one is good, but two is better, especially when it comes to security. :roll: But then again, I am a newbie, and there could be several good reasons why not, I just havent found them yet.

And I don't expect people to go through my code, I just hoped for a fast skimthrough if someone had the time. :-)

Eruondo · July 9, 2009

Re: First attempt at securing files.

                 $money = clean($ir['money'] + $moneygain);
                 $price = clean($ir['crystals'] - $bribes);
You're still doing way to much without any sort of specific goal in mind here. It's like having 10 pad locks on a cardboard door. You might feel safe, but you haven't understood the door you put the padlocks on.

I don't doubt that you're right, I just don't understand what I'm doing wrong... :| I know, I'm a slow learner, but I really want to understand.

Karlos · July 9, 2009

Re: First attempt at securing files.

// Function gets set fine.
function clean($Str) {
// Checking if Magic Quotes are on (Deprecated as of Php 5.3.0 and removed in Php 6.0.0)
if (get_magic_quotes_gpc()) {
 // String slashes to avoid problems if Magic Quotes are on.
 $Str = stripslashes($Str);
 // Ending If.
}
// returning an escape string with trimmed whitespaces at the start and end. (Notice the $c for mres()? Connection Identifier.. You should use it.)
return mysql_real_escape_string(trim($Str), $c);
// End function.
}

Eruondo · July 9, 2009

Re: First attempt at securing files.

Aha, I see. So I'm doing it wrong because I am securing variables which have nothing to do with user input?

...

I'm so stupid.

Karlos · July 9, 2009

Re: First attempt at securing files.

Not sure where you get that idea..

I was showing not to use mres() and htmlentities() togther.. and when usinh mrse() use a link idenifier.

Eruondo · July 9, 2009

Re: First attempt at securing files.

That's why I wrote "I'm so stupid" afterwards, when I got your point. :|

Still not to sure what mres() ($c?) and mrse() is though.

Haunted Dawg · July 9, 2009

Re: First attempt at securing files.

Still not to sure what mres() ($c?) and mrse() is though.

mres() is mysql_real_escape_string() for short.

($c) will be the link identifer so mres know's what to connect to.

mrse() i think he made a spelling mistake and wanted it to be mres.

Floydian · July 10, 2009

Re: First attempt at securing files.

Eruondo, the key is understanding the input and the output.

For instance, a MySQL field is an INTEGER and you want to match a value to that field.

You need to have an integer to match it up with.

Using abs() -- which removes the negative sign from a number and intval which converts a variables type to integer can be used on "user input" to create a safe positive integer.

Escaping it using mysql_real_escape_string() or using htmlentities() or trim() are all pointless because you have an integer now.

The converse is true. Once you have "cleaned" a string, you wouldn't use intval() or abs() on it.

Eruondo · July 10, 2009

Re: First attempt at securing files.

Ah, right! I (finaly) got it now!

Thanks for sticking out with me, hopefully I'll make it worth your while in the end!

Sign In

First attempt at securing files.

Recommended Posts

Eruondo

Karlos

Floydian

Zeggy

Eruondo

Eruondo

Floydian

Eruondo

Eruondo

Karlos

Eruondo

Karlos

Eruondo

Haunted Dawg

Floydian

Eruondo

Join the conversation

Browse

Activity

Hosting