Disclosed Vulerability (SQL Inject) in Additional Images

[defa]

I don't want to annoy anybody - but two days ago I sent this E-Mail to the full.disclosure mailings list, to the developers of the software and to this forum - actually I can find an advice to patch or an patched release anywhere.

 

Please regard that this flaw has been user to really hack an well-visited Online Shop an steal a lot of data from it.

 

bye

defa

 

Hello!

 

Doing forensics in an hacked shop system we found the following vulnerability in the "Additional Images" Module of OScommerce from "Author: zaenal <zaenal AT paramartha.org>. Find more detailed information here: http://www.oscommerce.com/community/contributions,1032

 

Description:

 

If a anonymous remote user changes the value of 'products_id' when he gets "product_info.php" he is able to insert SQL Code in an SQL Query, if the module in question is installed.

 

Impact:

 

An attacker might read out parts or the whole of the database.

 

Code:

 

the following code on line 16 in SHOPROOT/catalog/includes/modules/additional_images.php doesn't check the value of the "products_id" variable.

 

$images_product = tep_db_query("SELECT additional_images_id, products_id, images_description, medium_images, popup_images FROM " . TABLE_ADDITIONAL_IMAGES . " WHERE products_id = '" . $HTTP_GET_VARS['products_id'] . "'");

 

Solution:

 

Contact the author/vendor.

 

Workaround:

 

Change line 16 in SHOPROOT/catalog/includes/modules/additional_images.php to:

 

$images_product = tep_db_query("SELECT additional_images_id, products_id, images_description, medium_images, popup_images FROM " . TABLE_ADDITIONAL_IMAGES . " WHERE products_id = '" . (int)$HTTP_GET_VARS['products_id'] . "'");

 

thanks to the guy who found the log entry in question.

 

bye

defa

[defa]

Here is a proof of concept - test this URI on an Shop-System with the module installed:

 

http://www.vulnerable_shop.foo/path_to_shop/product_info.php?cPath=1&products_id=29'%20UNION%20ALL%20SELECT%20%20*%20FROM%20countries%20WHERE%20countries_id%3E'0

 

bye

defa

[MarcoZorro]

Contributions arent supported by oscommerce so you will need to contact the author of the contribution to get this fixed.

[defa]

Thank you for your information.

 

I did allready contact the authors but without any reaction yet. As this is an security related problem and allready has been abused to steal data, I thought it might be a good idea to post the necessary data here to fix the problem and proof the vulnerability.

 

As any popular PHP-coded software is under heavy monitoring by loads of security interested people (the good and the bad) it might be a good idea to establish something like an security tracker or announce security threads on the mailing list.

 

bye

defa

[defa]

Well - I finally fixed it on my own - find the fixed contributions on the contributions site.

 

http://www.oscommerce.com/community/contributions,1032

 

bye

defa

[Iggy]

Well - I finally fixed it on my own - find the fixed contributions on the contributions site.

 

http://www.oscommerce.com/community/contributions,1032

 

bye

defa

 

And that's why individual contribs should be open to modification by other users re: the conversations on a forge system and locking contribs to mods from users other than the original authors.

 

Thanks for your work on this.

 

Iggy

[Guest]

What I do not understand is that's a generic vulnerability with sql queries and so why you think has something to do with that particular contribution? That can happen to every script deployed.

 

It's better to filter the critical parameters passed between pages to eliminate the risk when integrating contributions. So instead of the int cast, you could deploy something in the application_top.php to check the ids passed. Is far more effective.

[Iggy]

What I do not understand is that's a generic vulnerability with sql queries and so why you think has something to do with that particular contribution? That can happen to every script deployed.

 

It's better to filter the critical parameters passed between pages to eliminate the risk when integrating contributions. So instead of the int cast, you could deploy something in the application_top.php to check the ids passed. Is far more effective.

 

That would make an excellent contribution, yes?

 

Iggy

[Guest]

I have something basic already deployed, just need some feedback then I can post it with the contributions and some notes. If you have a bit of time to go through the code and/or test it will much appreciate it. Here it is:

 

In application_top.php right after the osc copyright notice add this:

 

  foreach($HTTP_GET_VARS as $numeric_sec => $string) {
if( stristr($numeric_sec, '_id') !== false ) {
/*
  if( !is_numeric($string) ) 
	die($numeric_sec . '=' . $string);
*/
  settype($HTTP_GET_VARS[$string], 'integer');
}
 }
 reset($HTTP_GET_VARS);

 foreach($HTTP_POST_VARS as $numeric_sec => $string) {
if( stristr($numeric_sec, '_id') !== false ) {
  settype($HTTP_POST_VARS[$string], 'integer');
}
 }
 reset($HTTP_POST_VARS);

 $numeric_sec_array = array('page','edit','id','ID');
 for($i=0,$j=count($numeric_sec_array); $i<$j; $i++ ) {
if( isset($HTTP_GET_VARS[$numeric_sec_array[$i]]) ) {
/*
  if( !is_numeric($HTTP_GET_VARS[$numeric_sec_array[$i]]) ) 
	die($numeric_sec_array[$i] . '=' . $HTTP_GET_VARS[$numeric_sec_array[$i]] );
*/
  settype($HTTP_GET_VARS[$i], 'integer');
}
if( isset($HTTP_POST_VARS[$numeric_sec_array[$i]]) ) {
  settype($HTTP_POST_VARS[$i], 'integer');
}
 }

 

you could remove the comments from the "die" statements to see the params/ids as they passed. (Or duplicate it for the post vars during testing)

[Guest]

oops the settype parameter should be this within each foreach loops

 

settype($HTTP_GET_VARS[$numeric_sec], 'integer');

and

settype($HTTP_POST_VARS[$numeric_sec], 'integer');

Edited October 12, 2005 by enigma1

[MarcoZorro]

It's better to filter the critical parameters passed between pages to eliminate the risk when integrating contributions. So instead of the int cast, you could deploy something in the application_top.php to check the ids passed. Is far more effective.

 

Actually a whitelist approach is sometimes hard to keep track off and type casting is considered good programming practice and is used widely to prevent sql injection.

[Guest]

Actually a whitelist approach is sometimes hard to keep track off and type casting is considered good programming practice and is used widely to prevent sql injection.

 

Why its easier to check and verify the "data type casts" on 100 contributions deployed on a store, than to filter these parameters right at the beginning of the script? Cast omissions are unintentional yet often and hard to debug. Pass a parameter of an alphanumeric id and then see in your html source code generated, in how many places such a string appears. Its impossible to study the consequences between page transitions/combinations.

[MarcoZorro]

Why its easier to check and verify the "data type casts" on 100 contributions deployed on a store, than to filter these parameters right at the beginning of the script? Cast omissions are unintentional yet often and hard to debug. Pass a parameter of an alphanumeric id and then see in your html source code generated, in how many places such a string appears. Its impossible to study the consequences between page transitions/combinations.

 

Actually done right its very simple... it all depends on your level of experience I guess.

 

The code you posted above IMO is a cheap hack... and yes I am entitled to an opinion :)

[Guest]

Actually done right its very simple... it all depends on your level of experience I guess.

 

The code you posted above IMO is a cheap hack... and yes I am entitled to an opinion

 

What you're expecting with the correct cast in each case it's not simple and requires a lot of maintenance because the global arrays are manipulated in many different ways. So you could pass the parameter to a variable first and then do something with it before finally executing the sql query. And I am not going to spend the time calculating and estimating the infinite number of possible scenarios passing parameters from one page to the next.

 

The code above which I deployed has no hard-coded list of ids. Its simply process the parameters passed. But of course you do whatever you think is more convenient with your experience. (ie checking implications with script combinations)