PHP Developers Network
http://forums.devnetwork.net/

Please help me secure my script
http://forums.devnetwork.net/viewtopic.php?f=34&t=131604
Page 1 of 1

Author:  Bel [ Mon Sep 12, 2011 11:42 pm ]
Post subject:  Please help me secure my script

Hello

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.

Syntax: [ Download ] [ Hide ]

      $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
        date=now(),
        firstname='"
.$card_data['Firstname']."',
        lastname='"
.$card_data['Lastname']."',
        address='"
.$card_data['Address']."',
        suburb='"
.$card_data['Suburb']."',
        city='"
.$card_data['City']."',
        phone='"
.$card_data['Phone']."',
        email='"
.$card_data['Email']."',
        amount='"
.$card_data['Amount']."'
        "
;
        mysql_query($query) or die (mysql_error());
 


What would you do?

Author:  twinedev [ Tue Sep 13, 2011 3:00 am ]
Post subject:  Re: Please help me secure my script

Here is an example of something that will work:
Syntax: [ Download ] [ Hide ]
<?php

        // 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;
                        }
                }

                // OPTIONAL, IF ALL VALUES ARE REQUIRED
                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:

Syntax: [ Download ] [ Hide ]
                $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() )';
 

Author:  Mordred [ Tue Sep 13, 2011 4:13 am ]
Post subject:  Re: Please help me secure my script

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.

Author:  Bel [ Tue Sep 13, 2011 7:05 pm ]
Post subject:  Re: Please help me secure my script

WOw thats fantastic effort twinedev.

Cheers for that and yes ill be implementing that!

ps: what exactly is prepared statements?

Page 1 of 1 All times are UTC - 5 hours
Powered by phpBB® Forum Software © phpBB Group
http://www.phpbb.com/