Securing form

[chrisdee]

Hi,

I wonder if anyone could take a look at my script below in regards to securing the form data agains injection/hacking ? I specifically wonder if it's enough to run escapeshellcmd and htmlspecialchars in the form sentance/line or if I should run these functions on all input fields?

I also welcome suggestions in reguard to my way of checking how the input fields should be echo'ed. Maby there is a smarter, simpler, better way to avoid all the if else lines?

<?php 
$arr = ['dato','navn','epost','adresse','postnr','poststed','vare/tjeneste','ksted','analysenr','prosjektnr']; 

if (empty($_POST)) { 
 echo "<form action='".escapeshellcmd(htmlspecialchars($_SERVER[PHP_SELF]))."' method='post' autocomplete='off'>"; 
 echo "<table border=0 width=505px cellpadding=2>"; 
 echo "<tr><td height=30px bgcolor=lightgrey><strong>Reiseskjema</strong></td><td bgcolor=lightgrey align=right><a href=".$_SERVER[PHP_SELF]."><img src='img/ntnu_b.png'></a></td><$ 
 echo "<tr><td colspan=2></td></tr>"; 

 foreach($arr as $val) { 
  if($val=='dato') { 
   echo "<tr><td>" . ucfirst($val) . "</td><td><input type='date' name='$val' placeholder='".ucfirst($val)."' required='".$val."' size=50 min='1' max='100'></td></tr>"; 
  } 
  elseif($val=='navn') { 
   echo "<tr><td>" . ucfirst($val) . "</td><td><input type='text' name='$val' placeholder='Fornavn Etternavn' required='".$val."' size=50 min='1' max='100'></td></tr>"; 
  } 
  elseif($val=='epost') { 
   echo "<tr><td>" . ucfirst($val) . "</td><td><input type='email' name='$val' placeholder='[e-mail]' required='".$val."' size=50 min='1' max='100'></td></tr>"; 
  } 
  elseif($val=="vare/tjeneste") { 
   echo "<tr><td>".ucfirst($val)."</td><td><select name='".$val."'><option>".ucfirst($val)."</option><option value='Hotell'>Hotell</option><option value='Fly'>Fly</option></select$ 
  } 
  elseif($val=="poststed") { 
   echo "<tr><td>".ucfirst($val)."</td><td><input type='text' name='$val' value='Trondheim' readonly size=50 min='1' max='100'></td></tr>"; 
  } 
  elseif($val=="ksted") { 
   echo "<tr><td>".ucfirst($val)."</td><td><input type='number' name='$val' value='626005' readonly size=50 min='1' max='100'></td></tr>"; 
  } 
  elseif($val=="postnr") { 
   echo "<tr><td>".ucfirst($val)."</td><td><input type='number' name='$val' value='7491' readonly size=50 min='1' max='100'></td></tr>"; 
  } 
  elseif(empty($_POST[$val])) { 
   echo "<tr><td>" . ucfirst($val) . "</td><td><input type='text' name='$val' placeholder='".ucfirst($val)."' size=50 min='1' max='100'></td></tr>"; 
  } 
 } 
 echo "<tr><td colspan='2'><input type='submit' value='Lagre / last ned skjema'></td></tr></table></form>"; 
} 

else { 
 echo "<table border=0 width=505px cellpadding=2>"; 
 echo "<tr><td height=30px bgcolor=lightgrey><strong>Reiseskjema</strong></td><td bgcolor=lightgrey align=right><a href=".$_SERVER[PHP_SELF]."><img src='http://dillner.net/scripts$ 
 foreach($arr as $val) { 
  echo "<tr><td>".$val."</td><td><input type='text' name='".$val."' value='".$_POST[$val]."' size=50></td></tr>"; 
 } 
 echo "</table>"; 
 header("Content-Disposition: attachment;Filename=reiseskjema.doc"); // Open as word file 
} 
?>

[Bonesnap]

Regarding the construction of the form, is there are a reason why all the fields are initially stored in an array, and then iterated through? Can't you just write the form in plain HTML? You can still surround the whole thing with an if statement to show/hide it. Also, avoid using tables for layout and positioning. There are so many better alternatives to using tables. Tables should be reserved for tabular data.

[NogDog]

escapshellcmd() has no purpose here, since you are not using anything in a shell command.

htmlentities() would make sense here, though, for the $POST[$val] value:

	foreach($arr as $val)
	{
		echo "<tr><td>" . $val . "</td><td><input type='text' name='" . $val . "' value='" . $_POST[$val] . "' size=50></td></tr>";
	}