phpopencartsql-injectionopencart-moduleopencart2.3

PHP SQL Injection Possibility

I've been looking into opencart, which is written in PHP.

If you take a look at the following php file,

https://github.com/opencart/opencart/blob/master/upload/admin/model/customer/customer.php

The SQL statement looks like this

$this->db->query("INSERT INTO " . DB_PREFIX . "customer 
SET customer_group_id = '" . (int)$data['customer_group_id'] . "', 
firstname = '" . $this->db->escape($data['firstname']) . "', 
lastname = '" . $this->db->escape($data['lastname']) . "', 
email = '" . $this->db->escape($data['email']) . "', 
telephone = '" . $this->db->escape($data['telephone']) . "', 
custom_field = '" . $this->db->escape(isset($data['custom_field']) ? json_encode($data['custom_field']) : json_encode(array())) . "', 
newsletter = '" . (int)$data['newsletter'] . "', 
salt = '', 
password = '" . $this->db->escape(password_hash($data['password'], PASSWORD_DEFAULT)) . "', 
status = '" . (int)$data['status'] . "', 
safe = '" . (int)$data['safe'] . "', 
date_added = NOW()");

The recommended way to avoid PHP sql injection is to use prepared statements.

My question is considering how this particular code isn't using the prepared statements, is this code vulnerable to sql injection?

I'm not a a php expert so I might be missing something obvious here.

EDIT:

Let me list the reasons I'm a bit apprehensive to accept that this code is vulnerable.

  1. OpenCart (https://github.com/opencart/opencart) is a popular open source project with over 200 forks.

  2. It's specifically an shopping cart (e-commerce) solution, so the developer would've put some thought into security, and sql injections like this is one of the first things they would've checked.

  3. It does look like some kind of escaping is done using $this->db->escape($data['telephone'])

Solution

  • As everything is either escaped or converted to an integer I would say it is not vulnerable to SQL injection.

    Of course Qirel is right in the comments that using prepared statements is a better solution in all imaginable ways. It is much harder to read and it can be easily made vulnerable by mistake when modifying the code in the future.

    EDIT: After a bit of research it seem this is true only under the assumption that the database character set has been set correctly. Otherwise it might still be vulnerable to to multi-byte attacks.

    OpenCart seems to be vulnerable when using MySQL if improper character set is set at the server level as it uses SET CHARACTER SET query instead of mysql_real_escape_string. You can see it in latest v3.0.2.0 on Github. For details see MySQL Character sets in PHP Manual.

    I suggested a fix of this particular issue in !5812.