PHP code injection

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
benyboi
Forum Commoner
Posts: 80
Joined: Sat Feb 24, 2007 5:37 am

PHP code injection

Post by benyboi »

Hello,

I have a comments form on my site and i wanted to check that nothing bad could be inserted, is this good enough?:

Code: Select all

<?php

$invalidChars=array("/","\\","\"",";");

// Email to send to
$email = "my@email.com";

// The subject
$subject2=$_POST['name'];
$subject=str_replace($invalidChars,"",$subject2);

// The message
$comment2=$_POST['comment'];
$comment=str_replace($invalidChars,"",$comment2);

// Email from
$emailfrom2= "me@email.com";
$emailfrom=str_replace($invalidChars,"",$emailfrom2);

if ( $emailfrom2 == $emailfrom ) {

if ( $subject2 == $subject ) {

mail($email, $subject, $comment, "From: $emailfrom");
echo "Sending your comment,";

?>
<meta http-equiv="REFRESH" content="0; URL=thankyou.php">
<?

} else {
echo 'Your details were invalid, please go back and try again.';
}

} else {
echo 'Your details were invalid, please go back and try again.';
}


?>
Hope this is understandable code!

Also, can anyone explain how "code injection" works? I think i understand, but would like a professional opinion.

Thanks,
Ben
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

You are probably going to have to search for SQL injection, XSS and other exploits. When using mysql, pass the input data throught mysql_real_escape_string() before letting it touch your database. And what you are doing now, in my opinion, is not enough (and is not proper). You are changing someones input by using str_replace(). If there input is not acceptable, error out and tell them they cannot do that. In a nice way, of course. But in my opinion, the users input is what they intended to input. Changing is not really something you should do.
benyboi
Forum Commoner
Posts: 80
Joined: Sat Feb 24, 2007 5:37 am

Post by benyboi »

So would i do something like:

Code: Select all

$output=mysql_real_escape_string($input)
Where $output is what will be inserted into DB and $input is what the user put in.

Thanks
Dave2000
Forum Contributor
Posts: 126
Joined: Wed Jun 21, 2006 1:48 pm

Post by Dave2000 »

Everah wrote:When using mysql, pass the input data throught mysql_real_escape_string() before letting it touch your database.
I've often wondered... do you always always need to do this? For example, if you've verified a variable is numeric with ctype_digit(), do you still need to pass it through mysql_real_escape_string() before inserting into database?
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

Probaly not. But it doesn't add much overhead and is not that much to ask for when it comes to security.
benyboi
Forum Commoner
Posts: 80
Joined: Sat Feb 24, 2007 5:37 am

Post by benyboi »

When getting parts of a url, is this enough security:

Code: Select all

<?
	$f=$_GET['f'];
	
	$s=$_GET['s'];
	
	if ( $s == "1" ) {
	$file = $f . ".php";
	} else {
	$file = $f . ".html";
	}
	include($file);
	?>
It is only used to open the relevant page. Where $f is the name. eg if $f was "help" the page help.html would be opened. $s is used to choose between .html or .php file.

Im guessing this would be open for someone to put in a diff website url with possible bad code?

Thanks for your help
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

No, that is not safe. You might want to look at putting path names in the database then using the database table id as your querystring fetcher. Also, use file_exists() when including dynamically.
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post by Maugrim_The_Reaper »

I know this sounds weird but filter EVERYTHING and escape (both for database, and also when sending to browser with htmlentities()) pretty much EVERYTHING. Some things obviously don't need filtering - but that can change over time as code evolves without you noticing. Common cause of security exploits appearing...

I know tons of people have varying practices, but the single most common mistake an application makes is to let the developer decide when to escape on a case by case basis. It allows for errors and mistakes far too often - even for hardened professionals ;). Having a consistent practice, or even some method of handling escaping (db and browser output) automatically makes a big difference. In case anyone wonders I actually do escape everything - I've even subclasses the Zend Framework to force it unless a developer specifically breaks out of the automation.
When getting parts of a url, is this enough security:
include() can include a remote file - if the path is a url, or an unexpected path then it needs to be blocked. It's generally a good idea to make sure a user can never define an include.
benyboi
Forum Commoner
Posts: 80
Joined: Sat Feb 24, 2007 5:37 am

Post by benyboi »

I have changed the include so that the path is now the following:

"D:/inetpub/www-.../" . $variable . ".php"

Is that better?

How do you check for stuff?

Thanks
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post by RobertGonzalez »

Better, yes. But still, you shouldn't allow the user to directly pass information to be included.
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

You are probably going to have to search for SQL injection
Never a bad thing to learn about but there aren't any queries being performed in the code in the original post to I fail to see how it is really relevant.

However you could suffer from mail header injection. There are ways of thwarting this a search for "header injection" yielded this as the first result - looks like something you should read.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

benyboi wrote:"D:/inetpub/www-.../" . $variable . ".php"

Is that better?
Without proper filtering... absolutely not. It's no worse either. :)
Post Reply