CODEWITHSUNDEEP
phpsqlcodeigniterescaping
Jonno_FTW
Jonno_FTW

Reputation: 8809

Codeigniter: update_batch not escaping field names

I am trying to use the update_batch() function from the Active Record helper in Codeigniter as follows: 

var_dump($data);
$this->db->where('event_id',$data['event_id']);
$this->db->update_batch('results',$data['results'],'uid');
echo $this->db->last_query();

This prints out the following:

array(2) {
  ["event_id"]=>
  string(2) "11"
  ["results"]=>
  array(2) {
    [0]=>
    array(2) {
      ["uid"]=>
      string(36) "1beab26d-f705-11e0-a190-f46d048dfd25"
      ["res"]=>
      string(1) "2"
    }
    [1]=>
    array(2) {
      ["uid"]=>
      string(36) "9dcc9e0a-bf24-11e0-838c-f46d048dfd25"
      ["res"]=>
      string(1) "1"
    }
  }
}
UPDATE results SET res = CASE 
WHEN uid = '1beab26d-f705-11e0-a190-f46d048dfd25' THEN '2'
WHEN uid = '9dcc9e0a-bf24-11e0-838c-f46d048dfd25' THEN '1'
ELSE res END WHERE `event_id` = '11' AND uid IN ('1beab26d-f705-11e0-a190-f46d048dfd25','9dcc9e0a-bf24-11e0-838c-f46d048dfd25')

Obviously, this is a large security hole because the field and table names are not being escaped from the update_batch section (although they are escaped in the where section). Am I doing something wrong? This behaviour is not specified in the docs:

http://codeigniter.com/user_guide/database/active_record.html#update

Upvotes: 0

Views: 1387

Answers (1)

Damien Pirsy
Damien Pirsy

Reputation: 25445

Uhm, according to the documentation for the update_batch() function, they just claim that

"values are automatically escaped producing safer queries"

with these words, I see implied that fields and table names are not protected.

But in the "queries" page, they kind of contradict by saying:

In many databases it is advisable to protect table and field names - for example with backticks in MySQL. Active Record queries are automatically protected, however if you need to manually protect an identifier you can use:

$this->db->protect_identifiers('table_name');

And IIRC it's true, AR always wraps names in backticks (that's why you need to pass FALSE, usually, to the active record method if you don't want escaping to mess with your query part).

EDIT:

Uhm, I just went rapidly over the code, and it looks like it is escaped:

// Batch this baby
for ($i = 0, $total = count($this->ar_set); $i < $total; $i = $i + 100)
{
   $sql = $this->_update_batch($this->_protect_identifiers($table, TRUE, NULL, FALSE), array_slice($this->ar_set, $i, 100), $this->_protect_identifiers($index), $this->ar_where);

   $this->query($sql);
}

So, migth really be a bug; So far, if your table names aren't dynamically generated, you can simply ignore this and be safe anyway; or you could run the protect_identifiers() method and do that yourself (wrong in principle, but hey...).

You can try and reproduce the bug more than once, and then file a bug report

Upvotes: 1

Related Questions