Page 1 of 1

See any major security problems with the following code?

Posted: Mon Aug 20, 2007 7:21 am
by dbrock178
Any thoughts on improving the code would be appreciated. Thank you.

validation.inc.php

Code: Select all

<?php
	function validateorder($string,$name)
	{
		//First check for blank values (DL# is Optional)
		if ($name!="DL#" && $string==""  )
			die ("Missing $name Information");

		//Next remove excess spaces and convert to uppercase
		$string=trim(strtoupper($string));
		
		//Make sure values are A-Z, 0-9, Space, Hyphen, Period, @ symbol, # sign
		for($loop=0;$loop<strlen($string);$loop++)
		{
			if( !(($string[$loop]>='A' && $string[$loop]<='Z') || ($string[$loop]>=0 && $string[$loop]<=9) || $string[$loop]==' ' || $string[$loop]=='-' || $string[$loop]=='.' || $string[$loop]=='@'||$string[$loop]=='#'))
			{
				die ("Invalid Character In $name");
			}
		}
		return ($string);
	}
	function validatecomment($string,$name)
	{
		//First check for blank values
		if ($string==""  )
			die ("Missing $name Information");

		//Next remove excess spaces and convert to uppercase
		$string=trim(strtoupper($string));
		
		//Make sure values are A-Z, 0-9, Space, Hyphen, Period, @ symbol, ?, Comma, Colon, # sign, '\r', '\n'
		for($loop=0;$loop<strlen($string);$loop++)
		{
			if( !(($string[$loop]>='A' && $string[$loop]<='Z') || ($string[$loop]>='0' && $string[$loop]<='9') || $string[$loop]==' ' || $string[$loop]=='-' || $string[$loop]=='.' || $string[$loop]=='@' || $string[$loop]=='?' || $string[$loop]==',' || $string[$loop]==':' || $string[$loop]=='#'|| $string[$loop]=="\r" || $string[$loop]=="\n")) 
			{
				die ("Invalid Character In $name");
			}
		}
		return ($string);
	}
?>

order.php

Code: Select all

<html>
<head><title>Order</title></head>
<body>
<?
	//Include Comment Validation
	include("validation.inc.php");

	//Get Value From Users
	$dlnumber=validateorder($_POST['dlnumber'],'DL#');
	$dlfname=validateorder($_POST['dlfname'],'Driver License First Name');
	$dlmname=validateorder($_POST['dlmname'],'Driver License Middle Name');
	$dllname=validateorder($_POST['dllname'],'Driver License Last Name');
	$dladdress=validateorder($_POST['dladdress'],'Driver License Address');
	$dlcity=validateorder($_POST['dlcity'],'Driver License City');
	$dlzip=validateorder($_POST['dlzip'],'Driver License Zip');	
	$dldob=validateorder($_POST['dldob'],'Driver License DOB');
	$shipname=validateorder($_POST['shipname'],'Shipping Name');
	$shipaddress=validateorder($_POST['shipaddress'],'Shipping Address');
	$shipcity=validateorder($_POST['shipcity'],'Shipping City');
	$shipstate=validateorder($_POST['shipstate'],'Shipping State');
	$shipzip=validateorder($_POST['shipzip'],'Shipping Zip');
	$email=validateorder($_POST['email'],'Email');
	
	//Check That File Is Zip File And Size Is Greater Than 0b And 300kb Or Less
	if ($_FILES["filez"]["type"]!="application/x-zip-compressed" )
		die("File Is Not Zipped");
	if ($_FILES["filez"]["size"] <1 || $_FILES["filez"]["size"] > 300000 )
		die ("Bad File Size");

	//Connect To Database
	$username="myuser";
	$password="mypass";
	$database="orders";
	mysql_connect("localhost",$username,$password) or die("Unable To Connect To Server");
	@mysql_select_db($database) or die( "Unable to select database");	
	
	//Insert Current Time, Status, And Users Email Address Into Database
	$query="INSERT INTO orders (orderdate,status,email) VALUES (now(),'Pending','$email')";
	mysql_query($query) or die("Error Inserting Info Into Database");
	
	//Find orderid (auto_incremented) From Database
	$query="SELECT orderid FROM orders WHERE email= '$email' ";
	$result=mysql_query($query);
	
	//Take Last Entry of Email Address
	$num=mysql_numrows($result);
	$orderid=mysql_result($result,$num-1,"orderid");
	
	//Write Data To File
	$myFile = "Orders\\$orderid".".txt";
	$fh = fopen($myFile, 'w') or die ("Could Not Open File For Writing");
	$string=  "Order ID: $orderid\r\n".
		"DL#: $dlnumber\r\n".
	                "Name: $dlfname $dlmname $dllname\r\n".
		"Address: $dladdress\r\n".
		"City: $dlcity\r\n".
		"Zip: $dlzip\r\n".
		"DOB: $dldob\r\n\r\n".
		"Shipping Name: $shipname\r\n".
		"Shipping Address: $shipaddress\r\n".
		"Shipping City: $shipcity\r\n".
		"Shipping State: $shipstate\r\n".
		"Shipping Zip: $shipzip\r\n".
		"Email: $email";
	fwrite($fh, $string);
	fclose($fh);

	//Move User Uploaded Picture File
	$mypic="Orders\\".$orderid.".zip";
	move_uploaded_file($_FILES["filez"]["tmp_name"], $mypic);
	
	//Encrypt Files
	passthru("gpg --always-trust -a -r jimmy -e $myFile",$retval);
	if($retval)
	{
		die("Error Encrypting Data File");
	}	
	passthru("gpg --always-trust -a -r jimmy -e $mypic",$retval);
	if($retval)
	{
		die("Error Encrypting Picture File");
	}
	
	//Securely Delete Non Encrypted Files
	passthru("sd $myFile",$retval);
	if($retval)
	{
		die("Error Deleting Data File");
	}	
	passthru("sd $mypic",$retval);
	if($retval)
	{
		die("Error Deleting Picture File");
	}
	echo "<center><h2>Your Order ID is</h2> <h1> $orderid</h1></center>";
		
?>
</body>
</html>
contact.php

Code: Select all

<?php

	//Include Comment Validation
	include("validation.inc.php");

	//Get Values From User
	$subject=validatecomment($_POST['subject'],'subject');
	$comment=validatecomment($_POST['comment'],'message');

	//Email Comments To Me
	//mail('myaddress@aol.com',$subject,$comment);
?>
checkorder.php

Code: Select all

<?php
	//Include Order Validation
	include("validation.inc.php");
	
	//Get Values From User
	$orderid=validateorder($_POST['orderid'],'Order ID');
	$email=validateorder($_POST['email'],'Email');

	//Connect To Database
	$username="myuser";
	$password="mypass";
	$database="orders";

	mysql_connect('localhost',$username,$password) or die ("Error Connecting To Database");
	@mysql_select_db($database) or die( "Unable To Select Database");

	//Check Database For Records
	$query="SELECT * FROM orders WHERE orderid='$orderid' AND email='$email'";
	$result=mysql_query($query);	
	$num=mysql_numrows($result);
	
	//No Records Found
	if ($num==0)
	{
		echo "<center><h1>Order Not Found</h1></center>";
	}
	else
	{
		$orderid=mysql_result($result,0,"orderid");
		$status=mysql_result($result,0,"status");
		$tracking=mysql_result($result,0,"tracking");
		echo "<center><table><tr><td>Order ID</td><td>Status</td><td>Tracking</td></tr>".
                   	         "<tr><td>$orderid</td><td>$status</td><td>$tracking</td></tr></table></center>";
	}
	mysql_close();
?>

Posted: Mon Aug 20, 2007 8:28 am
by miro_igov
Woow

Code: Select all

/Make sure values are A-Z, 0-9, Space, Hyphen, Period, @ symbol, # sign
                for($loop=0;$loop<strlen($string);$loop++)
                {
                        if( !(($string[$loop]>='A' && $string[$loop]<='Z') || ($string[$loop]>=0 && $string[$loop]<=9) || $string[$loop]==' ' || $string[$loop]=='-' || $string[$loop]=='.' || $string[$loop]=='@'||$string[$loop]=='#'))
                        {
                                die ("Invalid Character In $name");
                        }
                }
Have you heard about regular expressions? We have a forum about these.

Posted: Mon Aug 20, 2007 10:05 am
by Mordred
# may cause error in SQL context.
\r and \n are not safe for use in the mail() function.
checkorder.php allows XSS
die() is not a useful thing to do when an error occurs.

Posted: Thu Aug 30, 2007 1:24 am
by dbrock178
@miro_igov
Thanks for the tip about regular expressions.

@Mordred
I've read up some on XSS, however I am still not fully clear on it. I changed my validation function to two different functions on the checkorder.php. The orderid can now only be digits and the email can only be letters, numbers, dash, underscore, @ symbol, and periods. Does this still mean the page is still vulnerable to XSS? If so could you show me where the problem is? Also, what you recommend doing on an error besides die()?

Posted: Thu Aug 30, 2007 4:18 am
by Mordred
dbrock178 wrote: I've read up some on XSS, however I am still not fully clear on it. I changed my validation function to two different functions on the checkorder.php. The orderid can now only be digits and the email can only be letters, numbers, dash, underscore, @ symbol, and periods. Does this still mean the page is still vulnerable to XSS? If so could you show me where the problem is? Also, what you recommend doing on an error besides die()?
Sounds good, but without seeing actual code I can't say if it is good.
The problem with die() is that it always prints to the output. trigger_error() with E_USER_ERROR is a better alternative, in your dev environment you can set it to print to the output, while on a production server it would print to the error log.

Posted: Thu Aug 30, 2007 11:05 pm
by dbrock178
Ok hopefully this will need no corrections.

validation.inc.php

Code: Select all

<?php

ini_set('error_reporting', E_ALL);
error_reporting(E_ALL);
ini_set('log_errors',TRUE);
ini_set('html_errors',FALSE);
ini_set('error_log','php_error_logs.txt');
ini_set('display_errors',FALSE);

function dlnumbervalidate($string)
{
	if($string!="")
	{
		$string=trim(strtoupper($string));
		if(!preg_match('/^[A-Z]{1}[0-9]{7}$/',$string))
		{
			die("DL Number Format is CDDDDDDD<br>
			Where C is a Character and D is Digit <br>
			Press The Back Button On Your Browser");
		}
	}
	return $string;
}
function namevalidate($string,$name)
{
	if ($string=="")
	{
		die("$name Name cannot be blank<br>
		      Press The Back Button On Your Browser");
	}
	$string=trim(strtoupper($string));	
	if(!preg_match('/^[A-Z]+$/',$string))
	{
		die("$name Name can only be characters<br>
		      Press The Back Button On Your Browser");
	}
	return $string;
}
function addressvalidate($string,$name)
{
	if($string=="")
	{
		die("$name Address cannot be blank<br>
		      Press The Back Button On Your Browser");
	}
	$string=trim(strtoupper($string));	
	if(!preg_match('/^[0-9]+[\040][A-Z\040]+[0-9]{0,4}$/',$string))
	{
		die("$name Address is in invalid format<br>
		      Press The Back Button On Your Browser");
	}
	return $string;
}
function cityvalidate($string,$name)
{
	if($string=="")
	{
		die("$name City cannot be blank<br>
		      Press The Back Button On Your Browser");
	}
	$string=trim(strtoupper($string));
	if(!preg_match('/^[A-Z\040]+$/',$string))
	{
		die("$name City can only be characters and spaces<br>
		      Press The Back Button On Your Browser");
	}
	return $string;
}
function statevalidate($string)
{
	if($string=="")
	{
		die("Shipping State cannot be blank<br>
		      Press The Back Button On Your Browser");
	}
	$string=trim(strtoupper($string));
	if(!preg_match('/^[A-Z]{2}$/',$string))
	{
		die("Use abbrivation for Shipping State<br>
		      Press The Back Button On Your Browser");
	}
	return $string;
}
function zipvalidate($string,$name)
{
	if($string=="")
	{
		die("$name Zip cannot be blank<br>
		      Press The Back Button On Your Browser");
	}
	$string=trim(strtoupper($string));
	if(!preg_match('/^[0-9]{5}$/',$string))
	{
		die("$name Zip can only be 5 digits<br>
		      Press The Back Button On Your Browser");
	}
	return $string;
}
function dobvalidate($string)
{
	if($string=="")
	{
		die("DOB cannot be blank<br>
		      Press The Back Button On Your Browser");
	}
	if(!preg_match('/^[0-9]{2}[-][0-9]{2}[-][0-9]{2}$/',$string))
	{
		die("DOB must be in MM-DD-YY format<br>
		      Press The Back Button On Your Browser");
	}
	//Make sure user doesn't enter month >12 or date >31
	if($string[0]=="1" && $string[1]>"2")
	{
		die("Month cannot be greater than 12<br>
		      Press The Back Button On Your Browser");
	}	
	if($string[3]>="3"&&$string[4]>"1")
	{
		die("Day cannot be greater than 31<br>
		      Press The Back Button On Your Browser");
	}	
	return $string;
}
function emailvalidate($string)
{
	if($string=='')
	{
		die("Email Address Cannot Be Blank<br>
		      Press The Back Button On Your Browser");
	}	
	$string=trim(strtoupper($string));
	if(!preg_match('/^[A-Z0-9._-]+@[A-Z0-9_-]+\.[A-Z]{2,4}$/',$string))
	{
		die("Invalid Email Address<br>
		      Press The Back Button On Your Browser");
	}
	return $string;	
}
function commentvalidate($string)
{
	if($string=="")
	{
		die("Comment Cannot Be Blank<br>
		      Press The Back Button On Your Browser");
	}
	$string=trim(strtoupper($string));
	//Allow digits, characters, Exc pt, Question mark, Period, underline, @, colon, comma, space, "\r","\n", dash
	if(!preg_match('/^[A-Z0-9!?._@:,\040\012\015-]+$/',$string))		
	{
		die("Comments may only include<br>
		      Letters, Digits, Exclamation point, Question mark, Periods, Underline, Dashes, @ symbol, Colons, Commas, and Spaces<br>
		      Press The Back Button On Your Browser");
	}
	//strip "\r" "\n"
	$string = str_replace("\n", "", $string);
	$string = str_replace("\r", "", $string);
	return $string;
}
function ordervalidate($string)
{
	if($string=="")
	{
		die("Order Number Cannot Be Blank<br>
		      Press The Back Button On Your Browser");
	}
	if(!preg_match('/^[0-9]+$/',$string))
	{
		die("Order ID can only be digits<br>
		      Press The Back Button On Your Browser");
	}
	return $string;	
}	
?>
order.php

Code: Select all

<html>
<head><title>Order</title></head>
<body>
<?php
	//Include Comment Validation
	include("validation.inc.php");

	//Get Value From Users
	$dlnumber=dlnumbervalidate($_POST['dlnumber']);
	$dlfname=namevalidate($_POST['dlfname'],'Driver License First');
	$dlmname=namevalidate($_POST['dlmname'],'Driver License Middle');
	$dllname=namevalidate($_POST['dllname'],'Driver License Last');
	$dladdress=addressvalidate($_POST['dladdress'],'Driver License');
	$dlcity=cityvalidate($_POST['dlcity'],'Driver License');
	$dlzip=zipvalidate($_POST['dlzip'],'Driver License');	
	$dldob=dobvalidate($_POST['dldob'],'Driver License DOB');
	$shipfname=namevalidate($_POST['shipfname'],'Shipping First');
	$shiplname=namevalidate($_POST['shiplname'],'Shipping Last');
	$shipaddress=addressvalidate($_POST['shipaddress'],'Shipping');
	$shipcity=cityvalidate($_POST['shipcity'],'Shipping');
	$shipstate=statevalidate($_POST['shipstate']);
	$shipzip=zipvalidate($_POST['shipzip'],'Shipping');
	$email=emailvalidate($_POST['email']);
	
	//Check That File Is Zip File And Size Is Greater Than 0b And 300kb Or Less
	if ($_FILES["filez"]["type"]!="application/x-zip-compressed" && $_FILES["filez"]["type"]!="application/zip")
	{
		echo "File Not Zipped";
		$msg=$_FILES["filez"]["type"]. " Attempted Upload";
		trigger_error($msg,E_USER_ERROR);
		exit();
	}	
	if ($_FILES["filez"]["size"] <1 || $_FILES["filez"]["size"] > 300000 )
	{
		echo "File Size Problem";
		$msg=$_FILES["filez"]["size"]. " Size Attempted Upload";
		trigger_error($msg,E_USER_ERROR);
		exit();
	}	

	//Connect To Database
	$username="myuser";
	$password="mypass";
	$database="orders";
	
	if(!mysql_connect("localhost",$username,$password))
	{
		echo "Error Connecting To Database";
		trigger_error('Connection To Database',E_USER_ERROR);
		exit();
	}
	if(!@mysql_select_db($database))
	{
		echo "Error With Database";
		trigger_error('Select Database',E_USER_ERROR);
		exit();
	}
	
	//Insert Current Time, Status, And Users Email Address Into Database
	$query="INSERT INTO orders (orderdate,status,email) VALUES (now(),'Pending','$email')";
	if (!mysql_query($query))
	{
		echo "Error With Database";
		trigger_error('Inserting To Database',E_USER_ERROR);
		exit();
	}
	
	//Find orderid (auto_incremented) From Database
	$query="SELECT orderid FROM orders WHERE email= '$email' ";
	$result=mysql_query($query);
	
	//Take Last Entry of Email Address
	$num=mysql_numrows($result);
	$orderid=mysql_result($result,$num-1,"orderid");
	
	//Write Data To File
	$myFile = "Orders\\$orderid".".txt";
	$fh = fopen($myFile, 'w') or die ("Could Not Open File For Writing");
	$string=  "Order ID: $orderid\r\n".
		"DL#: $dlnumber\r\n".
	                "Name: $dlfname $dlmname $dllname\r\n".
		"Address: $dladdress\r\n".
		"City: $dlcity\r\n".
		"Zip: $dlzip\r\n".
		"DOB: $dldob\r\n\r\n".
		"Shipping Name: $shipfname $shiplname\r\n".
		"Shipping Address: $shipaddress\r\n".
		"Shipping City: $shipcity\r\n".
		"Shipping State: $shipstate\r\n".
		"Shipping Zip: $shipzip\r\n".
		"Email: $email";
	fwrite($fh, $string);
	fclose($fh);

	//Move User Uploaded Picture File
	$mypic="Orders\\".$orderid.".zip";
	move_uploaded_file($_FILES["filez"]["tmp_name"], $mypic);
	
	//Encrypt Files
	passthru("gpg --always-trust -a -r jimmy -e $myFile",$retval);
	if($retval)
	{
		echo "Error With Data Encryption";
		trigger_error('Data Encryption Error',E_USER_ERROR);
		exit();
	}	
	passthru("gpg --always-trust -a -r jimmy -e $mypic",$retval);
	if($retval)
	{
		echo "Error With Picture Encryption";
		trigger_error('Picture Encryption Error',E_USER_ERROR);
		exit();
	}
	
	//Securely Delete Non Encrypted Files
	/*
	passthru("sd $myFile",$retval);
	if($retval)
	{
		echo "Error With Data Encryption";
		trigger_error('Data Delete FAILED',E_USER_ERROR);
		exit();
	}	
	passthru("sd $mypic",$retval);
	if($retval)
	{
		echo "Error With Picture Encryption";
		trigger_error('Picture Delete FAILED',E_USER_ERROR);
		exit();
	}*/
	echo "<center><h2>Your Order ID is</h2> <h1> $orderid</h1></center>";
		
?>
</body>
</html>
checkorder.php

Code: Select all

<?php
	//Include Validation
	include("validation.inc.php");

	//Get Values From User
	$orderid=ordervalidate($_POST['orderid']);
	$email=emailvalidate($_POST['email']);

	//Connect To Database
	$username="myuser";
	$password="mypass";
	$database="orders";

	if(!mysql_connect('localhost',$username,$password))
	{
		echo "Error Connecting To Database";
		trigger_error('Connection To Database',E_USER_ERROR);
		exit();
	}
	
	if(!@mysql_select_db($database) )
	{
		echo "Error With Database";
		trigger_error('Select Database',E_USER_ERROR);
		exit();
	}
	
	//Check Database For Records
	$query="SELECT * FROM orders WHERE orderid='$orderid' AND email='$email'";
	$result=mysql_query($query);	
	$num=mysql_numrows($result);

	//No Records Found
	if ($num==0)
	{
		echo "<center><h1>Order Not Found</h1></center>";
	}
	else
	{
		$orderid=mysql_result($result,0,"orderid");
		$status=mysql_result($result,0,"status");
		$tracking=mysql_result($result,0,"tracking");
		echo "<center><table><tr><td>Order ID</td><td>Status</td><td>Tracking</td></tr>".
                   	         "<tr><td>$orderid</td><td>$status</td><td>$tracking</td></tr></table></center>";
	}
	mysql_close();
?>
contact.php

Code: Select all

<?php
	//Include Validation
	include("validation.inc.php");
	
	$email=emailvalidate($_POST['email']);
	$subj=commentvalidate($_POST['subj']);
	$msg=commentvalidate($_POST['msg']);
	
	$msg="Mail from".$email." ".$msg;

	//Uncomment when moved over to server
	//mail('myaddress@aol.com',$subj,$msg);
?>

Posted: Fri Aug 31, 2007 2:50 am
by Mordred
\r and \n are still allowed in comments to be mailed, which is a problem
$_FILES["filez"]["type"] is nor reliable, although in your case you're doing the right thing and generating a file name and forcing the extension to .zip -- well done

emailvalidate will not allow user@server.co.uk -- email validation with regexps is NOT trivial, search in the forum.
There's still lots of die()-s. Maybe one day you'll want the error to appear in a nicely done HTML page, what will you do then?

Code: Select all

//Find orderid (auto_incremented) From Database 
 $query="SELECT orderid FROM orders WHERE email= '$email' ";

Code: Select all

mysql_insert_id()
Too long to otherwise check this thoroughly, but this is what I got from a diagonal scan.

Oh, and make sure that "Orders" is either out of the htdocs tree, or is protected by .htaccess