Active Record Vulnerability or Misunderstanding?

[eluser]maboa[/eluser]
Hi,

First time on this forum but been using codeigniter for about a year and love it.

I have noticed an issue when using activerecord and session data. I think the best way to explain it is with a very simple test case.

What happens is that if I pass a unset session->userdata value through to an activerecord query it seems to act as a wildcard.

I have a simple MySQL DB table set up - consist of two varchars id and password.

I am querying on id and pass through the id from the session. Which is fine when it is set but when it isn't set it acts as a wildcard.

Using the code below you would test by calling :

/activetest/clear
/activetest (you'll see all entries in the table)
/activetest/set/foo (sets the session variable to foo)
/activetest (only pulls out foo's record)


Test Controller :

Code:

<?php

class Activetest extends Controller
{
    function Activetest()
    {
        parent::Controller();        
        $this->load->model('activetest_model');    
    }
    
    function index()
    {
        $data['details'] = $this->activetest_model->getAccountDetails($this->session->userdata('logged_in_id'));            
        print_r($data['details']);    
    }

    function set()
    {
        $uid = $this->uri->segment(3);        
        $this->session->set_userdata('logged_in_id', $uid);
    }

    function clear()
    {
        $this->session->unset_userdata('logged_in_id');
        $this->session->sess_destroy();
    }
}

?>

Test Model :

Code:

<?php  
  
class activetest_model extends Model
{    
    function getAccountDetails($id)
    {    
        $this->db->where('id', $id);        
        $query = $this->db->get('testusers');
        return $query->result_array();
    }
}  
?>

Just wanted to know if this was expected behaviour.

Cheers

Mark

[eluser]WanWizard[/eluser]
If you pass an unset session variable, you're passing FALSE.

Since you can't pass a boolean to a query, it is converted to a string. The result is

Code:

SELECT * FROM (`testusers`) WHERE `id` = 0

If id is an integer field, it returns only that row (if it exists).

If id is a character field, the syntax is incorrect. "id = 0" evaluates to "is the string 'id' false'", which is true, so the query becomes "SELECT * FROM testusers WHERE TRUE", which returns all records.

[eluser]maboa[/eluser]
Thanks, that makes sense. I wasn't sure what the value was, I tried logging it but it wasn't appearing for some reason.

For some reason I thought that activerecord would protect against something like SELECT * FROM testusers WHERE TRUE being created as I read somewhere that it protects against SQL injection.

Thanks again.

Mark

[eluser]WanWizard[/eluser]
There's a difference between SQL injection protection (which is about properly escaping values) and passing the wrong value.

In general, always validate your data before using it (unless you're absolutely sure it has the value you expect).

[eluser]maboa[/eluser]
I still feel that it might avoid a lot of potential security issues if the activerecord implementation just picked this up and stopped the WHERE TRUE getting through to the query. After all when would you purposefully want to make a query with WHERE TRUE in it?

To be clear - I didn't expect my code to work if the session had expired, but I expected it to throw an error. You are entirely right to say that on the whole you should validate your data before using it, but sometimes through human error stuff slips through and I feel it would be better in these cases to fail than expose the contents of the database.

Mark

[eluser]WanWizard[/eluser]
Report it as an issue so it can be picked up: http://bitbucket.org/ellislab/codeignite...status=new.

[eluser]pickupman[/eluser]
[quote author="maboa" date="1291763732"]Thanks, that makes sense. I wasn't sure what the value was, I tried logging it but it wasn't appearing for some reason.

For some reason I thought that activerecord would protect against something like SELECT * FROM testusers WHERE TRUE being created as I read somewhere that it protects against SQL injection.

Thanks again.

Mark[/quote]

A helpful code snippet to use is:

Code:

$this->output->enable_profiler(TRUE);

This appends to the bottom of your page all sql queries, REQUEST array, and execution time. Very helpful to see what queries are being generated/duplicated.

[eluser]maboa[/eluser]
OK guys thanks for your help and advice. My first experience of this forum has been very positive I'll file a report and see what comes of it.

Cheers

Mark