Is this code sufficient to protect me from SQL injection attacks, and PHP injection attacks?

Question

I have this function in an include file of functions:

function strclean ($string) {
  $outstr = '';
  if (strlen ($string) > 0) {
    $ix = 0;
    $char = substr ($string, $ix, 1);
    // strip leading spaces
    while ($char == ' ') {
      $ix = $ix + 1;
      $char = substr ($string, $ix, 1);
      }
    // disarm naughty characters
    while ($ix < strlen ($string)) {
      $char = substr ($string, $ix, 1);
      if ($char == '<') $char = '&lt;';
      else if ($char == '>') $char = '&gt;';
      else if ($char == '"') $char = '&quot;';
      else if ($char == '&') $char = '&amp;';
      else if ($char < chr(20)) $char = '';
      $outstr = $outstr . $char;
      $ix = $ix + 1;
      }
    // strip trailing spaces
    while (substr ($outstr, strlen ($outstr) - 1, 1) == ' ' && strlen ($outstr) > 0) {
      $outstr = substr ($outstr, 0, strlen ($outstr) - 1);
      }
    $outstr = mysql_real_escape_string ($outstr);
    }
  return $outstr;
  }

Later on in my page, I have various strings returned from form input such as this example:

$username = strclean ($_POST['username']);
$password = strclean ($_POST['password']);

And even later, I have the following SQL:

$result = mysql_query ('SELECT * FROM users WHERE 
  username = "' . $username . '"', $dbconn) or die (mysql_error());

I don't search for username and password together in the query. A few lines after this, I check for a valid password like this:

if ($rowsfound == 1) {
  $userrow = mysql_fetch_array ($result);
  $userword = $userrow ["password"];
  if ($userword == $password) {
     // logon
     }
  else {
    // incorrect password
    }
  }
else if ($rowsfound == 0) {
  // unknown user
  }  
else {
  // something strange happened!  possible sql injection attack?
  }

Why are you trying to make one yourself instead of using functions in the PHP core? — Richard Simões, Mar 31 '12 at 00:32
Given that it's long and complicated to read, I'd say this is not a feasible approach. — Kerrek SB, Mar 31 '12 at 00:34
possible duplicate of [Secure XSS cleaning function (updated regularly)](http://stackoverflow.com/questions/6382442/secure-xss-cleaning-function-updated-regularly) — g.d.d.c, Mar 31 '12 at 00:45
possible duplicate of [How to prevent code injection attacks in PHP?](http://stackoverflow.com/questions/1205889/how-to-prevent-code-injection-attacks-in-php) — Ben, Mar 31 '12 at 17:41
Please see [this answer](http://stackoverflow.com/a/6382505/908471) to a similar question. — Anthony Ledesma, Mar 31 '12 at 00:37

score 1 · Accepted Answer · answered Mar 31 '12 at 00:55

The general rule is to deny everything and allow through only valid characters, rather than removing what you consider to be invalid. The most important aspect is what you do with these string afterwards. If you have a line later saying: tsql = "SELECT * FROM Users WHERE Username='" . $username . "' AND " then this is the primary area of risk, although mysql_real_escape_string should avoid this.

By using a libraries or features that allow passing of parameters to the database there can never be any sql injection, as the database parameters can't be interpreted into TSQL, leaving only PHP/Javascript injection as a possibility. Basically, look at the bind_param functions as the only true protection.

Whenever displaying data on-screen, consider something like htmlspecialchars() to convert it to HTML. There's no point in storing something escaped if you need it un-escaped later, and raw data in the database poses no risk as long as you always consider it raw.

In summary, the code you list may or may not reduce injection, but there are too many combinations to exclude every possibility, including aspects such as a user using single quotes (you're only replacing double quotes). All user input data is potentially dangerous. Feel free to store it raw, but whenever USING it make sure your operations are protected using one of the above options.

My PHP is a bit rusty now, but exactly the same rules apply to SQL Server, Oracle, .NET, Java any any other database/language.

Later, I have a line that says: `$result = mysql_query ('SELECT * FROM users WHERE username = "' . $username . '"', $dbconn);` I don't search for the password within the query. A few lines later, I test the supplied password against the password on the db. — Fredashay, Mar 31 '12 at 01:42
Yes, this is exactly the type of thing to avoid! If someone finds a flaw in your complex logic they can inject any statement into that SQL. Look at the `bind_param` functions in mysql lib and use those. No escaping will then be required at all. In theory you can assume that `mysql_real_escape_string` protects against this, but I never take that chance. — JSobell, Apr 01 '12 at 04:12

Is this code sufficient to protect me from SQL injection attacks, and PHP injection attacks?

1 Answers1