Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Input not sanitized #4114

Closed
dsopas opened this issue Mar 14, 2016 · 3 comments
Closed

Input not sanitized #4114

dsopas opened this issue Mar 14, 2016 · 3 comments

Comments

@dsopas
Copy link

dsopas commented Mar 14, 2016

Hi,

I think I found a variable that's not sanitized and could lead to a SQL Injection or information disclosure.

The source is openbay.php:
$carrier = $this->request->post['carrier_other'][$order_id];

As I checked under OpenCart documention Request class uses htmlspecialchars() and encodes double quotes
so it's still possible to use single quote right? Most of your database queries use escape() to prevent that from
happening. I found one that don't use escape before going to a SQL query.

So we have this line:
$carrier = $this->request->post['carrier_other'][$order_id];
(...)
$this->model_openbay_amazon->updateAmazonOrderTracking($order_id, $carrier, $carrier_from_list, !empty($carrier) ? $this->request->post['tracking'][$order_id] : '');

After this it calls amazon.php file:
public function updateAmazonOrderTracking($order_id, $courier_id, $courier_from_list, $tracking_no) {
$this->db->query("
UPDATE " . DB_PREFIX . "amazon_order
SET courier_id = '" . $courier_id . "',
courier_other = " . (int)!$courier_from_list . ",
tracking_no = '" . $tracking_no . "'
WHERE order_id = " . (int)$order_id . "");
}

Because the single quote is not escaped and Query class uses single quote to delimiter possible SQL Injection
in place.

A possible solution is to use escape() function on courier_id.

I hope I'm not mistaken and feel free to give me your feedback.

Best,

@danielkerr
Copy link
Member

because who ever write this part of the code was begin lazy. not sure there is a hack here though as its protected because of the admin login.

@danielkerr
Copy link
Member

does need fixing though

@dsopas
Copy link
Author

dsopas commented Mar 14, 2016

Hi Daniel,

Thanks for the fast reply. Still a security issue in my opinion.
Keep up the good work.

Best

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants