Check Code

[marcopolo]

I'm getting the following error in the debug logs:

[19-Jan-2016 10:59:49 America/New_York] PHP Fatal error:  1064:You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ''A=0                 
             ORDER BY products_name asc' at line 8 :: SELECT * 
               FROM products p, 
                    products_to_categories t1, 
                    products_description t2  
              WHERE p.products_id = t1.products_id
                AND t1.products_id = t2.products_id
                AND p.products_status = 1
                AND t1.categories_id = 22'A=0                 
             ORDER BY products_name asc ==> (as called by) /var/www/zencart/includes/modules/pages/quick_order/header_php.php on line 30 <== in /var/www/zencart/includes/classes/db/mysql/query_factory.php on line 155 

Here is the code from the header file it is referencing:

  if (isset($_REQUEST['catid'])){
     $_SESSION['navigation']->add_current_page();
       $catid = zen_sanitize_string($_REQUEST['catid']);
     $sql = "SELECT * 
               FROM ".TABLE_PRODUCTS." p, ".TABLE_PRODUCTS_TO_CATEGORIES." t1, ".TABLE_PRODUCTS_DESCRIPTION." t2  
              WHERE p.products_id = t1.products_id
                AND t1.products_id = t2.products_id
                AND p.products_status = 1
                AND t1.categories_id = ".$catid . "                 
             ORDER BY products_name asc";
     $rs = $db->Execute($sql);
     $i=0; 

Can anyone see anything wrong with the code above?

[DrByte]

1. There is no "quick_order" file like that built-in to Zen Cart. So that means this code is from some plugin you've added.

2. It's using the $_REQUEST superglobal, instead of specifying $_GET or $_POST. In that case it needs to be more aggressive about analyzing the input data.

3. Since it's expecting an integer value for the lookup, it shouldn't be using only zen_sanitize_string(). It should just be casting it to (int), or at least using zen_

4. Thus, the problem you're seeing is a result of someone attempting to hack your site by probing for problems using malformed URLs to seek for MySQL blind SQL Injection vulnerabilities. They're trying to close the quotes of your query and add malicious parameters by passing quotes and extra query data in the URL.


So, I'd stop using that plugin, or at the very least clean up the query data by casting to integer:

       $catid = (int)$_REQUEST['catid'];

or by specifically protecting the query by escaping it according to the database's own internal sanitization rules:

       $catid = $db->prepareInput($_REQUEST['catid']);

Or even better, use both.

[marcopolo]

Thanks DrByte,

I changed the code as you suggested, as well as a another small change:

  if (isset($_REQUEST['catid'])){
     $_SESSION['navigation']->add_current_page();
     $catid = $db->prepareInput($_REQUEST['catid']);
     $sql = "SELECT * 
               FROM ".TABLE_PRODUCTS." p, 
                    ".TABLE_PRODUCTS_TO_CATEGORIES." t1, 
                    ".TABLE_PRODUCTS_DESCRIPTION." t2  
              WHERE p.products_id = t1.products_id
                AND t1.products_id = t2.products_id
                AND p.products_status = 1
                AND t1.categories_id = ". (int)$catid . "                
             ORDER BY products_name asc";
     $rs = $db->Execute($sql);
     $i=0;

[DrByte]

Thumbs up for using both. ;)