0

My form input Sanitizing function:

function clean_input($data) 
            {
              $data = trim($data);
              $data = strip_tags($data);
              $data = stripslashes($data);
              $data = htmlspecialchars($data);
              return $data;
            }

My SQL lines:

$q  = "SELECT * FROM users WHERE username = {mysqli_real_escape_string   ('$username')} AND password = {mysqli_real_escape_string('$password')}";
                            $r = mysqli_query($dbc, $q);

Is this correct usage ?

tim
  • 1,999
  • 17
  • 32
Robin
  • 37
  • 2

1 Answers1

4

Is this correct usage ?

No, it's not. It would be reasonably secure if you were using single quotes around the input and if you are not using certain multibyte charsets, but it's really not the recommended approach to security or usability/data integrity.

Preventing SQL Injection

Use prepared statements. With PHP, you can use mysqli or PDO (also note this, which might be a problem depending on your versions/configuration).

Here is what OWASP has to say:

This second technique is to escape user input before putting it in a query. However, this methodology [mysqli_real_escape_string] is frail compared to using parameterized queries and we cannot guarantee it will prevent all SQL Injection in all situations. This technique should only be used, with caution, to retrofit legacy code in a cost effective way.

clean_input function

Let's look at each of these functions:

  • trim: removes whitespace from beginning and end of string. This might be useful in some situations, and doesn't hurt in too many situations. It doesn't really affect security.
  • strip_tags: removes NULL bytes, HTML and PHP tags. This can be a nice first line of defense against some attacks (some code injections, some XSS, some LFI/RFI), but it can corrupt your data (eg the smaller sign (<) is interesting becomes the smaller sign (). It is also not a proper defense against any of the mentioned attacks, so I wouldn't really recommend using it like this.
  • stripslashes: as the name suggests, it removes slashes. This is only useful if magic quotes is on (which is the functionality that adds slashes), so you should check that first, as otherwise you may corrupt your data. If magic quotes is on (it shouldn't be), using stripslashes is recommended, as otherwise you will have corrupted data.
  • htmlspecialchars again, this might corrupt your data (as in, you are not saving your original data in the database, but changed data). It is proper defense against XSS in most situations (see here for exceptions, and use ENT_QUOTES), but it really should not happen before inserting data into the database, for two reasons: 1. corrupted data 2. This approach makes it easy to introduce XSS vulnerabilities. XSS is a vulnerability that is introduced when echoing data, so that is when it should be prevented. Otherwise, it will be hard to remember which data is secure and which is not.

To summarize: Your approach changes input data before it is inserted into the database, which could impact usability. This doesn't even add that much security, and is certainly not considered a best practice approach to security.

tim
  • 1,999
  • 17
  • 32
  • My gratefulness at your extended response and insights. I learned much from your comments, and have new directions within which to study. – Robin Aug 09 '15 at 17:57