Hello all. This is my first post - if it is in the wrong forum I deeply apologize and hopefully it can be moved. On to the message:
In looking through an OpenCart template from templatemonster.com, I noticed that there is a potential risk of SQL injection. According to a forum already on OpenCart SQL Injection (viewtopic.php?f=18&t=515), OpenCart has made sure that all security measures are taken care of. However, these measures don’t seem to be present in even the latest version 2.2.0.0. If I am not looking in the right place, folder, or file, please let me know, but for one, I do not see the “database::parse” method the forum above mentioned. The only measure against SQL injection I have seen so far is the use of “mysql_real_escape_string()”, but according to this site (https://johnroach.info/2011/02/17/why-m ... n-attacks/) and several others, this function is not enough. Instead, prepared statements should be used (among other practices which I will not mention, but prepared statements are the first and probably most important step to avoiding SQL injection). I do not see the use of prepared statements in OpenCart, and I strongly believe this is a serious security concern. Please share your opinions.
Hi Daniel,
Thank you for the prompt reply. I agree that it indeed only supports the main drivers, but both mysqli and PDO support prepared statements (see: https://phpdelusions.net/pdo ). This is where the problem in OpenCart lies - the function to use prepared statements is available, but it is not being taken advantage of (and becomes a huge security issue). OWASP rates SQL Injection the #1 security flaw, and not using prepared statements just invites the problem. It will require a little work to modify the query statements to work with prepared PDO or mysqli uses, but it's definitely worth it considering the alternative of far less security.
Thank you for the prompt reply. I agree that it indeed only supports the main drivers, but both mysqli and PDO support prepared statements (see: https://phpdelusions.net/pdo ). This is where the problem in OpenCart lies - the function to use prepared statements is available, but it is not being taken advantage of (and becomes a huge security issue). OWASP rates SQL Injection the #1 security flaw, and not using prepared statements just invites the problem. It will require a little work to modify the query statements to work with prepared PDO or mysqli uses, but it's definitely worth it considering the alternative of far less security.
Seeing as this crucial topic has apparently been forgotten for a reason unknown to me, I thought I'd share a link I found. For those developers who strive to make OpenCart safe, I hope you find this useful in doing so. For those others who use OpenCart and want to overcome its loopholes, I highly recommend you work on implementing this class as well. Hopefully in the future I or another, better developer will release a fully functional implementation of it. Here it is: https://github.com/colshrapnel/safemysq ... .class.php
6 years ago, I already published my own PDO library module as an Opencart extension which queries were done by simple WHERE clause but the way to handle the values over PDO were more focused compared to the demonstrated link above as no special methods were required as no alterations to the original queries were required. It was a very solid contribution for v1.5x releases of Opencart.Te3apo0t wrote:Seeing as this crucial topic has apparently been forgotten for a reason unknown to me, I thought I'd share a link I found. For those developers who strive to make OpenCart safe, I hope you find this useful in doing so. For those others who use OpenCart and want to overcome its loopholes, I highly recommend you work on implementing this class as well. Hopefully in the future I or another, better developer will release a fully functional implementation of it. Here it is: https://github.com/colshrapnel/safemysq ... .class.php
Dedication and passion goes to those who are able to push and merge a project.
Regards,
Straightlight
Programmer / Opencart Tester
I appreciate your efforts, and PDO is a good step. Unfortunately, using PDO and using prepared statements and using placeholders are 3 different things all together. Just using PDO won't stop SQL Injection, but using prepared statements with placeholders will (as the link I put demonstrates and elaborates on). Does your extension use prepared statements with placeholders in your PDO queries? Could I have a link if it does?
I couldn't agree more on this one. Previously, this year, I did posted a topic on the subject regarding bind vars that should rather be used.Te3apo0t wrote:I appreciate your efforts, and PDO is a good step. Unfortunately, using PDO and using prepared statements and using placeholders are 3 different things all together. Just using PDO won't stop SQL Injection, but using prepared statements with placeholders will (as the link I put demonstrates and elaborates on). Does your extension use prepared statements with placeholders in your PDO queries? Could I have a link if it does?
My PDO library for Opencart already have these functionalities integrated since all that time, though.Does your extension use prepared statements with placeholders in your PDO queries? Could I have a link if it does?
Dedication and passion goes to those who are able to push and merge a project.
Regards,
Straightlight
Programmer / Opencart Tester
Sure, if there's more demand to it, I'll post it since the v2.x release already has the PDO extension integrated into the core with unsecured preparations after I already publicized my own version of it on the Opencart extension store, unfortunately.
Dedication and passion goes to those who are able to push and merge a project.
Regards,
Straightlight
Programmer / Opencart Tester
What do you mean? I checked out v2.2 and I did not see a PDO library.straightlight wrote:the v2.x release already has the PDO extension integrated into the core with unsecured preparations
Is there any way I could check your code before potentially purchasing your PDO extension which does use prepared statements and placeholders?
The integrated PDO library is located in the system/library/db/mpdo.php file.Te3apo0t wrote:What do you mean? I checked out v2.2 and I did not see a PDO library.straightlight wrote:the v2.x release already has the PDO extension integrated into the core with unsecured preparations
I didn't realized that my PDO contribution would still be in demand after so long since the one being integrated in the core ...Is there any way I could check your code before potentially purchasing your PDO extension which does use prepared statements and placeholders?
Dedication and passion goes to those who are able to push and merge a project.
Regards,
Straightlight
Programmer / Opencart Tester
Ah, my bad. I must have had an incomplete download - a fresh one had the file. I notice that it does contain a function to make use of BindParam in PDO...but the basic queries don't seem to use it. Am I wrong?straightlight wrote:The integrated PDO library is located in the system/library/db/mpdo.php file.
Last month my store signs were hacked, Because of this, my friend for my website security scanning.
Because the first scan report I mentioned a problem with the installation of expansions, So I uninstall this extensions, I scanned once again, Also a warning appears.
2016/8/4
I hope this report, just to remind not really a problem.
Because the first scan report I mentioned a problem with the installation of expansions, So I uninstall this extensions, I scanned once again, Also a warning appears.
2016/8/4
2016/8/31SQL injection (verified)
Classification
Base Score: 6.8
- Access Vector: Network
- Access Complexity: Medium
- Authentication: None
- Confidentiality Impact: Partial
- Integrity Impact: Partial
- Availability Impact: Partial
CVSS
Base Score: 10
- Attack Vector: Network
- Attack Complexity: Low
- Privileges Required: None
- User Interaction: None
- Scope: Changed
- Confidentiality Impact: High
- Integrity Impact: High
- Availability Impact: None
CVSS3
CWE CWE-89
Affected items Variation
/index.php 4
SQL injection (verified)
Severity High
Type Validation
Reported by module Scripting (Sql_Injection.script)
/index.php
Details
URL encoded POST input data was set to 'and(select 1 from(select count(*),concat((select
concat(CHAR(52),CHAR(67),CHAR(117),CHAR(56),CHAR(85),CHAR(101),CHAR(110),CHAR(104),CHAR(73),CHAR(8
0),CHAR(69)) from information_schema.tables limit 0,1),floor(rand(0)*2))x from information_schema.tables group by
x)a)and'
Injected pattern found: 4Cu8UenhIPE
POST /index.php?route=product/product_oosn HTTP/1.1
Content-Length: 417
Content-Type: application/x-www-form-urlencoded
Cookie: PHPSESSID=eu6hri9odiem0udmrbp4h0pbr1; language=tw; currency=TWD;
visitor_id=6726a3e5300b4d6042b49d5de2f3aafb
Host: ***
Connection: Keep-alive
Accept-Encoding: gzip,deflate
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.21 (KHTML, like Gecko)
Chrome/41.0.2228.0 Safari/537.21
Accept: */*
Request headers
Acunetix Website Audit
all_selected_option=0&data='and(select%201%20from(select%20count(*)%2cconcat((select%20c
oncat(CHAR(52)%2cCHAR(67)%2cCHAR(117)%2cCHAR(56)%2cCHAR(85)%2cCHAR(101)%2cCHAR(110)%2cCH
AR(104)%2cCHAR(73)%2cCHAR(80)%2cCHAR(69))%20from%20information_schema.tables%20limit%200
%2c1)%2cfloor(rand(0)*2))x%20from%20information_schema.tables%20group%20by%20x)a)and'&na
me=e&phone=&product_id=&selected_option=0&selected_option_value=0
/index.php
Details
URL encoded POST input name was set to 'and(select 1 from(select count(*),concat((select
concat(CHAR(52),CHAR(67),CHAR(117),CHAR(101),CHAR(48),CHAR(79),CHAR(67),CHAR(81),CHAR(111),CHAR(10
1),CHAR(74)) from information_schema.tables limit 0,1),floor(rand(0)*2))x from information_schema.tables group by
x)a)and'
Injected pattern found: 4Cue0OCQoeJ
POST /index.php?route=product/product_oosn HTTP/1.1
Content-Length: 434
Content-Type: application/x-www-form-urlencoded
Cookie: PHPSESSID=eu6hri9odiem0udmrbp4h0pbr1; language=tw; currency=TWD;
visitor_id=6726a3e5300b4d6042b49d5de2f3aafb
Host: ***
Connection: Keep-alive
Accept-Encoding: gzip,deflate
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.21 (KHTML, like Gecko)
Chrome/41.0.2228.0 Safari/537.21
Accept: */*
all_selected_option=0&data=sample%40email.tst&name='and(select%201%20from(select%20count
(*)%2cconcat((select%20concat(CHAR(52)%2cCHAR(67)%2cCHAR(117)%2cCHAR(101)%2cCHAR(48)%2cC
HAR(79)%2cCHAR(67)%2cCHAR(81)%2cCHAR(111)%2cCHAR(101)%2cCHAR(74))%20from%20information_s
chema.tables%20limit%200%2c1)%2cfloor(rand(0)*2))x%20from%20information_schema.tables%20
group%20by%20x)a)and'&phone=&product_id=&selected_option=0&selected_option_value=0
Request headers
/index.php
Details
URL encoded POST input option%5B955%5D was set to 'and(select 1 from(select count(*),concat((select
concat(CHAR(52),CHAR(67),CHAR(117),CHAR(113),CHAR(119),CHAR(89),CHAR(104),CHAR(98),CHAR(77),CHAR(5
2),CHAR(105)) from information_schema.tables limit 0,1),floor(rand(0)*2))x from information_schema.tables group by
x)a)and'
Injected pattern found: 4CuqwYhbM4i
POST /index.php?route=checkout/cart/add HTTP/1.1
Content-Length: 366
Content-Type: application/x-www-form-urlencoded
Cookie: PHPSESSID=eu6hri9odiem0udmrbp4h0pbr1; language=tw; currency=TWD;
visitor_id=6726a3e5300b4d6042b49d5de2f3aafb
Host: ***
Connection: Keep-alive
Accept-Encoding: gzip,deflate
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.21 (KHTML, like Gecko)
Chrome/41.0.2228.0 Safari/537.21
Accept: */*
option%5B955%5D='and(select%201%20from(select%20count(*)%2cconcat((select%20concat(CHAR(
52)%2cCHAR(67)%2cCHAR(117)%2cCHAR(113)%2cCHAR(119)%2cCHAR(89)%2cCHAR(104)%2cCHAR(98)%2cC
HAR(77)%2cCHAR(52)%2cCHAR(105))%20from%20information_schema.tables%20limit%200%2c1)%2cfl
oor(rand(0)*2))x%20from%20information_schema.tables%20group%20by%20x)a)and'&product_id=1
158&quantity=e
Request headers
/index.php
Details
URL encoded POST input product_id was set to 'and(select 1 from(select count(*),concat((select
concat(CHAR(52),CHAR(67),CHAR(117),CHAR(103),CHAR(88),CHAR(119),CHAR(115),CHAR(51),CHAR(67),CHAR(9
0),CHAR(122)) from information_schema.tables limit 0,1),floor(rand(0)*2))x from information_schema.tables group by
x)a)and'
Injected pattern found: 4CugXws3CZz
Daniel mentioned that although not a security issue, I found that I was against the report provides a reference for everyone, Of course, there is no problem that is the best!SQL injection
Classification
Base Score: 6.8
- Access Vector: Network
- Access Complexity: Medium
- Authentication: None
- Confidentiality Impact: Partial
- Integrity Impact: Partial
- Availability Impact: Partial
CVSS
Base Score: 10
- Attack Vector: Network
- Attack Complexity: Low
- Privileges Required: None
- User Interaction: None
- Scope: Changed
- Confidentiality Impact: High
- Integrity Impact: High
- Availability Impact: None
CVSS3
CWE CWE-89
Affected items Variation
/index.php 1
SQL injection
Severity High
Type Validation
Reported by module Scripting (Sql_Injection.script)
Affected items
/index.php
Details
URL encoded GET input path was set to 1'"
Error message found: You have an error in your SQL syntax
GET /index.php?path=1'%22&product_id=3818&route=product/product HTTP/1.1
Referer: ***
Cookie: PHPSESSID=gu802ko6l2flu2bop1aho9i0q7; language=tw; currency=TWD;
visitor_id=6726a3e5300b4d6042b49d5de2f3aafb
Host: ***
Connection: Keep-alive
Accept-Encoding: gzip,deflate
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.21 (KHTML, like Gecko)
Chrome/41.0.2228.0 Safari/537.21
Accept: */*
I hope this report, just to remind not really a problem.
This URL where the SQL Injection occurred is not a standard part of OpenCart:
index.php?route=product/product_oosn
You should contact the author of that extension, and let them know their code is vulnerable to SQL injection. They need to get this fixed or they are adding security holes to OpenCart. Until it's fixed, you should uninstall that extension (whatever has the code "product_oosn").
index.php?route=product/product_oosn
You should contact the author of that extension, and let them know their code is vulnerable to SQL injection. They need to get this fixed or they are adding security holes to OpenCart. Until it's fixed, you should uninstall that extension (whatever has the code "product_oosn").
I had sent Daniel a private message in this regard recently, however as this is already in the public domain, I see no reason not to discuss things here.
It really needs doing, it probably should have been revised prior to the release of the first 3x major version which would have been the optimal time for such a heavy update to the queries but it wasn't.
Further more, to still be implemented in the same way in the development code is concerning. I would like to see this prioritised to be completed for 3.1 release, I know that may still be some time off, but i don't know that any 3.0.x release would be the appropriate place for such a large change to the project.
While I find that PDO has indeed been implemented, its doesn't seem to be utilised correctly.
Current git (25/08/2019):
This one line of code tells me that the method in use for 2.x is still in use today in the current dev branch, from this I now know that the implementation has not been looked at in any great depth.
The query executes as it should I am not disputing that, however PDO is not meant to be used in such a manner because it negates one of its key functions. So the problem as I see is it is that this current implementation of PDO is an obfuscation, whether intended or not.
From 3.0.2x though little has changed since 2.1:
The important thing to take note of here is the lack of parameter binding, there is a function for it within the class, but it appears to not be used. so this happens when you attempt to submit a prepared query:
The sql query is prepared, the parameters are applied for execution and so the exception is thrown because the preparation is only partially implemented with binding not taking place.
In summary:
Without wanting to come across as overly critical because that's not my intent, unsuspecting person might view PDO being implemented as some sort of indicator of secure coding. This isn't strictly true though, the method used at the moment can be gotten around and that's been well documented.
It really needs doing, it probably should have been revised prior to the release of the first 3x major version which would have been the optimal time for such a heavy update to the queries but it wasn't.
Further more, to still be implemented in the same way in the development code is concerning. I would like to see this prioritised to be completed for 3.1 release, I know that may still be some time off, but i don't know that any 3.0.x release would be the appropriate place for such a large change to the project.
While I find that PDO has indeed been implemented, its doesn't seem to be utilised correctly.
Current git (25/08/2019):
Code: Select all
$this->db->query("INSERT INTO `" . DB_PREFIX . "customer_activity` SET `customer_id` = '" . (int)$customer_id . "', `key` = '" . $this->db->escape($key) . "', `data` = '" . $this->db->escape(json_encode($data)) . "', `ip` = '" . $this->db->escape($this->request->server['REMOTE_ADDR']) . "', `date_added` = NOW()");The query executes as it should I am not disputing that, however PDO is not meant to be used in such a manner because it negates one of its key functions. So the problem as I see is it is that this current implementation of PDO is an obfuscation, whether intended or not.
From 3.0.2x though little has changed since 2.1:
Code: Select all
public function query($sql, $params = array()) {
$this->statement = $this->connection->prepare($sql);
$result = false;
try {
if ($this->statement && $this->statement->execute($params)) {
$data = array();
while ($row = $this->statement->fetch(\PDO::FETCH_ASSOC)) {
$data[] = $row;
}
$result = new \stdClass();
$result->row = (isset($data[0]) ? $data[0] : array());
$result->rows = $data;
$result->num_rows = $this->statement->rowCount();
}
} catch (\PDOException $e) {
throw new \Exception('Error: ' . $e->getMessage() . ' Error Code : ' . $e->getCode() . ' <br />' . $sql);
}
if ($result) {
return $result;
} else {
$result = new \stdClass();
$result->row = array();
$result->rows = array();
$result->num_rows = 0;
return $result;
}
}
Code: Select all
<b>Warning</b>: PDOStatement::execute(): SQLSTATE[HY093]: Invalid parameter number: no parameters were bound in <b>/pathdeleted/system/library/db/mpdo.php</b> on line <b>58</b>
In summary:
Without wanting to come across as overly critical because that's not my intent, unsuspecting person might view PDO being implemented as some sort of indicator of secure coding. This isn't strictly true though, the method used at the moment can be gotten around and that's been well documented.
Last edited by Chris_UK on Sun Aug 25, 2019 10:53 pm, edited 1 time in total.
Who is online
Users browsing this forum: No registered users and 320 guests
