PHP Developers Network

A community of PHP developers offering assistance, advice, discussion, and friendship.
 
Loading
It is currently Tue Aug 04, 2020 2:50 pm

All times are UTC - 5 hours




Post new topic Reply to topic  [ 4 posts ] 
Author Message
PostPosted: Mon Sep 12, 2011 11:42 pm 
Offline
Forum Newbie

Joined: Wed Jan 19, 2011 11:04 pm
Posts: 6
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?


Top
 Profile  
 
PostPosted: Tue Sep 13, 2011 3:00 am 
Offline
Forum Regular
User avatar

Joined: Tue Sep 28, 2010 11:41 am
Posts: 984
Location: Columbus, Ohio
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() )';
 


Top
 Profile  
 
PostPosted: Tue Sep 13, 2011 4:13 am 
Offline
DevNet Resident
User avatar

Joined: Sun Sep 03, 2006 5:19 am
Posts: 1579
Location: Sofia, Bulgaria
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.


Top
 Profile  
 
PostPosted: Tue Sep 13, 2011 7:05 pm 
Offline
Forum Newbie

Joined: Wed Jan 19, 2011 11:04 pm
Posts: 6
WOw thats fantastic effort twinedev.

Cheers for that and yes ill be implementing that!

ps: what exactly is prepared statements?


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 4 posts ] 

All times are UTC - 5 hours


Who is online

Users browsing this forum: Majestic-12 [Bot] and 17 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Jump to:  
cron
Powered by phpBB® Forum Software © phpBB Group