Help with PHP contact form security

Hi guys,

I'm wondering if anyone can help me. I'm trying to make my contact form on my site a little more secure by making certain fields required, but I'm pretty useless with PHP. Could anyone take a look for me please and suggest what I need to do to make the following fields required?

Name, Email, Subject and Message

<?php
$name = $_POST['name'];
$lname = $_POST['email'];
$email = $_POST['company'];
$email = $_POST['subject'];
$email = $_POST['message'];


//Sending Email with form data
$header = "From: $email\n"
. "Reply-To: $email\n";
$subject = "Contact form submission";
$email_to = "admin@anaddress.com";
$message = "name: $name\n"
. "E-mail: $email\n"
. "Comapny: $company\n"
. "Subject: $subject\n"
. "Message: $message\n";
mail($email_to, $subject ,$message ,$header ) ;

?>

<?php

$newlinecounter = 0;
foreach($_POST as $key => $val_newline){
if(stristr($val_newline, '\r')){$newlinecounter++;}
if(stristr($val_newline, '\n')){$newlinecounter++;}
if(stristr($val_newline, '\\r')){$newlinecounter++;}
if(stristr($val_newline, '\\n')){$newlinecounter++;}
if(stristr($val_newline, '\r\n')){$newlinecounter++;}
if(stristr($val_newline, '\\r\\n')){$newlinecounter++;}
if(stristr($val_newline, 'Bcc')){$newlinecounter++;}
}
if ($newlinecounter >= 1){ die('There was an error with your submission, please try again.');}

?>



<?php
// checks if bot
if ($_POST['address'] != '' ){
die("Changed field");}

?>

PHP:

Thanks in advance for the help.

 

Just mark the inputs "required" in the HTML (which will deter non-malicious users who are just doing something wrong), and then do a check serverside for empty or just wrong inputs.
Btw - in the code above, why do you have three assignments to $email? $email will always contain $_POST['message'] in the above code.
And what's the point of the newline-counter?

 

Ah, the $email x 3 is a mistake. The code was put together for me by a friend (who I cant get hold of at the moment to help), but I believe the newline counter is something to do with stopping the form being attacked?

PHP is not my strong point :/

 

Thanks for that.

What would you recommend for SQL injections? Not even sure what they are.

 

As long as this is a pure contact form, and not something you also store in a database, pure SQL injection is probably not something you need to be too concerned about. However, cleaning the input (never, ever trust input made by a user) before sending it, limiting sending to just one email-address, etc. would probably make the form more secure.

 

Some advice:
1) STOP making "variables for nothing" -- it's just a waste of memory making copies of things that already have values.

2) don't just blindly dump them into things like $header without any sort of cleaning / sanitation.

3) That newline check is cute, but flawed in that:

A> you are parsing ALL $_POST values, and really you SHOULD be allowing those characters in $_POST[message]

B> You should be doing that BEFORE you even THINK about building $header or calling 'mail'.

C> filtering out the bad characters with a simple regex instead of aborting would probably be a far better approach.

D> Some real checks of values like seeing if it's a valid e-mail address would be far more useful.

E> Even if you were to do that type of check, you should be short-circuit evaluating so once you know there's a 'invalid' character or combination of characters, you STOP running checks you don't need to. Once you know there's at least ONE \r, you can STOP and die, you don't need to keep going with the other checks.

4) for the most compatibility, you should probably be using \r\n instead of just \r... and sending the X-Mailer property might not be a bad idea either.

5) more of a personal preference, I prefer to put the "contact form" notice as part of the FROM and allow the user's entered subject to be the mail subject.

My approach would go something like this -- note this version is untested and may have typos or other issues, but should give you the gist of it.

<?php

/*
	We'll assume there's a template_contactForm() function that is,
	well... the contact form. It should check for the existance of 
	$formErrors and if present, show them.
	
	I'll also be using some PHP 5.4+ only constructs here...
*/

$formErrors = [];
/*
	$requireds contains the names of fields that must not be left blank
	in your form. 
*/
$requireds = [
	'email', 'name', 'subject', 'message'
];

function cleanPost($index) {
	return preg_replace('/\s+/', ' ', $_POST[$index]);
}

function isValidEmail($address) {
	if (filter_var($address,FILTER_VALIDATE_EMAIL)==FALSE) {
		return false;
	}
	/* explode out local and domain */
	list($local,$domain)=explode('@',$address);
	
	$localLength=strlen($local);
	$domainLength=strlen($domain);
	
	return (
		/* check for proper lengths */
		($localLength>0 && $localLength<65) &&
		($domainLength>3 && $domainLength<256) &&
		(
			checkdnsrr($domain,'MX') ||
			checkdnsrr($domain,'A')
		)
	);
}

if (
	isset($_POST['fromForm']) &&
	($_POST['fromForm] == 'contactForm')
) {
	// check required fields
	foreach ($required as $index) {
		if (!isset($_POST[$index])) $formErrors[$index] = 'You must enter a value here.';
	}
	
	// check for valid e-mail
	if (
		(count($formErrors) == 0) &&
		!isValidEmail($_POST['email'])
	) $formErrors['email'] = 'Invalid E-mail Address';
	
	if (count($formErrors) > 0) {
		template_contactForm();
	} else {
	
		$message = 'Name: ' . ($name = cleanPost['name']) . "\r\n" . (
			isset($_POST['company']) ?
			'Company: ' . cleanPost('company') . "\r\n" :
			''
		) . 'Message: ' . $_POST['message'];
		
		mail(
			'admin@anaddress.com',
			cleanPost['subject'],
			$message,
			'From: CONTACT FORM - ' . $name . '<' . ($from = cleanPost['email']) . ">\r\n" . 
			'Reply-To: ' . $from . "\r\n" .
			'X-Mailer: PHP/' . phpversion()
		);
		
		/*
			put message that mail was sent successfully here
		*/
		
	}
	
} else template_contactForm();

?>

Code (markup):

It expects that your form would have a hidden input called "fromForm" containing the value "contactForm" -- a better version would store a hash in a session value with an expiration and send that in the form instead. Expire it and re-send a new one on every user interaction as a second stage session verification (in addition to regenerating the session ID) -- you'd be shocked how many spambots that makes fall flat on their face, and it also prevents accidental re-submits.

really though, the big magic is this:

function cleanPost($index) {
	return preg_replace('/\s+/', ' ', $_POST[$index]);
}

Code (markup):

Which strips out whitespace and treats it like HTML does, replacing ALL long runs of whitespace characters with single spaces. No CRLF, no way someone can inject something like BCC: into the header since commands are only obeyed at the start of a line.

I also don't bother cleaning the message since there's little if anything you can put into $message that would screw things up -- well, at least not until you get into multi-part sending.

It is also meant to be a "one contact.php" format, where the same .php is called to show the form as is used to submit the form. It's just a superior approach in most cases since it means none of that header redirect bull a lot of people crap all over their sites with, and it also means a single entry vector. IDEALLY I like to build all my websites using the "one index to rule them all" approach for the same reason -- one single entry and exit vector for ALL site requests means you only need one gate and one guard posted at that gate. It's what in military parlance is called a "force multiplier"

Hope that helps and doesn't confuse you too much.

 

In addition to the above I'd check form fields against a fields name white-list array to make sure only your intended fields are sent.