[SECURITY] Hackers love the querystring!

[♥FWR Media]

Quite recently I was involved in a topic related to customer_testimonials contribution where the "hacking world" had been made aware of an opportunity to hack osCommerce via a vulnerability in the querystring ($_GET/$HTTP_GET_VARS).

 

Our response was to "cleanse" the incoming $_GET/$HTTP_GET_VARS. However this approach is a losing game as with security it never makes sense to run around trying to sure up contributions individually. So I've been looking at this on "another forum" and have come up with a solution that I would now call beta.

 

### DISCLAIMER ###

I am in no way suggesting that osC is vulnerable because it is not. But .. Harald and the team cannot possibly police all the contributions so this topic is aimed at contributions alone.

 

The concept here (not a new one) is to totally sanitise the incoming ($_GET/$HTTP_GET_VARS) at source (the top of catalog/includes/application_top.php) then to sanitise $_REQUEST by $_REQUEST = $_GET + $_POST (Yes we lost $_COOKIE).

 

By "sanitise" they key here is that we are ALLOWING certain characters to exist in the querystring NOT trying to clean away some dirty ones.

 

The danger here of course is that we inadvertently remove a character that is required for a legitimate osCommerce function.

 

After much testing allowed characters are as follows: -

a-z

A-Z

0-9

.(dot)

-(hyphen)

_(underscore)

{}

space (needed for search)

 

If any characters are found that need to be added I would really appreciate some feedback here please.

 

The code:

 

create a php file containing the following: -

 

<?php
function tep_clean_get__recursive($get_var)
 {
 if (!is_array($get_var))
 return preg_replace("/[^ {}a-zA-Z0-9_.-]/i", "", $get_var);

 // Add the preg_replace to every element.
 return array_map('tep_clean_get__recursive', $get_var);
 }
?>

 

 

Save it in catalog/includes/functions as security.php

 

 

catalog/includes/application_top.php

 

 

Find ..

 

 

define('PAGE_PARSE_START_TIME', microtime());

 

 

Add immediately above

 

 

// FWR Media Security
// If you want to turn security off just comment (//require_once) the line below
 require_once('includes/functions/security.php');
 if ( function_exists('tep_clean_get__recursive') ) {
// Recursively clean $HTTP_GET_VARS and $_GET
// There is no legitimate reason for these to contain anything but ..
// A-Z a-z 0-9 -(hyphen).(dot)_(underscore) {} space
 $HTTP_GET_VARS = tep_clean_get__recursive($HTTP_GET_VARS);
 $_GET = tep_clean_get__recursive($_GET);
 $_REQUEST = $_GET + $_POST; // $_REQUEST now holds the cleaned $_GET and std $_POST. $_COOKIE has been removed.
 }

 

This will greatly increase your security and your site should be totally free from querystring based hacks. The recent multitude of hack attempts on customer_tesimonials could not have happened if this had been in place. I believe information pages to be similarly vulnerable (amongst who knows how many other contributions).

 

This needs testing and feedback so if you find flaws in this .. again feed back please.

 

Below is a script so you can do some easy testing: -

 

Save the following as a php file called test_security.php. Save it to your server/test server.

 

		 <?php
	 // Access this file with the following querystring
	 // http://www.MYSITE.COM/test_security.php?products_id=5{6}&goodstuff={}aZ%2009._-&hack=[]||()+;
	   $HTTP_GET_VARS = tep_clean_get__recursive($HTTP_GET_VARS);
	   $_GET = tep_clean_get__recursive($_GET);
	   $_REQUEST = $_GET + $_POST; // $_REQUEST now holds the cleaned $_GET and std $_POST. $_COOKIE has been removed.
	 function tep_clean_get__recursive($get_var)
	   {
	   if (!is_array($get_var))
	   return preg_replace("/[^ {}a-zA-Z0-9_.-]/i", "", $get_var);

	   // Add the preg_replace to every element.
	   return array_map('tep_clean_get__recursive', $get_var);
	   }
	 die(print_r($_REQUEST));
	 ?>

 

 

Access the script using the following url ..

 

www(dot)MYSITE.COM/test_security.php?products_id=5{6}&goodoscstuff={}aZ 09._-&hack=[]||()+;

 

 

The $_GET, $HTTP_GET_VARS will be printed so you can see what they contain once they are sanitised.

 

After this initial test you can play with the querystring creating whatever $_GET/$HTTP_GET_VARS you wish.

[♥FWR Media]

Correction contributed by tomh .. Thanks Tom.

 

security.php should now contain ..

 

<?php
function tep_clean_get__recursive($get_var)
 {
 if (!is_array($get_var))
 return preg_replace("/[^ {}\%a-zA-Z0-9_.-]/i", "", $get_var);

 // Add the preg_replace to every element.
 return array_map('tep_clean_get__recursive', $get_var);
 }
?>

[Guest]

after testing and discussion on "another forum" the conclusion was that the preg_replace string needs a little tweak to prevent urlencoded string from breaking.

 

In the code above replace

  return preg_replace("/[^ {}a-zA-Z0-9_.-]/i", "", $get_var);

with

  return preg_replace("/[^ {}%a-zA-Z0-9_.-]/i", "", $get_var);

 

Tom

[Guest]

beat me to it!

[♥FWR Media]

beat me to it!

 

 

Yer a star Tom

[Guest]

thanks for posting this :thumbsup:

[Guest]

Thanks Tom, :thumbsup: Very well done.

 

Can you put it up as a contribution so this thread does not get lost and also to ensure that the correct number of spaces are included rather than rely on copy/paste.

[♥FWR Media]

Thanks Tom, :thumbsup: Very well done.

 

Can you put it up as a contribution so this thread does not get lost and also to ensure that the correct number of spaces are included rather than rely on copy/paste.

 

Tom? lol

 

I will put it up as a contribution but I want more feedback from testing first to ensure that no essential characters are missing.

 

In the meantime .. attachment included.

[♥FWR Media]

Attachment below: -

[Dennisra]

This is similar to what ScanAlert has recommended for PCI certification:

When accepting user input ensure that you are HTML encoding potentially malicious characters if you ever display the data back to the client.

 

Ensure that parameters and user input are sanitized by doing the following:

Remove < input and replace with <

Remove > input and replace with >

Remove ' input and replace with '

Remove " input and replace with "

Remove ) input and replace with )

Remove ( input and replace with (

 

I'll try the security contribution and report what the security scan reveals.

[♥FWR Media]

This is similar to what ScanAlert has recommended for PCI certification:

When accepting user input ensure that you are HTML encoding potentially malicious characters if you ever display the data back to the client.

 

Ensure that parameters and user input are sanitized by doing the following:

Remove < input and replace with <

Remove > input and replace with >

Remove ' input and replace with '

Remove " input and replace with "

Remove ) input and replace with )

Remove ( input and replace with (

 

I'll try the security contribution and report what the security scan reveals.

 

It's actually not similar that's just htmlspecialchars($var)/htmlentities($var). This security script is recursively REMOVING malicious characters from $_GET/$HTTP_GET_VARS at source.

[Dennisra]

It's actually not similar that's just htmlspecialchars($var)/htmlentities($var). This security script is recursively REMOVING malicious characters from $_GET/$HTTP_GET_VARS at source.

 

I understand. Thanks.

[♥FWR Media]

This topic is now CLOSED as a contribution has been uploaded.

 

http://www.oscommerce.com/forums/index.php?showtopic=293326