bcit-ci / CodeIgniter

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
18.27k stars 7.6k forks source link

database functions get escaped as if they were strings #1895

Closed entomb closed 12 years ago

entomb commented 12 years ago

When inserting data into the database, mysql functions get escaped as strings. during insert or update when i want to use "NOW()" as a value.

example:

$this->db->insert('table',array(
                'id' => 1,
                'name' => "this is a name",
                'date' => "NOW()",
            )
        );

when using this statement the "NOW()" get escaped and the date ends up being null. this is also true for any other statement. Another example is the use of DATE_ADD or DATE_SUB on WHERE statements.

$this->db->delete('table',array(
                'date' => "DATE_SUB(now(), interval 1 day)"
            )
        );

this should work, but instead my statement gets escaped. I suggest a way of forcing a non-escaped value or a white-list dictionary of allowed functions to skip the regular string escape functions

GDmac commented 12 years ago

Which version of CI are you using? Some DB methods accept an extra parameter to not try and protect identifiers.

This works for me for 'where':

// see user_guide third param on where
$this->db->select('session_id');
$this->db->where('`last_activity` < ', 'UNIX_TIMESTAMP(NOW())', FALSE);
$query = $this->db->get('ci_sessions');
var_dump($query->result());

db->insert() doesn't allow escaping directly (see manual). For INSERT use the db->set() method.

// false = not escaped
$this->db->set('last_activity', "UNIX_TIMESTAMP(NOW())", FALSE);
// others in associative array
$this->db->set(
  array(
    'session_id' => 10,
    'user_agent' => "blah",
  )
);
$result = $this->db->insert('ci_sessions');
var_dump($result);

"if all else fails, read the manual"

entomb commented 12 years ago

What happens if I don't want to use active record, or if i'm using insert_batch()?

GDmac commented 12 years ago

I don't know what happens if you don't want to use active record.

Have you looked at or tried regular queries? http://codeigniter.com/user_guide/database/queries.html

insert_batch again 'is' an active record method. a quick google turned up "set $this->db->_protect_identifiers to FALSE before your query? add any backticks yourself?"

entomb commented 12 years ago

The problem is that i want to turn of the protection only in one of the array values, the rest should be treated normaly.

Ive added a simple solution for myself on the CI_DB_driver->escape() method so it works for insert() update() and delete() as well as the where parameter on those methods.

so I can do this:

$this->db->insert('table',array(
                'id' => 1,
                'name' => "this is a name",
                'date' => array("NOW()"),
            )
        );

and know the NOW() function will be passed correctly as a function and not as a string "NOW()". its not an elegant solution, thats why I submitted this issue, in case someone else has a better work-around.

GDmac commented 12 years ago

As a work-around you could also, for the batch, calculate the NOW() and DATE_SUB() with php. or even get the mysql values beforehand with a simple query if you have elaborate date functions.

SELECT 
  NOW() AS mysql_now, 
  DATE_SUB(now(), interval 1 day) AS mysql_yesterday
entomb commented 12 years ago

Stressing the database with queries is not a very good solution, as well as the possible date missmatch on slow batches. With the escape() "hack" i'm fixing all problems in all methods, so i'm pretty happy with it for now,

Thanks for the feedback, It has been helpfull

GDmac commented 12 years ago

On slow +1 second queries the NOW() might that not actually jump a second during insert / update, no? Also, if i recall, form input can also be made an array fairly easy, by just adding two square brackets[] to the name of an input, you don't want those unescaped.

entomb commented 12 years ago

Yes, this is a security issue for anyone who doesn't verify and sanitize form data properly.

Got any Ideas on how to do this without a huge security hole?

GDmac commented 12 years ago

maybe someone more knowledgable than me (@narfbg , @dchill42 , @pkriete ) can look into this. Wanting to use native MySQL functions is not uncommon.

narfbg commented 12 years ago

Added an optional escape parameter to insert() and insert_batch(), as you can see in the above commit. Additionally, this was still possible with the use of set() (which already had optional escaping):

$this->db->set('id', 1);
$this->db->set('name', "this is a name");
$this->db->set('date', 'NOW()', FALSE); // note the third argument
$this->db->insert('table');
GDmac commented 12 years ago

That makes it an all or nothing option then. remember to escape() any fields from outside sources

entomb commented 12 years ago

Thanks for this update, it behaves perfectly now.

dev-david commented 11 years ago

I am having problems using now() for an insert_batch.

$insert_data = array();

    foreach($form_post_data as $guest_id)
    { 
        $insert_data[]= array(
        'owner' => $this->session->userdata['id'],
        'guest_id' => $guest_id,
        'event_name' => $this->session->userdata['event_name'],
        );
    }

    $this->db->set('created_at', 'NOW()', FALSE);
    $insert_user_query = $this->db->insert_batch('my_table', $insert_data);

I have used the NOW(), FALSE method before and it works fine for single inserts but I am new to code igniter and the insert_batch option. Any ideas?

entomb commented 11 years ago

Hello David,

I solved this "tinny" issue of mine with the help of PHP, this might help you too:

 $insert_data[]= array(
        'owner' => $this->session->userdata['id'],
        'guest_id' => $guest_id,
        'event_name' => $this->session->userdata['event_name'],
        'created_at' => Date("Y-m-d h:i:s"),
    );

Using Date("Y-m-d h:i:s") will set a date that mysql can read as a datetime type of string

All mysql date function can more or less be emulated this way with the PHP date functions.

note depending on your configs, the string might need to be different, this is the default

dev-david commented 11 years ago

Yes this was one of the first things I considered doing but really wanted to see if there was a CI way of avoiding this without going into system files etc etc.

Ended up using the Date() method in the end like you did but trying to see if there is a better way.

entomb commented 11 years ago

there's a third parameter, as referenced in this issue that turns on and off the auto escaping. I wouldn't recommend if you have user inserted data going trough your query unless its a controlled query and the user is you (eg: a cron job, batch script, etc).

The Date() method is what I ended up using and I'm still happy with it.