Topic awaiting preservation: Security with passing variables (Page 1 of 1)

What kind of security issues come up with passing variables via php? For example I'm using one page to handle different pop-ups....

<a href="preview.php?name=image1">click to enlarge</a>

within preview.php
<img src="<?php echo $_GET['name']; ?>.jpg">

Is that OK? Or do I need to verify that the variable is actually coming from me.

you should do that yes.
A good basic rule is never ever trust a users input, nor should you trust something that another user can see and manipulate.

what if your url is altered to "preview.php?name=really_super_big_hardcore_xxx" ?
or: "preview.php?name=really_bad_file.exe"

Granted, there are worse things you can do than your example, but it's best to set a good habit from the start.
/Dan

{cell 260} {Blog}
-{ ?The Internet treats censorship as a malfunction and routes around it.? (-Barlow, John Perry) }-

... I see your point. So is there a good (or standard) way to verify something like my example? My php skills are super basic at best. I can think of using an 'if' function to test against my known names... but that kind of defeats my purpose to "simplify" things. Thanks for your help.

I agree that one should never trust a user input, yet I don't understand why this script is insecure.

Even if a moron had fun submitting an url to pornographic image, he would still be the only one seeing the image. I don't see the security problem then ?

And the code cannot be escaped to execute a script, special html chars such as " or ' will be printed with an antislash.

----
If wishes were fishes, we'd all cast nets.

It's not actually about this particular script, it's mostly the basic principles.

Consider this though, by passing something in the url, a visitor can see it, what if he finds a way to get the page to traverse directorys on the server to show a pwd file or something else interesting that the php script can reach. Perhaps a longshot but still...

I did a snip for Steve over at the GN for a flash RSS reader, here: http://www.gurusnetwork.com/discussion/thread/2942/1/ this basically looks in a directory, gathers info that is used to verify the input coming from the url.

That example is built for XML, but the basic principle should be possible to adapt.
/Dan

{cell 260} {Blog}
-{ ?The Internet treats censorship as a malfunction and routes around it.? (-Barlow, John Perry) }-

Hmm...? that was a little over my head. Would it work if I prevented a "." from being used? Something along the lines of this:

<?php
$name = $_GET['name'];
if(strstr($name, ".") || strstr($name, "/")){
$file = "invalid";
}
else {
$file = $name;
}
?>

<img src="<?php echo $file; ?>.jpg">

This particular example is fine. In fact if you are just writing out a link you don't need to check anything.


The danger comes if you start using includes

this:

include($file);

would be berry berry dangerous.



.:[ Never resist a perfect moment ]:.

if the " and ' are not escaped, anyone can break the IMG tag and do whatever they want, i.e. insert a bunch of JavaScript code, alter the HTML markup, ... which at first sight may seem rather harmless in itself but that could be used to submit some fake datas to a script or to hijack a page.

I once had a script that allowed people to download images, but then I realized that they could also download any file they wanted if they new what it was called. I was able to download my database access inclusion file which contained my database username and password.

Best way to test your scripts, try and break it yourself before someone else does. That's what I do and I've found lots of my own scripts had flaws that could potentially cause problems if a user put in the wrong stuff. Always think what could happen and eliminate the possibility.

Sorry, I'm still a little confused. So is what I've got OK?

You should additionally block " (double quote) and ' (single quote) like poi said above. This would prevent people from getting out of the image src and add stuff like onload="location.reload()" which would continually refresh the page with no end until you exit the page. But I think the only problem faced is stuff off-site or quotes. Even then, I don't think off-site images matter since none of your website visitors will see it if someone does something with it.

This should do fine:

<?php
$name = str_replace('&','&amp;',$_GET['name']); // It's standard to use &amp; instead of & (I like standard)
if(preg_match("#(^[a-z]+://|[\"'])+#i",$name)){ // Prevent anything off-site or containing quotes to show.
$file = "invalid.gif"; // Let 'em know through a image.
}
else {
$file = $name;
}
?>

<img src="<?php echo $file; ?>">

Now you can show even .gif and .png images too if you want or use php scripts like script.php?str=This+can+show+up+in+an+image&size=12. You should allow dots in a filter, because my.file.is.cool.jpg is a valid filename. You can also go through different folders like ../my_other_folder/my_image.jpg but you can't use http://www.google.com/images/logo.gif because it's off-site.

Does this help ya out?

Edit: Regular expression was changed from #(^[a-z]+://|[\"'])+# to #(^[a-z]+://|[\"'])+#i (just the i was added to make it case insensitive)



(Edited by zavaboy on 02-06-2005 07:08)

you should also filter javascript in that input.
A malicous person could send a link that contains javascript in your variable to some one else, and for example,
steal is cookies with it.

Here in the Asylum, the following code snippet is doing this:

function _secureHTMLParmeter($strParameter)

	{

		$arrToRemove = array('javascript:', 'vbscript:');

		//65 90 - uppercase

		//97 - 122 - lowercase

		foreach ($arrToRemove as $aKey => $aVar)

		{

			$aVar = strtolower($aVar);

			$aUpperVar = strtoupper($aVar);

			$strSearchVar = '/(?Uis)';

			for ($i = 0; $i < strlen($aVar); $i++)

			{

				$iOrdLower = ord($aVar{$i});

				if (($iOrdLower >= 97) && ($iOrdLower <= 122))

				{

					$iOrdHigher = ord($aUpperVar{$i});

					$strSearchVar .= '('. $aVar{$i}. '|&'. strval($iOrdLower). ';|&'. strval($iOrdHigher). 

                                             ';|&x'. sprintf('%2x',$iOrdLower). ';|&x'. sprintf('%2x',$iOrdHigher). ';)';

				}

				else

					$strSearchVar .= $aVar{$i};

			}

			$arrToRemove[$aKey] = $strSearchVar. '/';

		}



		return preg_replace($arrToRemove,'NoScript!',$strParameter);

	}

so long,

->Tyberius Prime

Nice TP.
I'm involved in building a community right now and we used regular UBB tags, as we tested it we also managed to allow manipulation of the whole page DOM and linking in whatever we wanted through some clever nesting of quotes and other tricks...

Would allow for intersting effects if released live

That's not possible here due to TP's handling of the UBB, I actually tried but you caught anything I could come up with.

So, never trust user input
/Dan

{cell 260} {Blog}
-{ ?The Internet treats censorship as a malfunction and routes around it.? (-Barlow, John Perry) }-

Rad! K... I can understand everything except TP's code. Is that checking for just java and vbscript... so I would need to combine it with something like zavaboy's example, right? Thanks again!

yeah, my code removes just javascript.
You will still need to filter quotes and make things safe for the database,
I usually just use php->htmlentities() with ENT_QUOTES
(and take care when outputing that stuff again, since all it's html entities have been replaced. And take care to unslash first, if php->get_magic_quotes_gpc returns true).

So, basically, what I'd do with your variables was
$_POST['var'] = _secureHTMLParameter(htmlentities($_POST['var'],ENT_QUOTES));

That _secureHTMLParmeter code is so complex because browsers also intepret things like j&#65;vascript and any other variation of it.

So long,

->Tyberius Prime

Ah... man. I hate to bug you guys again. I'm getting the following error now:

Fatal error: Call to undefined function: _securehtmlparameter() in /home/roy/templates4flashbuttons.com/templates/preview.php on line 32

line 32 has $_POST['name'] = _secureHTMLParameter(htmlentities($_POST['name'],ENT_QUOTES));

<?php

function _secureHTMLParmeter($strParameter)

{

	$arrToRemove = array('javascript:', 'vbscript:');

	//65 90 - uppercase

	//97 - 122 - lowercase

	foreach ($arrToRemove as $aKey => $aVar)

	{

		$aVar = strtolower($aVar);

		$aUpperVar = strtoupper($aVar);

		$strSearchVar = '/(?Uis)';

		

		for ($i = 0; $i < strlen($aVar); $i++)

		{

			$iOrdLower = ord($aVar{$i});

			if (($iOrdLower >= 97) && ($iOrdLower <= 122))

			{

				$iOrdHigher = ord($aUpperVar{$i});

				$strSearchVar .= '('. $aVar{$i}. '|&'. strval($iOrdLower). ';|&'. strval($iOrdHigher). 

										 ';|&x'. sprintf('%2x',$iOrdLower). ';|&x'. sprintf('%2x',$iOrdHigher). ';)';

			}



			else

				$strSearchVar .= $aVar{$i};

		}

		$arrToRemove[$aKey] = $strSearchVar. '/';

	}

	return preg_replace($arrToRemove,'NoScript!',$strParameter);

}



$_POST['name'] = _secureHTMLParameter(htmlentities($_POST['name'],ENT_QUOTES));



$name = str_replace('&', '&amp;', $_GET['name']);

if(preg_match("#(^[a-z]+://|[\"'])+#i", $name)){

$file = "invalid/invalid.gif";

}

else {

$file = $name;

}

?>

check the name of your function, and the name of the function you call on the line 32

oh my! .... Bed time for me. Thanks.

Always always always check the user input. You can not have any idea where there might be a possible buffer overflow exploit. Something as simple as including a couple hundred junk characters followed by some other code could allow an attacker full access to the server. You don't know where there might be unknown holes in the server side applications, never leave anything to chance.

PHP and Apache and MySQL have a pretty good track reccord for critical vulnerabilities, but one pops up every 6 months or so.

Dan @ Code Town

This is sort of in response to bitdamaged...

so in passing to page.php?dirNum=1 which has the following code:

<?php include("jCheck.php"); ?>

<?php include("config.php"); ?>

<?php

$_POST['dirNum'] = _secureHTMLParameter(htmlentities($_POST['dirNum'],ENT_QUOTES));

$dirNum = str_replace('&', '&amp;', $_GET['dirNum']);

if(preg_match("#(^[a-z]+://|[\"'])+#i", $dirNum) || is_numeric($dirNum) == false)

{

	$inc = "error";

}

else

{

	$d = $dirNum;

	if($d > $btnCat)

	{

		$inc = "error";

	}

	else

	{

		for($c = 1; $c <= $btnCat; $c++)

		{

			if($d == $c)

			{

				$inc = ${dirInc.$c};

			}

		}

	}

}

?>









<?php include("templates/inc_{$inc}.php"); ?>

jCheck.php has the function TP gave me
config.php has the declarations for $btnCat, $dirInc1, $dirInc2... and so on.

Am I asking for trouble with this?

nah, reads pretty secure to me.
I'd shoot your for the upper case in 'jCheck' though ;-).

Cool! thanks for looking it over... is it bad to name files like that?

well.... not per se.
Problem is that unix systems are case sensitive, while windows is not.
Meaning, jCheck.php and jcheck.php are completly different files for most webservers,
while they are not for windows.
So, you could be testing locally, with a file named jCheck.php and calling it 'jcheck.php' all the time
and everything works just fine.
Then you upload, and things start to go haywire. Same with images, links, etc... save trouble all around to
just lowercase all filenames.
(And when I'm leading a project, I set a coding standard up front - part to deny any discussion on the subject of
'where does this bracket go', but also in part because I'm an egomanic who had to dig through such messes once
too many. So either they follow the standard, or, when the tech comes around to bite their behinds, they also get
chewed out for not listening to their eldars ;-).

So long,

->Tyberius Prime

That's really good to know, thanks. So when trying to make files easier to read do you use hyphens? underscores?