3
\$\begingroup\$

I'll gladly appreciate it if you could review my code below and let me know if they are sufficiently secure.

My main website and these scripts will use same database, so I need to make sure they are sufficiently secure before I put them live.

Since I cannot post more than two links, I'll post my Anti-SQL Injection script, and below are the actual two scripts.

<?php
if (!function_exists('sql_inject_chec')) {
function sql_inject_chec(){
 // Anti-SQL Injection
 $badwords = array("--",";","'",'"',"DROP",
                   "SELECT", "UPDATE", "DELETE",
                   "drop", "select", "update", "delete", 
                   "WHERE", "where","exec","EXEC",
                   "procedure","PROCEDURE");
 foreach($_REQUEST as $value)  {
  foreach($badwords as $word)
   if(substr_count($value, $word) > 0) {                      
    die("SQL Эnjection Denemesi Yaptэnэz... ama Baюarэsэz Oldunuz ;)");  
    }
  }
 }
}
sql_inject_chec();
?>

Ingame registration - SCRIPT

Score display - SCRIPT

\$\endgroup\$
9
\$\begingroup\$

This is not a constructive way to protect your scripts from SQL injection — the objective of SQL-injection prevention should not be to disallow characters or words, but to escape characters. What you have now, prevents your users from submitting perfectly valid English words such as select, update, etc. and/or perfectly valid characters such as --, " and '.

Consider this fictional comment:

When I select "where" from the menu, it updates the procedure -- strange.

This would be rejected by your code.

Now, I'm sure, for your application, this might (currently) be irrelevant, but your code is not very flexible or portable. What happens if your current application, or another future application needs to accept these words? You'd have to rewrite your code, and for a possible new application you wouldn't be able to copy existing code.

More importantly though, your current code is probably not elaborate enough to mitigate all possible attack vectors. You'd really be wise to implement prepared, parameterized statements by using PDO, instead of trying to prevent your users from entering possible SQL keywords and characters. With prepared, parameterized statements you don't have to worry about escaping the correct characters anymore — PDO (or, if the driver supports it natively, the database driver) will escape the parameter values you supply for you.

Let me give you a short introductory example of how you could implement SQL-injection prevention with PDO:

// create a database connection handle
$driver   = 'odbc';
$database = 'kn_online';
$username = '*redacted*';
$password = '*redacted*';
$dbh = new PDO( "$driver:$database", $username, $password );

// create a parameterized statement to select a record from CURRENTUSER
// uses a named parameter (denoted by :), called accountId
// that will be properly escaped by PDO
$sql = 'SELECT * FROM CURRENTUSER WHERE StrAccountID = :accountId';

// prepare the statement; returns a PDOStatement object
$stmt = $dbh->prepare( $sql );

// execute the statement by passing the parameter values in an associative array
$executionResult = $stmt->execute( array(
    ':accountId' => $acc
) );

// if the execution succeeded
if( $executionResult )
{
    // fetch the record as an associative array
    $record = $stmt->fetch( PDO::FETCH_ASSOC );
}

The beauty of prepared statements is that you can also easily execute them repeatedly, with new parameter values. Consider this example (connection handle creation omitted for brevity):

// insert into some table
$sql = 'INSERT INTO SOMETABLE VALUES( :id, :name )';

$stmt = $dbh->prepare( $sql );

// some fictional data
// parameter value keys don't need : prepended
$data = array(
    array( 'id' => 1, 'name' => 'Name One' ),
    array( 'id' => 2, 'name' => 'Name Two' ),
    array( 'id' => 3, 'name' => 'Name Three' )
);

foreach( $data as $datum )
{
    // using the same prepared statement
    // to execute for each datum
    $stmt->execute( $datum );
}
\$\endgroup\$
  • \$\begingroup\$ very nice, thank you for the Examples and Explanation. \$\endgroup\$ – Malachi Nov 1 '13 at 13:37
5
\$\begingroup\$

if (!function_exists('sql_inject_chec')) {

Oh dear, fail on line one.

If you don't know how your code is linking at runtime then you can't predict it's behaviour and hence can be fairly sure that you won't be able to say with any certainty that it's secure. Admittedly it's impossible to keep track of exactly how a complex system will behave - that's why we test code. A better approach is to define the function unconditionally - then if it fails try to work out why.

Next, why the mix of lower case and UPPER case words? I can simply bypass your check by sending DrOp TaBlE uSeRs;

Not reviewing the linked code.

\$\endgroup\$
2
\$\begingroup\$

personally I would just parameterize your Queries, this is going to be easier and more secure to do for your code than to try and weed out all the bad words or Escape Characters or other SQL Symbols.

I am not 100% sure how you do it PHP. lucky for us, someone has asked this question on StackOverflow already.

they go through ways to protect your Database with prepared statements and parameterized queries.

you should probably look at this, fix your code, and then repost.

\$\endgroup\$
-3
\$\begingroup\$

Trying to improve the structure of the code which you posted by removing the inner foreach loop.

foreach($_REQUEST as $value)  {
  if isset($badwords[$value]) {                                                                                                                                                                                                              
   die("SQL -njection Denemesi YaptMnMz... ama BaNarMsMz Oldunuz ;)");  
  }
} 
\$\endgroup\$
  • \$\begingroup\$ You don't use $value in your code, if you meant to invert the badwords array, then it won't pick up cases where the string contains more than just the badword - which is a pre-requisite for SQL injection, you ignore the case issue..... \$\endgroup\$ – symcbean Nov 5 '13 at 20:49
  • \$\begingroup\$ @symcbean you are right. I actually typed wrong and wrote $word instead of $value. I edited my answer.My intention with this code is to eliminate the inner foreach loop. \$\endgroup\$ – Kinjal Nov 6 '13 at 3:14
  • \$\begingroup\$ This is different functionality than the original code (and the original functionality isn't worth recreating to begin with...). Also, there's a syntax error in it. \$\endgroup\$ – Corbin Nov 6 '13 at 5:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.