Post by Te3apo0t » Wed May 11, 2016 11:07 am

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.

Newbie

Posts

Joined
Wed May 11, 2016 10:58 am

Post by Daniel » Fri May 13, 2016 12:05 am

2.x does not even work with the function mentioned. it uses the mysqi or pdo msql.

OpenCart®
Project Owner & Developer.


User avatar
Administrator

Posts

Joined
Fri Nov 03, 2006 6:57 pm

Post by Te3apo0t » Sat May 14, 2016 6:39 am

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.

Newbie

Posts

Joined
Wed May 11, 2016 10:58 am

Post by Te3apo0t » Thu Jul 07, 2016 4:46 am

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

Newbie

Posts

Joined
Wed May 11, 2016 10:58 am

Post by straightlight » Thu Jul 07, 2016 5:23 am

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
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.

The most generated errors being found on Opencart forum originates from contributed programming. The increased post counters are caused by redundancies of the same solutions that were already provided prior.


Regards,
Straightlight
Opencart.com Administrator / Quality Assurance Analyst / Programmer


Legendary Member

Posts

Joined
Mon Nov 14, 2011 11:38 pm
Location - Canada, ON

Post by Te3apo0t » Sun Jul 10, 2016 3:22 am

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?

Newbie

Posts

Joined
Wed May 11, 2016 10:58 am

Post by straightlight » Sun Jul 10, 2016 3:51 am

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?
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.
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.

The most generated errors being found on Opencart forum originates from contributed programming. The increased post counters are caused by redundancies of the same solutions that were already provided prior.


Regards,
Straightlight
Opencart.com Administrator / Quality Assurance Analyst / Programmer


Legendary Member

Posts

Joined
Mon Nov 14, 2011 11:38 pm
Location - Canada, ON

Post by Te3apo0t » Sun Jul 10, 2016 11:06 pm

straightlight wrote:My PDO library for Opencart already have these functionalities integrated since all that time, though.
Could I have a link to your PDO library so I could examine it? This would be great news.

Newbie

Posts

Joined
Wed May 11, 2016 10:58 am

Post by straightlight » Sun Jul 10, 2016 11:19 pm

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.

The most generated errors being found on Opencart forum originates from contributed programming. The increased post counters are caused by redundancies of the same solutions that were already provided prior.


Regards,
Straightlight
Opencart.com Administrator / Quality Assurance Analyst / Programmer


Legendary Member

Posts

Joined
Mon Nov 14, 2011 11:38 pm
Location - Canada, ON

Post by Te3apo0t » Mon Jul 11, 2016 2:18 am

straightlight wrote:the v2.x release already has the PDO extension integrated into the core with unsecured preparations
What do you mean? I checked out v2.2 and I did not see a PDO library.

Is there any way I could check your code before potentially purchasing your PDO extension which does use prepared statements and placeholders?

Newbie

Posts

Joined
Wed May 11, 2016 10:58 am

Post by straightlight » Mon Jul 11, 2016 2:45 am

Te3apo0t wrote:
straightlight wrote:the v2.x release already has the PDO extension integrated into the core with unsecured preparations
What do you mean? I checked out v2.2 and I did not see a PDO library.
The integrated PDO library is located in the system/library/db/mpdo.php file.
Is there any way I could check your code before potentially purchasing your PDO extension which does use prepared statements and placeholders?
I didn't realized that my PDO contribution would still be in demand after so long since the one being integrated in the core ...

The most generated errors being found on Opencart forum originates from contributed programming. The increased post counters are caused by redundancies of the same solutions that were already provided prior.


Regards,
Straightlight
Opencart.com Administrator / Quality Assurance Analyst / Programmer


Legendary Member

Posts

Joined
Mon Nov 14, 2011 11:38 pm
Location - Canada, ON

Post by Te3apo0t » Mon Jul 11, 2016 5:24 pm

straightlight wrote:The integrated PDO library is located in the system/library/db/mpdo.php file.
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?

Newbie

Posts

Joined
Wed May 11, 2016 10:58 am

Post by HAO » Wed Aug 31, 2016 2:27 pm

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
SQL 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
2016/8/31
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: */*
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!

I hope this report, just to remind not really a problem.

HAO
Active Member

Posts

Joined
Fri Jun 03, 2011 2:52 pm

Post by ppiper » Wed Nov 30, 2016 6:41 pm

Hi @HAO, Which tool did you use for scanning?
Thanks

Newbie

Posts

Joined
Wed Sep 14, 2016 12:44 am

Post by Johnathan » Wed Nov 30, 2016 11:04 pm

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").

Image
Image Image Image Image


User avatar
Global Moderator

Posts

Joined
Fri Dec 18, 2009 3:08 am


Post by Chris_UK » Sun Aug 25, 2019 9:27 pm

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):

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()");
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:

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;
		}
	}
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:

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>
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.
Last edited by Chris_UK on Sun Aug 25, 2019 10:53 pm, edited 1 time in total.

New member

Posts

Joined
Wed Jan 20, 2016 4:39 am
Who is online

Users browsing this forum: No registered users and 20 guests