Isn't this a security issue ?

As everyone can see the data is not sanitized directly by mysql_real_escape_string

  $sql = "select count(*) as total
          from " . TABLE_PRODUCTS . " p, " .
                   TABLE_PRODUCTS_DESCRIPTION . " pd
          where    p.products_status = '1'
          and      p.products_id = '" . (int)$_GET['products_id']. "'
          and      pd.products_id = p.products_id
          and      pd.language_id = '" . (int)$_SESSION['languages_id'] . "'";

I'd like to know if the wrapper takes care of it and that's why (after all to do it properly you need to have an active db connection when you use the mysql_real_escape_string function...)
Or if it's just a missed security issue.

of course since the data is "sanitized" by the (int) it's not an imminent danger, But...

I modified files to make the products pull up by Model Number, with
&products_model=MODEL

And since it's open type I'd like to know if this is going to cause security holes...

	// PRODUCT MODEL SELECTOR
	if(isset($_GET['products_model'])) {
		$sql = "select products_id
	          from " . TABLE_PRODUCTS . "
	          where    products_model= '" . mysql_real_escape_string($_GET['products_model']) . "'";     
	          
	  $res = $db->Execute($sql);  
	  $ProductRealId = $res->fields[products_id];
	} else {
		$ProductRealId = $_GET['products_id'];
	} 

I then replace every $_GET['products_id'] with $ProductRealId...

Notice I manually sanitized the string even if I'm not 100% sure that the connection is avail and the sanitation occurs...

I'm just wondering and wanting to know if that's just an exploit waiting to happen in my peculiar situation...

I think I will pack this up as a mod and give it to you guys if you don't think I'm opening possible exploits with it.

As far as functionality it's working like a charm :) pretty good for a 20 min work around :P

the files that were modified are:

includes/modules/pages/product_info/
header_php.php
main_template_vars.php

Template Files

tpl_product_info_display.php

includes/modules/

also_purchased_products.php
product_prev_next.php

Thanks for any info about what I think might be an issue, or any comment about this.

There's no need to package up an addon.

In the case of the core query you quoted, the (int) takes care of the required sanitization.

For writing your own queries esp when sanitizing strings, using bindVars takes care of all the sanitization requirements. You'll notice that new queries added since v1.3.5 have been largely using the newer format. And in cases where blatant risks were apparent, others have been updated in subsequent versions.

To smooth the way forward, all the built-in SQL queries have been converted to using bindVars in v2.0, along with a lot of optimization as well.

Ok, so there is an addon that allows someone to pull up the items by model number rather then item ID ?
Beats me; I wasted 20 minutes if there is one :P

If that's not the case then you don't think I should package it up and release it as an addon to simplify the lives of other zen-cart users?

I was replying based on:
a) your thread title is "Isn't this a security issue?"
b) you posted in the private "Reports of Security Problems" area of the forum
c) your discussion was predominantly around sanitizing data and even adding use of mysql_real_escape_string()

In that regard there's no need for a packaged addon to deal with sanitization in this part of the code, since if one simply uses the existing built-in sanitization facilities when writing customizations, that will care of the potential security matters. In short: use bindVars where you need to sanitize your inputs.

As far as packaging an addon with specific new functionality which you think will benefit others, feel free to do as you wish.
Do remember that touching multiple core files does result in lots more support questions. Please be prepared to help people merge your addon's changes along with those of other addons which they might use that also alter the same files for some reason.

Ok dr byte ;)
Thanks for the tip, I'll pack it up and write a basic FAQ to help people with the basic issues one might get.