Page 1 of 1

PHP code injection

Posted: Sat Feb 24, 2007 9:06 am
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

Posted: Sat Feb 24, 2007 9:11 am
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.

Posted: Sat Feb 24, 2007 9:26 am
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

Posted: Sat Feb 24, 2007 10:50 am
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?

Posted: Sat Feb 24, 2007 11:01 am
by RobertGonzalez
Probaly not. But it doesn't add much overhead and is not that much to ask for when it comes to security.

Posted: Sat Feb 24, 2007 12:21 pm
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

Posted: Sat Feb 24, 2007 12:58 pm
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.

Posted: Sat Feb 24, 2007 5:50 pm
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.

Posted: Sat Feb 24, 2007 7:16 pm
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

Posted: Sat Feb 24, 2007 7:26 pm
by RobertGonzalez
Better, yes. But still, you shouldn't allow the user to directly pass information to be included.

Posted: Sun Feb 25, 2007 3:39 am
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.

Posted: Sun Feb 25, 2007 10:18 am
by feyd
benyboi wrote:"D:/inetpub/www-.../" . $variable . ".php"

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