Jump to content
wetzel

Querystring protection against SQL injection for custom code

Recommended Posts

For the past couple of months I've been rebuilding heavily modded 2.2 site (10000s of lines of custom code) within Phoenix. It's going alright. Things are working okay.

Today I noticed we're being attacked by SQL injection methods, ie. get variables concatenated with  stuff like 51111111111111'%20UNION%20SELECT%20CHAR(45,120,49,45,81,45),CHAR(45,120,50,45,81,45),CHAR(45,120,51,45,81,45),CHAR(45,120,52,45,81,45),CHAR(45,120,53,45,81,45),CHAR(45,120,54,45,81,45),CHAR(45,120,55,45,81,45),CHAR(45,120,56,45,81,45),CHAR(45,120,57,45,81,45),CHAR(45,120,49,48,45,81,45),CHAR(45,120,49,49,45,81,45),CHAR(45,120,49,50,45,81,45),CHAR(45,120,49,51,45,81,45),CHAR(45,120,49,52,45,81,45),CHAR(45,120,49,53,45,81,45),CHAR(45,120,49,54,45,81,45),CHAR(45,120,49,55,45,81,45),CHAR(45,120,49,56,45,81,45),CHAR(45,120,49,57,45,81,45),CHAR(45,120,50,48,45,81,45),CHAR(45,120,50,49,45,81,45),CHAR(45,120,50,50,45,81,45),CHAR(45,120,50,51,45,81,45),CHAR(45,120,50,52,45,81,45),CHAR(45,120,50,53,45,81,45),CHAR(45,120,50,54,45,81,45)--%20%20

This does generate an SQL error visible to the user, which is often the point of these things, I understand, at the start of the attack.

In the old 2.2 framework, we sanitized get variables within application_top using an addon 'Security Pro', which seemed to work pretty well. I'm pretty sure this relied on global variables in a way the new framework doesn't permit, though I'm not sure. At any rate, Phoenix doesn't seem to have a method of sanitizing get variables pre-installed in a global way. Nobody's fault but our own, but this is leaving us vulnerable in many of our custom scripts in that these injections are actually being evaluated as SQL. 

Is there a general protocol in Phoenix, an addon or accepted method, for protection against SQL injection that does not involve individually cleansing get variables at the script level? This would be a lot of work and we might miss something.

If there is not and we need to go script by script, is there a good practice PHP implementation at the script level that can be recommended?

Thanks!

 

 

Share this post


Link to post
Share on other sites

Show an example of such at the Demo Site:

http://template.me.uk/phoenix/

In other words post up a URL that includes the injection so that we can see the produced output.
Then it is possible to further diagnose if that is what is needed.


Help shape the future of Phoenix; join the Phoenix Club

Share this post


Link to post
Share on other sites

Thank you for your reply, but I'm not sure if you understand the question, which is more general. Basically I am hoping to find out if there a general script method for Phoenix that functions such as Security Pro functioned in the old framework to sanitize get variables as a general, sitewide protocol. Here is Security Pro.  https://apps.oscommerce.com/zNblL . Is there something like this for Phoenix?

It looks like I can do this at the script level with this statement.

$my_get_variable=mysqli_real_escape_string($db_link,$_GET['my_get_variable']);

I am hoping to avoid doing this script by script. It's no big deal if I need to. Thanks.

Share this post


Link to post
Share on other sites

The only reason for "security pro" was poorly coded addons. 
If you do not add poorly coded addons, then you should not need it.

Hence my request that you show a q-string manipulation at the Demo site - the demo site has no addons.

If you can't show an example, then I will amend the topic title to more accurately reflect the thread.


Help shape the future of Phoenix; join the Phoenix Club

Share this post


Link to post
Share on other sites

I understand. That's fine to alter the topic title. I see now that the current title implies a vulnerability in the standard installation. Maybe 'Protecting custom coded query strings from SQL injection within the Phoenix framework'. Someone may be inspired to add a few paragraphs of 'best practices' which would be helpful for pluggers like me.

An aside, maybe not so important, maybe a little defensive, but my understanding is that Security Pro arose to address vulnerabilities in the base 2.2 installation. Additionally, if you did have Security Pro present, then an addon wouldn't necessarily be terrible programming that were to rely on it. It was useful in that it allowed simple handling of get variables and they could be passed to SQL queries without the need to sanitize them at the script level. It's not properly consistent with other security measures in the old installation, a bit ad hoc, but it worked. It doesn't any more. That's okay!

So I guess the better question would be: Is there an addon for protecting simply passed and simply called get variables in a global manner within Phoenix. If not, I guess I can just use mysqli_real_escape_string script by script. It's no big deal. 

If there is a better way, it is very much appreciated. It might be valuable for me or for another in a similar predicament in the future. 

Cheers.

 

 

Share this post


Link to post
Share on other sites
2 hours ago, wetzel said:

Is there a general protocol in Phoenix, an addon or accepted method, for protection against SQL injection that does not involve individually cleansing get variables at the script level? This would be a lot of work and we might miss something.

The accepted best practice is to cleanse the variables not at the _GET level but at the level of insertion into the database.  That's what the cast to int and tep_db_input do.  Because if you sanitize at the _GET level and then don't sanitize at the insertion level, there are attacks that can get past the _GET level sanitization. 

Note that if you really want to do this at the _GET level, you could just do something like

function sanitize_recursive(&$data) {
  foreach ($data as $key => &$value) {
    if (is_null($value)) {
      continue;
    } elseif (is_array($value)) {
      sanitize_recursive($value);
    } else {
      $value = tep_db_input(tep_db_prepare_input($value));
    }
  }

  unset($value);
}

sanitize_recursive([$_GET, $_POST]);

But like I said, that's the wrong place to do it.  And doing it there can cause problem in other things.  For example, it can make the display wonky.  Because you're mangling the data.  Which is why tep_db_output exists.  To take the mangled data from the database and unmangle it for use in the HTML. 

In Phoenix (as in osCommerce), the standard way to handle this is to call tep_db_prepare_input and tep_db_input (or cast to int) on every piece of data before including it in the SQL.  So

$id = tep_db_prepare_input($_GET['id']);
$key = tep_db_prepare_input($_GET['key']);
$query = tep_db_query("SELECT * FROM table WHERE id = " . (int)$id . " AND key = '" . tep_db_input($key) . "'");

The tep_db_input does the mysqli_real_escape_string call for you.  And if you do it here, at the last moment, then there is no way that you might miss doing it some places (or accidentally undo it). 

Longer term we should probably switch to PDO or something that handles placeholders.  Something like

$id = tep_db_prepare_input($_GET['id']);
$key = tep_db_prepare_input($_GET['key']);
$query = $Phoenix_database->query("SELECT * FROM table WHERE id = ? AND key = ?", (int)$id, $key);

or

$query = $Phoenix_database->query("SELECT * FROM table WHERE id = :id and key = :key", ['id' => $id, 'key' => $key]);

Then the string wouldn't need to be escaped (or you could consider it to be automatically escaped).  But that's a rather large change.  It's sort of on my TODO list if I can figure out a way to do it in stages. 


Always back up before making changes.

Share this post


Link to post
Share on other sites
9 hours ago, wetzel said:

Is there a general protocol in Phoenix, an addon or accepted method, for protection against SQL injection that does not involve individually cleansing get variables at the script level? This would be a lot of work and we might miss something. 

Installing the OSC Error Handler will prevent the errors from showing to your customers, or more importantly, to the hacnkers.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

×