0

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'])

chris85
  • 23,846
  • 7
  • 34
  • 51
Thihara
  • 7,031
  • 2
  • 29
  • 56
  • 5
    Yes this code is vulnerable, as it is not using [Prepared Statements](https://www.w3schools.com/php/php_mysql_prepared_statements.asp), **Escaping strings is NO LONGER the recommended solution to SQL Injection, and it is NOT enough** – GrumpyCrouton Jul 28 '17 at 16:24
  • 3
    Possible duplicate of [How can I prevent SQL injection in PHP?](https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php) – Qirel Jul 28 '17 at 16:25
  • The only, definitive way to protect against SQL injection is using prepared statements. This is also better in many other aspects - avoid escaping, just use prepared statements. – Qirel Jul 28 '17 at 16:27
  • This is not a duplicate, please read the edits. I'm aware of the recommended way to avoid SQL Injection. But this is not about that. – Thihara Jul 28 '17 at 16:33
  • `$this->db->escape(isset($data['custom_field']) ? json_encode($data['custom_field']) : json_encode(array()))` is horrid to read. See https://security.stackexchange.com/questions/3611/sql-injection-why-isnt-escape-quotes-safe-anymore for your specific question. `Always using prepared statements with parameters is something that can be validated by static code analysis tools. A missing call to xxx_escape_string is not spotted that easily and reliably.` If a future developer changes this to `safe = '(int)" . $data['safe'] . "` you are open. – chris85 Jul 28 '17 at 16:35

2 Answers2

1

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.

Petr Heinz
  • 71
  • 4
0

You can foresee some types of sql injection with htmlspecialchars but not all. Generally the correct one is to use the own resources of the database objects of data of PHP or objects of CRM / Frameworks.

See the following links: http://php.net/manual/pt_BR/pdo.prepare.php https://www.doctrine-project.org/projects/doctrine-dbal/en/2.7/reference/data-retrieval-and-manipulation.html https://framework.zend.com/manual/2.4/en/modules/zend.db.sql.html

Marcel Bezerra
  • 161
  • 1
  • 9