SQL Injections

[gazalec]

Hi I was just wondering if anyone could help with SQL injections i just want to know a basic way to increase security, i know it'll never be 100% secure but just a few things i can do to improve it as much as possible

 

thanks

[Balmung-San]

Sanitize all your variables.

 

In other words, validate input, and if necessary, look for keywords that might indicate a SQL Injection (i.e., SELECT, INSERT, UPDATE, DESCRIBE, etc.).

 

One thing I've thought about (though haven't had time to test) is to use this:

 

foreach($_POST as $key => $val)

{

$_POST[$key] = ereg_replace("\$","xXx",$val);

}

 

To replace all $'s in $_POST with xXx. This will prevent variable usage within the string. Like I said, I haven't tested this yet so I'm not sure if/how well it works.

[gazalec]

the reason i need this is that i have a products page and all products are displayed on this one page using a database it's just if you choose one product it displays all its information well the way i am doin this is there is a product list page and i use :-

 

<a href="product.php?product=example">

 

the product page then assigns the variable

 

$product=$_GET['product'];

 

this is then searched in a database and all the information needed is took and displayed

 

The only problem is i know this is not secure at all as it can be subjected to SQL injections very easily, but i really need to work this way as there is hundreds of products and to make a page for each product would be too time consuming.

 

So if you have any suggestions they would be very welcomed

[obsidian]

With that one, you could simply set up an array of allowable options for the $_GET['product'] variable based on your predefined links. Then, if the URL has anything besides your predefined values, you know there has been some tampering of some sort, and you can ignore it.

<?php
// Predefined values
$options = array('example'); // add anything else here you wish to allow
$product = $_GET['product'];
if (in_array($product, $options)) {
  // It's a valid request, so parse it.
}
?>

[Balmung-San]

I would make an array of keywords which are all MySQL reserved words:

 

$keywords = array('SELECT','INSERT','UPDATE','DESCRIBE',etc.)

 

Then when you get your product:

 

$product=$_GET['product'];

$upprod = strtoupper($product);

foreach($keyword as $word)

{

if(substr_count($word,$upprod) > 0)

{

//error out

}

}

//do stuff!

[obsidian]

I would make an array of keywords which are all MySQL reserved words:

 

I don't like restricting use of words as common as insert, update, delete, create, etc. at all. If you are accepting user text input to query the database against, you are much better off creating a white list of allowable items and/or patterns than trying to come up with a comprehensive black list of things you want to screen out. You will almost always come up with something you've forgotten or a new threat if you depend on a black list. It is also often a longer comparison. If you come up with a very defined list of things that are allowed, you will be in a much better position (especially if you have to update your list later).

[gazalec]

ok so would this protect against SQL injections? because i tried it and it brings up a valid page for a product on the array but i set it up to display a blank page for an invalid product (for now) but if it is displaying the blank page then isn't it technically still accepting the invalid data which could be an SQL injection

[Balmung-San]

I'll agree that a white list is a better solution for security, gazalec did state that there were hundreds of products, which would require a list of every product to be in the file. So while that is the tighter security, I'm not sure how hard the overhead will hit, especially if it was configured for a low timeout. Then again, I've never really looked into how fast PHP could create an array of hundreds of items, so it may not be that bad.

 

gazalec, as long as you detect that the variable is bad before you query, and use something like die() or exit(), then it'll never query.

[obsidian]

ok so would this protect against SQL injections? because i tried it and it brings up a valid page for a product on the array but i set it up to display a blank page for an invalid product (for now) but if it is displaying the blank page then isn't it technically still accepting the invalid data which could be an SQL injection

 

It's what you do with the data that opens you up to SQL injections. The fact that I alter a URL to attempt a SQL injection means nothing... the failure to properly handle that attempt is where the issue lies. So, if you screen out your invalid attempts and display a blank page without running a query to MySQL with the faulty data, you are not prone to SQL injection, because you are never presenting that data to your database.

 

Does that make sense? You can never fully control the data that people send to your site, but you can control what you do with it once it has been received.

[gazalec]

ah i get it now, can i just ask a misc. question to do with SQL injections, well they say that a person can enter DROP TABLE `product_list`, but how does that person know your table name  ??? i mean if you made it quite random but simple enough for you to know it then how would the drop your table

[roopurt18]

1) Sanitize all data going into the DB:

<?php
   // my_clean
   // $data - data to clean before inserting into db
   // $add_sq - true to add single quotes, false to just clean
   // $force_string - true to treat the data as a string
   // RETURN: properly escaped and db safe data
   function my_clean($data, $add_sq = true, $force_string = false){
     if(is_array($data)){
       foreach($data as $key => $val){
         $data[$key] = $this->my_clean($val, $add_sq, $force_string);
       }
     }else{
       if(!is_numeric($data) || $force_string){
         if(get_magic_quotes_gpc()){
           $data = stripslashes($data);
         }
         $data = trim($data);
         if($add_sq){
           $data = "'" . mysql_real_escape_string($data) . "'";
         }else{
           $data = mysql_real_escape_string($data);
         }
       }
     }
     return $data;
   }
?>

 

2) Use a clean array when passing parameters into the DB:

<?php
function insertDB($val1, $val2, $val3){
  $Clean = Array();
  $Clean['Val1'] = my_clean($val1);
  $Clean['Val2'] = my_clean($val2);
  $Clean['Val3'] = my_clean($val3);
  $sql = "INSERT INTO table (fld1, fld2, fld3) VALUES ("
        . "{$Clean['Val1']}, {$Clean['Val2']}, {$Clean['Val3']})";
  $q = mysql_query($sql);
  // Do error checking and return
}
?>

 

3)  I've never done this, but I've toyed with the idea of creating regexps to match the various types of SQL statements.  So let's say you're using SELECT, after building your SQL statement you could match it against the regexp for a SELECT and if it fails, don't run the query.  Let's say you're creating a delete SQL statement.  If the statement is always to look the same, you could create a regexp that matches the statement and prevent a crafty hacker from appending a dreaded OR 1=1 onto the end of you WHERE clause.

 

Just some suggestions.

[obsidian]

I'll agree that a white list is a better solution for security, gazalec did state that there were hundreds of products, which would require a list of every product to be in the file. So while that is the tighter security, I'm not sure how hard the overhead will hit, especially if it was configured for a low timeout. Then again, I've never really looked into how fast PHP could create an array of hundreds of items, so it may not be that bad.

 

Well, in the case of hundreds, if not thousands of records, you can still come up with a cleanly, easily matched pattern for all values that would be valid item calls. For instance, if I know that anything that is a valid request for an item contains only letters and numbers, I could simply run a check like this:

<?php
$product = $_GET['product'];
if (preg_match('|^[a-z\d]+\z|i', $product)) {
  // It's clean (contains only letters and digits), so run your qeury
} else {
  // May contain tampering, don't query
}
?>

 

I'm not necessarily encouraging the listing of every item into an array as much as trying to teach the principle of white-listing your site. It is much more sound control over your data.

[obsidian]

ah i get it now, can i just ask a misc. question to do with SQL injections, well they say that a person can enter DROP TABLE `product_list`, but how does that person know your table name  ??? i mean if you made it quite random but simple enough for you to know it then how would the drop your table

 

Well, here's the key: if I can get your site to error out with a SQL injection and give me the type of errors I'm looking for as a hacker, I can then use the information I learn from the errors I generate and send SQL statements that will then give me a list of all your tables and even the column names of those tables and all the data in them. To be honest, someone dropping my table would be the least of my worries. Someone gaining access to altering membership information, especially when finances could be affected is a much greater worry IMHO.

[Balmung-San]

Oh yes, never did think of regex as a solution for that. Then again, I'm fairly new to regex, so I haven't quite realized just how many places they can be used. Yes, white listing is a very good practice to follow, but it isn't followed in as many places as it should be. I was never really against white listing, I just naturally tend to jump towards black listing for some reason.

 

gazalec, what obsidian said is perfect. However, if you're worrying about them finding your table names without doing what obsidian said, the best way would be to obscure it. That is, instead of using a logical sounding table name, use one that makes no sense. To protect yourself even better, setup a table with a logical sounding name so that way they can validly drop it, but not affect your site.

[gazalec]

sorry this is a two part, in the array couldn't i use a variable, e.g,

 

connect to database //

 

select products that are relavant //

 

assign to $product

 

then check if $input=$_GET['input'];

 

put the $product in an array then check if $input matches any in the array, it just new products could be added and it means i could just update the database,

 

Part 2,

 

i @ all of my sql statements after i know it works so if anything does go wrong it doesn't say any errors, how would this work ???

[obsidian]

i @ all of my sql statements after i know it works so if anything does go wrong it doesn't say any errors, how would this work ???

 

I'll answer this one first, and see if I have time to get to the other one as well ... To be blunt, it's never a good practice to rely on '@' to filter your DB errors. The solution to this is two fold:

 

1. Create handling techniques in such a way that you always have a general fall back option so you are never outputting the actual SQL errors to the screen for a user to see.

 

2. Once you have your site ready, you should turn off SQL error reporting completely on your production location. This helps not only to filter what people are seeing, but also protects you in that you have a development environment that you have tested on and know what the errors would be if they were shown.

[roopurt18]

I personally never use die in my code.  The type of information programmers tend to place in their die statements is exactly the type of information you don't want non-developers to have access to.

 

It's a much better practice, IMO, to program your application to handle failure in a graceful manner.  For example, if someone attempts to hack my site, I prefer the product page to come up with a message stating, "Sorry, but no products found." instead of "die: You have an error in your MySQL statement.  Check the syntax for the correct usage near your_sql_query."

[Balmung-San]

I tend to use die() in production scripts, however I wouldn't use something like die(mysql_error()) in a production script. Instead I would use die("Hacking attempt noted.") or something similar. That way it dies off, gives them something, and prevents the hacking attempt. Which is basically what you do, only I like to tell them in a not-so-nice manner.

[obsidian]

I personally never use die in my code.  The type of information programmers tend to place in their die statements is exactly the type of information you don't want non-developers to have access to.

 

Agreed 100%. It's great for debugging, not so hot for production. This is where I am absolutely in love with PHP5's Exception handling. You can throw an exception on anything and handle it gracefully instead of letting the script throw whatever kind of errors it feels the need to generate.

[roopurt18]

Instead I would use die("Hacking attempt noted.") or something similar. That way it dies off, gives them something, and prevents the hacking attempt. Which is basically what you do, only I like to tell them in a not-so-nice manner.

 

That particular method gives the would-be hacker a little bit of extra information.  Namely that you handled the particular attack that they tried, which is bad practice IMO.  In addition, you run the risk of a legitimate user viewing that message, which is something you don't want to spring on a legitimate user.