Is this MODEL method safe?

Hi guys,

I am debating with someone on the security of the following method:
He insists it could allow someone to take control or delete the database.

$limit, $offset and $filters are passed from the controller and I've got them through $this->input->get('varname') method.

He says that I should escape the $filter variables before I pass them to the $this->db->where method.

Currently I have something like $sql .= " b.status = '" . $filters['product_status'] . "'";
He says that I should have do it like $sql .= " b.status = '" . $this->db->escape($filters['product_status']) . "'";

As far as I know where method is already escaping the strings, there is no need to do what he suggests.

Also before getting to the Model method the input -> get() method is doing xss filtering as I set up the variable in config to be true:
$config['global_xss_filtering'] = TRUE;

Plus the security filtering method is called automatically when a new controller is invoked.

So my question: Are there any security issues with building the queries this way?

PHP Code:

public function getOrdersFiltered($limit, $offset, $filters) {
        $order_sort = "DESC";

        if ($filters['order_sort'] == 2) {
            $order_sort = "DESC";
        } else if ($filters['order_sort'] == 1) {
            $order_sort = "ASC";
        }

        $order_by = 'a.date_added';

        if ($filters['order_by'] == '1') {
            $order_by = 'a.date_added';
        } else if ($filters['order_by'] == 2) {
            $order_by = "a.email";
        } else if ($filters['order_by'] == 3) {
            $order_by = 'CONCAT(a.first_name," ", a.last_name)';
        } else if ($filters['order_by'] == 4) {
            $order_by = "a.order_id";
        } else if ($filters['order_by'] == 5) {
            $order_by = "b.product_name";
        } else if ($filters['order_by'] == 6) {
            $order_by = "b.product_status";
        }

        $sql = "";

        if ($filters['product_status'] == "All") {
            $sql .= " b.status IS NOT NULL";
        } else {
            $sql .= " b.status = '" . $filters['product_status'] . "'";
        }

        if (!empty($filters['customer_name'])) {
            $sql .= " AND CONCAT(a.first_name,' ', a.last_name) LIKE '%" . $filters['customer_name'] . "%'";
        }

        if (!empty($filters['customer_email'])) {
            $sql .= " AND a.email LIKE '" . $filters['customer_email'] . "'";
        }

        if (!empty($filters['order_id'])) {
            $sql .= " AND a.order_id = '" . $filters['order_id'] . "'";
        }

        if (!empty($filters['order_date'])) {
            $sql .= " AND a.date_added = '" . $filters['order_date'] . "'";
        }

        if (!empty($filters['product_ordered'])) {
            $sql .= " AND b.product_name LIKE '%" . $filters['product_ordered'] . "%'";
        }

        if (!empty($filters['state_shipping_info'])) {
            $sql .= " AND a.delivery_address LIKE '%\"state_county\":\"" . $filters['state_shipping_info'] . "\"%'";
        }

        $this -> db -> limit($limit, $offset);
        $this -> db -> order_by($order_by, $order_sort);
        $this -> db -> where($sql);
        $this -> db -> group_by('a.order_id');
        $this -> db -> join("order_products b", "b.order_id=a.order_id", "LEFT");
        $this -> db -> select("*, a.status as order_status");
        $orders = $this -> db -> get("orders a") -> result_array();

        foreach ($orders as $key => $order) {
            $this -> db -> where("order_id", $order['order_id']);
            $orders[$key]['products'] = $this -> db -> get("order_products") -> result_array();

            foreach ($orders[$key]['products'] as $k => $p) {
                $product_options = json_decode($p['product_options'], TRUE);
                $orders[$key]['products'][$k]['customized'] = FALSE;
                foreach ($product_options as $po) {
                    if ($po['label'] == "Customization Instructions") {
                        $orders[$key]['products'][$k]['customized'] = TRUE;
                        break;
                    }
                }
            }
        }
        return $orders;
    } 


I once thought by using active methods then it is automatically safe with sql injection. It was totally wrong when my company hired a pen tester
and he was able to get through db with some of the hack script. However, I patch them up by simply adding $this->db->escape('your-input-values-from-post-data')
just before using it in the db query then you are ok

I once thought by using active methods then it is automatically safe with sql injection.  It was totally wrong when my company hired a pen tester
and he was able to get through db with some of the hack script.  However,  I patch them up by simply adding $this->db->escape('your-input-values-from-post-data')
just before using it in the db query then you are ok

If you take a look into system/database/DB_query_builder.php you will see that where method escape the query itself if we set the $escape variable to TRUE, so would be no need in escaping variables again and again. 

if ($escape === TRUE)
{
$v = ' '.$this->escape($v);
}

Wouldn't this be OK?

@George.adrian

Your code simply is ugly. It is not good enough even for being evaluated about security. Excuse me for being so direct.

@George.adrian

Your code simply is ugly. It is not good enough even for being evaluated about security. Excuse me for being so direct.

No problem. We are here to learn. Can you please come up with some arguments?

@George.adrian

1. As a start you could use the query builder to construct the WHERE and the ORDER clause. This is possible.
2. You join the table `order_products` in the first query, and then for every returned result item you are making a separated query within the cycle to the same table. Why is that.
3. $sql .= " AND a.email LIKE '" . $filters['customer_email'] . "'"; ----> Come on, don't you see this? And other similar things...

First make code beautiful, without smells, and then say "Guys, is it secure?". Before thinking about security you need code quality.

can we move on from beauty and personal experiences.
Are there any security issues with building the queries this way or not?

@George.adrian

1. As a start you could use the query builder to construct the WHERE and the ORDER clause. This is possible.
2. You join the table `order_products` in the first query, and then for every returned result item you are making a separated query within the cycle to the same table. Why is that.
3. $sql .= " AND a.email LIKE '" . $filters['customer_email'] . "'"; ----> Come on, don't you see this? And other similar things...

First make code beautiful, without smells, and then say "Guys, is it secure?". Before thinking about security you need code quality.

You are completely right about all this. Now regarding my question about security can you please give an answer on this code and on the scenarious I presented without being sarcastic. I would truly appreciate.

As far as I know where method is already escaping the strings, there is no need to do what he suggests.

Yes, you are right, in the docs you can see:

It also allows for safer queries, since the values are escaped automatically by the system.

https://www.codeigniter.com/user_guide/d...lder-class

This is true for every query builder function. However, you seem to be setting your sql in a variable to run like this:

PHP Code:

$this->db->query($sql); 


This is then NOT automatically escaped, so in that case, yes, you should escape the variables as your security advisor suggested.

Hope that helps,

Best wishes,

Paul

PS Well done for not rising to the comments about your actual coding.

No problem. We are here to learn. Can you please come up with some arguments?

Although I have to agree, it is somewhat 'clunky', but then again, if some of the better or more experienced programmers saw some of the code I churn out, they might justifiably have the same reaction. We all code what we can in the best way we know. The beauty of coding (IMHO) is there is always more to learn. Some people might say that if it works it is fine. I personally like beautiful code, but beauty is in the eye of the beholder of course. To me it is code that for the most part, makes sense when you see it, is easy to understand, almost plain, but laid out strictly and beautifully. However, I also love clever code and am a fan of code golf, the cleverness of some people in the formulation of code to do something in a particular way never ceases to amaze me.

About 2. I was not correct.

This is how I would try to implement this piece of code, it is untested, of course.

Code:

public function getOrdersFiltered($limit, $offset, $filters) {

        $this->db
            ->select("*, a.status AS order_status, CONCAT(a.first_name, ' ', a.last_name) AS customer_name") // The * at the begining might be replace with more precise set of needed fields.
            ->from("orders AS a")
            ->join("order_products AS b", "b.order_id = a.order_id", 'left')
        ;

        if (!isset($filters['product_status']) || $filters['product_status'] == 'All') {
            $this->where("b.status <>", null); // Or remove this line at all, maybe? This depends on data.
        } else {
            $this->where("b.status", $filters['product_status']);
        }

        if (isset($filters['customer_name']) && $filters['customer_name'] != '') {
            $this->db->like('customer_name', $filters['customer_name']);
        }

        if (isset($filters['customer_email']) && $filters['customer_email'] != '') {
            $this->db->like('a.email', $filters['customer_email']);
        }

        if (!empty($filters['order_id'])) {
            $this->db->where('a.order_id', (int) $filters['order_id']);
        }

        if (isset($filters['order_date']) && $filters['order_date'] != '') {
            $this->db->where('a.order_id', (int) $filters['order_id']);
        }

        if (isset($filters['product_ordered']) && $filters['product_ordered'] != '') {
            $this->db->like('b.product_name', $filters['product_ordered']);
        }

        if (isset($filters['state_shipping_info']) && $filters['state_shipping_info'] != '') {
            $this->db->like('a.delivery_address', '"state_county":"'.$filters['state_shipping_info'].'"'); // A guess.
        }

        $order_sort = 'desc';

        if (isset($filters['order_sort'])) {

            if ($filters['order_sort'] == 1) {
                $order_sort = 'asc';
            }
            // Remove this.
            //elseif ($filters['order_sort'] == 2) {
            //    $order_sort = 'desc';
            //}
        }

        $order_by = 'a.date_added';

        if (isset($filters['order_by'])) {

            switch ((int) $filters['order_by']) {

                case 1:
                    $order_by = 'a.date_added';
                    break;

                case 2:
                    $order_by = "a.email";
                    break;

                case 3:
                    $order_by = "customer_name";
                    break;

                case 4:
                    $order_by = "a.order_id";
                    break;

                case 5:
                    $order_by = "b.product_name";
                    break;

                case 6:
                    $order_by = "b.product_status";
                    break;
            }
        }

        $this->db
            ->order_by($order_by, $order_sort)
            ->limit($limit, $offset)
            ->group_by('a.order_id');

        //echo $this->db->get_compiled_select(); // Remove this, it was for quick testing.

        $orders = $this->db
            ->get()
            ->result_array();

        if (!empty($orders)) {

            foreach ($orders as $key => & $order) {

                $order['products'] = $this->db
                    ->where("order_id", $order['order_id'])
                    ->get("order_products")
                    ->result_array();

                if (!empty($order['products'])) {

                    foreach ($order['products'] as $k => $p) {

                        $product_options = json_decode($p['product_options'], TRUE);
                        $order['products'][$k]['customized'] = FALSE;

                        if (!empty($product_options)) { // An empty check, because it is not guaranteed an array result from json_decode().

                            foreach ($product_options as $po) {

                                if (isset($po['label']) && $po['label'] == "Customization Instructions") {

                                    $order['products'][$k]['customized'] = TRUE;
                                    break;
                                }
                            }
                        }
                    }
                }
            }

            unset($order); // Unsetting the reference at the moment it is not needed.
        }

        //return empty($orders) ? array() : $orders; // Check whether you need this, I don't remember whether NULL or array() is returned when there are not returned rows.
        return $orders;
    }