[SOLVED] code sanitising?

[bubblybabs]

I know, dumb question but I'm not sure how to go about doing this...

 

How does one go about checking their code to see if it is "sanitised" to prevent exploitation?

 

Thanks,

Babs

[zfred09]

Make sure that anywhere where a user can insert something, such as a html input field or the address bar if you are using $_GET variables, that that input is checked.  This can be done in a variety of ways including Regular Expressions, mysql_escape_string, etc.  Basically just make sure they have to enter what you expect them to enter.  Say you have an input where you ask for their name, make sure the only thing they can enter is letters, not special characters, etc.

[bubblybabs]

I understand the sanitizing idea but am having difficulty understanding how to implement it...  I'm not a complete newbie to php but I'm currently only at the stage of editing what others have done before me (so, I guess you'd consider me a newbie)...  A program I am using has an injection problem that was just discovered and I am trying to learn how this is done so I can try to fix the script...  I know of several users who have been affected and I want to help others as well as myself who use this script...

 

I've looked at the malicious coding and have been able to do a "simple" work-around but I'm sure a smart programmer would know how to get around that...  I don't know where to read to understand how to sanitize the program code...  Looking on the internet I keep coming across a sanitizing code but that is not what I want to do...  One problem is that I don't know how to actually run this malcious coding so I can't check my new coding with it to see if it's fixed...

 

You state "Make sure that anywhere where a user can insert something, such as a html input field or the address bar if you are using $_GET variables" = this part of the script uses a GET variable...  Looking at this malicious coding, it somehow uses an unsanitized GET variable to allow a script to "inject" information onto a persons website as well as snagging the website owners username and passwords from their MySQL tables...

 

The program I am referring to is gCards 1.46 ...  The file in question is the getnewsitem.php file...  The code in question is information given before the MySQL fetch string...

 

Problem part of code (from what I understand, it is the get newsid part):

 

include_once('inc/adodb/adodb.inc.php');	   # load code common to ADOdb
include_once('config.php');
include_once('inc/UIfunctions.php');
include_once('inc/newsclass.php');

$page = new pagebuilder;
include_once('inc/setLang.php');
$page->showHeader();

$newsid = $_GET['newsid'];

if ($newsid)
{
$ADODB_FETCH_MODE = ADODB_FETCH_ASSOC;
$conn = &ADONewConnection('mysql');	# create a connection
$conn->Connect($dbhost,$dbuser,$dbpass,$dbdatabase);

 

My coding I've played with:

include_once('inc/adodb/adodb.inc.php');	   # load code common to ADOdb
include_once('bbconfigff.php');
include_once('inc/UIfunctions.php');
include_once('inc/newsclass.php');

$page = new pagebuilder;
include_once('inc/setLang.php');

$newsid = (int)$_GET['newsid'];
$page->langargs = "&newsid=$newsid";

$page->showHeader();

if (!$newsid)
{
echo '<span class="error" id="news_id_error">"News ID required"</span>';
$page->showFooter();
exit;
}

 

How do I figure out if what I have done will be secure?

 

Thanks so much,

babs

 

 

[zfred09]

Well one thing I see right away is this

 

$newsid = (int)$_GET['newsid'];

 

This is what I was talking about, there is no check on this $_GET value,

 

say you expect this in the address bar www.yoursite.com?newsid=230

what keeps a hacker from entering www.yoursite.com?newsid=getallyourpasswords

 

This is where you need to implement one of the techniques I outlined above to make sure that you only get what you want in that $_GET variable, not something malicious.

 

[bubblybabs]

Ah!  Let me look at this and work on it more...  Many thanks for the input...

Babs

[GingerRobot]

Well one thing I see right away is this

 

$newsid = (int)$_GET['newsid'];

 

This is what I was talking about, there is no check on this $_GET value,

 

say you expect this in the address bar www.yoursite.com?newsid=230

what keeps a hacker from entering www.yoursite.com?newsid=getallyourpasswords

 

This is where you need to implement one of the techniques I outlined above to make sure that you only get what you want in that $_GET variable, not something malicious.

 

 

The use of type casting to prevent sql injection is actualy a recommended process by Zend. If you define a variable as an integer, then attempt to assign a string to it, the integer will be 0.

 

<?php
$myvar = (int) 'test';
echo $myvar;
//echos 0
?>

[chocopi]

If you know it is just going to be a interger then you could use something like this:

 

$newsid = sprintf("%d",$_GET['newsid']);

 

Hope it helps

 

~ Chocopi

[bubblybabs]

OK, how does this look?  I can test it by putting a letter into the address bar and getting the error message I intended...  I noticed that the newsid is always a number so I tried to code it so only numbers can be used...  I don't know if the size limit is of any use but thought I'd toss it in there and ask you what you thought...

 

The formfunction coding is at the bottom...

 

Does this seem a solid?

 

include_once('inc/adodb/adodb.inc.php');	   # load code common to ADOdb
include_once('config.php');
include_once('inc/UIfunctions.php');
include_once('inc/formFunctions.php');
include_once('inc/newsclass.php');

$page = new pagebuilder;
include_once('inc/setLang.php');
$page->showHeader();

// Check size of post data and throw errors if it is too big
if (strlen($newsid) > 100)
{
	echo $nav16;
	exit;
}


// Check $newsid for numbers only
if (!validNewsid($newsid))
{
	echo $nav16;
	exit;
}
else
{
$ADODB_FETCH_MODE = ADODB_FETCH_ASSOC;

 

formfunction code:

 

function validNewsid($newsid)
{
if (eregi("^[[:digit:]]$", $newsid ))
	return true;
else
	return false;
}

 

Thanks,

Babs

[bubblybabs]

If you know it is just going to be a interger then you could use something like this:

 

$newsid = sprintf("%d",$_GET['newsid']);

 

Hope it helps

 

~ Chocopi

 

Whoops!  Here I was looking all over the net trying to find a way to make sure numbers were only used and here is some nice coding...  Is this better than what I have?

 

Babs

[chocopi]

No, neither is better to have, its just what you prefer, and as long as they both work then you should be fine no matter which script you decided to use. The script that I gave you is just one that one of the mods gave me when I asked a similar question to you.

 

Hope that helps

 

~ Chocopi

[bubblybabs]

Thanks chocopi...

 

I know it can be difficult to tell if this is secure, but does the coding seem to be now?

 

Babs

[chocopi]

Yes the code is secure and you should be fine

 

Just out of intrest what is the :digit: part in this

 

if (eregi("^[[:digit:]]$", $newsid ))

 

You could just use this

 

if (eregi("^([0-9])+$",$newsid)) //doesnt really need eregi, but ohwell

 

I hope that helps

 

~ Chocopi

 

[bubblybabs]

I found that digit coding on the net and it worked so I used it...  Would yours be more standard and therefore more cross-site friendly?

 

Babs

[bubblybabs]

Oops, found a problem...  If I have more than 9 news items I get my error message due to this coding:

 

// Check size of post data and throw errors if it is too big

// Change this number if your news entries go over the digit shown below

// Keep as small as possible to discourage script kiddies

if (strlen($newsid) > 100)

{

echo $nav16;  // error message

exit;

}

 

What is wrong with this code?  You'd think I could go from 1-100 characters...

 

Babs

 

[bubblybabs]

Well, nevermind, I just realized strlen is not what I want to use...

And your example does work better than my digit one chocopi so I'm using that one instead...

Otherwise, the original topic seems to be solved..

:-)

Babs

[bubblybabs]

Wanted to add some more info...

 

Your example does work better than my digit one chocopi so I'm using that one instead...  *That* was what was causing the error, not the code I thought...

 

I modified the original coding in question to:

// Check size of post data and throw errors if it is longer than 3 characters
if (strlen($newsid) > 3)
{
echo $nav16;
exit;
}

 

so we won't have more than characters in the newsid which should be sufficient...

 

And modified the inc/formFunction.php coding to:

function validNewsid($newsid)
{
if (eregi("^([0-9])+$",$newsid))
	return true;
else
	return false;
}

 

Kept the eregi even though you say it doesn't need it...

 

So, the original topic seems to be solved..

:-)

 

Again, many thanks,

Babs

[chocopi]

kool, well im glad you got it fixed.

 

I think

if (eregi("^[[:digit:]]$", $newsid ))

wasnt working when you got over 9 because you are missing a '+' sign befiore the '$'. I don't know much about regex but I think using '+$' instead of just '$' means that you can have n^th length instead of just the one. So if you used:

if (eregi("^[[:digit:]]+$", $newsid ))

It should act in the same way as

if (eregi("^([0-9])+$",$newsid))

 

And what I was saying about eregi is that the 'i' means it is case-insensitive wheres using ereg would do the same thing as you are using numbers, and numbers cant be capitals or lowercase

 

Well I hope that clears up everything,

Happy Codding

 

~ Chocopi