Please help me secure my script

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
Forum Newbie
Posts: 6
Joined: Wed Jan 19, 2011 10:04 pm

Please help me secure my script

Post by Bel »


I have some good understanding of PHP mysql sanitization but really want to know if what im currently doing is the best possible security.

Data is coming from a form then passed via ajax to a php file for uploading to mysql. Currently zero sanitization.

Code: Select all

      $data1 = $_GET["data1"];

      $data = array();
      $data = explode("|", $data1);
        foreach ($data as $value) {            
            $data2 = explode(":", $value);
            $data[$data2[0]] = $data2[1];        
        $query = "INSERT INTO transactions SET
        mysql_query($query) or die (mysql_error());
What would you do?
User avatar
Forum Regular
Posts: 984
Joined: Tue Sep 28, 2010 11:41 am
Location: Columbus, Ohio

Re: Please help me secure my script

Post by twinedev »

Here is an example of something that will work:

Code: Select all


	// Make sure the passed information looks properly formatted
	if (preg_match('/^([a-z]+:[^|]+\|)+$/i',$_GET['data1'].'|')) {
		$data = array_fill_keys(array('Firstname','Lastname','Address','Suburb','City','Phone','Email','Amount'),'');
		$pieces = explode('|', $_GET['data1']);
		foreach($pieces as $pair) {
			list($key,$val) = explode(':',$pair,2);
			if (array_key_exists($data,$key)) {
				$data[$key] = $val;

		foreach($data as $key=>$val) {
			if ($val=='') {
				die ('[ERR:'.__LINE__.'] Missing value for '.$key);

		$query = 'INSET INTO `transactions` SET ';
		foreach($data as $key=>$val) {
			$query .= '`'.strtolower($key).'` = "'. mysql_real_escape_string($val).'",';
		$query .= '`date`=NOW()';

		mysql_query($query) or die ('[ERR:'.__LINE__.'] ' . mysql_error());
	else {
		die ('[ERR:'.__LINE__.'] Invalid parameters passed');

Here it is spelled out...

1. Make sure that we are get a properly passed string, of key:value|key:value|key:value. Note that the regular expression looks for a | after each set, so for the check, I add an extra one to the end to match.

2. Create an array $data that contain all the fields, set to an empty string to start with.

3. Break the string to groups like you did, then loop through those groups doing the following:

3a. Makes sure you only explode to two elements, so that you can actually have a colon in the data values

3b. Only if the key already exists in $data assign it, otherwise it gets skipped

4. This part was optional, however if all values are supposed to be required, loop through $data to make sure all are set to something (otherwise error with message)

5. Build up the query. Normally I would do INSERT INTO `table` (`field1`,`field2`,`field3`) VALUES ("value1","value2","value3) To be honest, I have been doing SQL so long, I didn't know they they adapted to allow the INSERT INTO `table` SET `field1`="value1", `field2`="value2", `field3`="value3". Learn something new every day even after 11 years programming php/mysql

5a. Since all the fields are in $data, we just loop through it to build up pairs. Note that each loop places a comma at the end of the string, see next step. Note also that I am properly escaping the value so that SQL injection doesn't happen.

5b. I add the DATE field here, instead of the beginning. I do this since the query at this point is already ending in a comma, ready for another set. Otherwise I would have had to trim the ending comma off.

6. execute the query, if failed, error out.

7. The else for the initial preg_match, if it hits here, the data wasn't passed correctly, so give the correct error.

Hope this helps, and again, thank you for opening my eyes to the alternative INSERT method. It makes the code more readable. Using the way I always did it before, I would have done:

Code: Select all

		$query1 = 'INSET INTO `transactions` (';
		$query2 = ') VALUES (';
		foreach($data as $key=>$val) {
			$query1 .= '`'.strtolower($key).'`,';
			$query2 .= '"'. mysql_real_escape_string($val).'",';
		$query = $query1 . '`date`' . $query2 . 'NOW() )';
User avatar
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Please help me secure my script

Post by Mordred »

1. Use trigger_error instead of die(), it will do the right thing both on your developer setup and on the production server
2. Check if $_GET['data1'] exists and is a string

There are things that can go wrong depending on the db setup here, if one of the strings is larger than the field size in the DB, but they would hardly make a difference security-wise with this table. It can be dangerous if it happens on the login table, though so you must be aware of that. Otherwise it looks ok.
Forum Newbie
Posts: 6
Joined: Wed Jan 19, 2011 10:04 pm

Re: Please help me secure my script

Post by Bel »

WOw thats fantastic effort twinedev.

Cheers for that and yes ill be implementing that!

ps: what exactly is prepared statements?
Post Reply