0

I'm using SensioLabsInsight to profile any vulnerabilities in my code.

I've received several errors for possible sql injection, and it recommends using parameter binding with PDO. This is fine since I'm already using PDO for my db driver.

Right now my model is passed a $data array and then checks for specific values in the array in order to add to the sql query if present, like so:

public function getDownloads($data = array()) {
    $sql = "
        SELECT * 
        FROM {$this->db->prefix}download d 
        LEFT JOIN {$this->db->prefix}download_description dd 
            ON (d.download_id = dd.download_id) 
        WHERE dd.language_id = '" . (int)$this->config->get('config_language_id') . "'";

    if (!empty($data['filter_name'])) {
        $sql .= " AND dd.name LIKE '" . $this->db->escape($data['filter_name']) . "%'";
    }

    $sort_data = array(
        'dd.name',
        'd.remaining'
    );

    if (isset($data['sort']) && in_array($data['sort'], $sort_data)) {
        $sql .= " ORDER BY " . $data['sort'];   
    } else {
        $sql .= " ORDER BY dd.name";    
    }

    if (isset($data['order']) && ($data['order'] == 'DESC')) {
        $sql .= " DESC";
    } else {
        $sql .= " ASC";
    }

    if (isset($data['start']) || isset($data['limit'])) {
        if ($data['start'] < 0) {
            $data['start'] = 0;
        }           

        if ($data['limit'] < 1) {
            $data['limit'] = 20;
        }   

        $sql .= " LIMIT " . (int)$data['start'] . "," . (int)$data['limit'];
    }

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

    return $query->rows;
}

The error referenced from the SensioLabsInsight analysis references only the $data['sort'] clause as being a possible injection point.

My question is, do I need to test for $data array presence when creating a prepare statement, or will it simply return null if the array value is empty.

My proposed new query with parameter binding would look like so:

public function getDownloads($data = array()) {
    $sql = "
        SELECT * 
        FROM {$this->db->prefix}download d 
        LEFT JOIN {$this->db->prefix}download_description dd 
            ON (d.download_id = dd.download_id) 
        WHERE dd.language_id = '" . (int)$this->config->get('config_language_id') . "'";

    if (!empty($data['filter_name'])) {
        $sql .= " AND dd.name LIKE :filter_name%";
    }

    $sort_data = array(
        'dd.name',
        'd.remaining'
    );

    if (isset($data['sort']) && in_array($data['sort'], $sort_data)) {
        $sql .= " ORDER BY :sort";  
    } else {
        $sql .= " ORDER BY dd.name";    
    }

    if (isset($data['order']) && ($data['order'] == 'DESC')) {
        $sql .= " DESC";
    } else {
        $sql .= " ASC";
    }

    if (isset($data['start']) || isset($data['limit'])) {
        if ($data['start'] < 0) {
            $data['start'] = 0;
        }           

        if ($data['limit'] < 1) {
            $data['limit'] = 20;
        }   

        $sql .= " LIMIT :start, :limit";
    }

    $this->db->prepare($sql);
    $this->db->bindParam(':filter_name', $data['filter_name']);
    $this->db->bindParam(':sort', $data['sort']);
    $this->db->bindParam(':start', $data['start'], PDO::PARAM_INT);
    $this->db->bindParam(':limit', $data['limit'], PDO::PARAM_INT);

    $query = $this->db->execute();

    return $query->rows;
}

Will this work as is, or do the parameter bindings need to be moved within the if/else conditionals?

secondman
  • 3,233
  • 6
  • 43
  • 66
  • You can't use parameters for the `LIMIT` clause. Parameters are only allowed where expressions can be put in the SQL, and `LIMIT` arguments have to be literals, not expressions. – Barmar Dec 27 '14 at 08:02
  • Ok, well as I said in my question, START and LIMIT aren't the issue, the error was found in the sort portion. This query is implemented via a GET request, so I simply attempted to bind anything that could be manipulated via a user. If STARt and LIMIT don't apply then great. But the question is, do I need to test for the existence of the array value or will PDO ignore it if it's empty? – secondman Dec 27 '14 at 08:22
  • If an array element is not defined, PHP will treat it as `null` when you use it. And if you have error reporting enabled, it will report a notice about an undefined index. – Barmar Dec 27 '14 at 08:25
  • So yes is the answer? I need to move the bindings into the if/else conditionals? – secondman Dec 27 '14 at 09:12

0 Answers0