views:

9848

answers:

15

So specifically in a mysql database. Take the following code and tell me what to do.

// connect to the mysql database

$unsafe_variable = $_POST["user-input"];

mysql_query("INSERT INTO table (column) VALUES ('" . $unsafe_variable . "')");

// disconnect from the mysql database
+1  A: 

Parametrized sql (check your sql provider) plus htmlentities

George Mauer
htmlentities won't protect you against SQL Injection, XSS yes, but not htmlentities.
Allain Lalonde
+9  A: 

I'd recommend using PDO (PHP Data Objects) to run parameterized SQL queries. Not only does this protect against SQL injection, it also speeds up queries. And by using PDO rather than mysql_, mysqli_, and pgsql_ functions, you make your app a little more abstracted from the database, in the rare occurence that you have to switch database providers.

Kibbee
+1  A: 

you could do something basic like this.

   $safe_variable = mysql_real_escape_string($_POST["user-input"]);
   mysql_query("INSERT INTO table (column) VALUES ('" . $safe_variable . "')");

this won't solve every problem but its a very good stepping stone. i left out obvious items such as checking the variable's existance, format (numbers, letters, etc)

Tanerax
A: 

I would say something like this in addition to mysql_real_escape_string():

<?php
// Quote variable to make safe
function quote_smart($value)
{
    // Stripslashes
    if (get_magic_quotes_gpc()) {
        $value = stripslashes($value);
    }
    // Quote if not integer
    if (!is_numeric($value)) {
        $value = "'" . mysql_real_escape_string($value) . "'";
    }
    return $value;
}
?>
Swish
A: 

Anyone suggesting anything besides prepared statements or parametrized queries deserves a downvote:

Parametrized queries in mySQL:

http://us.php.net/manual/en/mysqli.prepare.php

FlySwat
Anybody recommending using libraries where the function names begin with the databqse name should also get a downvote.
Kibbee
Hey, I don't program in PHP, so don't judge me for their crappy mySQL integration.
FlySwat
You are also making the strong assumption that they are able to use paramterised queries. I know a couple of systems that have mutli-database interface which strongly limits the types of things you can do.
kaybenleroll
That is only a part of the answer. Url scrubbing, security audits, and common sense are all parts of the solution. You have to apply them all, and with the knowledge that anything less is a false sense of security.
joseph.ferris
Can you elaborate why ANY other suggestion should be down voted?
Nikhil
+25  A: 

You've got two options - Escaping the special characters in your unsafe_variable, or using a parameterized query. Both would protect you from SQL Injection. The parameterized query is considered better practice, but escaping characters in your variable will require fewer changes.

We'll do the simpler string escaping one first.

//connect

$unsafe_variable = $_POST["user-input"];
$safe_variable = mysql_real_escape_string($unsafe_variable);

mysql_query("INSERT INTO table (column) VALUES ('" . $safe_variable . "')");

//disconnect

See also, the details of the mysql_real_escape_string function.

To use the paramterised query, you need to use MySQLi rather than the MySQL functions. To rewrite your example, we would need something like the following.

<?php
$mysqli = new mysqli("server", "username", "password", "database_name");

// TODO - Check that connection was successful.

$unsafe_variable = $_POST["user-input"];

$stmt = $mysqli->prepare("INSERT INTO table (column) VALUES (?)");

// TODO check that $stmt creation succeeded

// s means the database expects a string
$stmt->bind_param("s", $unsafe_variable);

$stmt->execute();

$stmt->close();

$mysqli->close();
?>

The key function you'll want to read up on there would be mysqli::prepare.

Also, as others have suggested, you may find it useful/easier to step up a layer of abstraction with something like PDO.

Matt Sheppard
+116  A: 

Use prepared statements. These are SQL statements that sent to and parsed by the database server separately from any parameters.

If you use PDO you can work with prepared statements like this:

$preparedStatement = $db->prepare('SELECT * FROM employees WHERE name = :name');

$preparedStatement->execute(array(':name' => $name));

$rows = $preparedStatement->fetchAll();

(where $db is a PDO object, see the PDO documentation)

What happens is that the SQL statement you pass to prepare is parsed and compiled by the database server. By specifying parameters (either a ? or a named parameter like :name in the example above) you tell the database engine where you want to filter on. Then when you call execute the prepared statement is combined with the parameter values you specify.

The important thing here is that the parameter values are combined with the compiled statement, not a SQL string. SQL injection works by tricking the script into including malicious strings when it creates SQL to send to the database. So by sending the actual SQL separately from the parameters you limit the risk of ending up with something you didn't intend. Any parameters you send when using a prepared statement will just be treated as strings (although the database engine may do some optimization so parameters may end up as numbers too, of course). In the example above, if the $name variable contains 'Sarah'; DELETE * FROM employees the result would simply be a search for the string "'Sarah'; DELETE * FROM employees", and you will not end up with an empty table.

Another benefit with using prepared statements is that if you execute the same statement many times in the same session it will only be parsed and compiled once, giving you some speed gains.

Oh, and since you asked about how to do it for an insert, here's an example:

$preparedStatement = $db->prepare('INSERT INTO table (column) VALUES (:column)');

$preparedStatement->execute(array(':column' => $unsafeValue));
Theo
great resource, thanks!
Andrew G. Johnson
the link in this document appears to be out of date, I believe the following link will work: http://php.net/manual/en/book.pdo.php
Dave
This make sense, However, PDO is an extension right? meaning it needs to be installed? Is there a way I can check to see if it is installed? Also I am using a shared hosting, so if it is not installed and my hosting provider cannot/will not install it, is there an alternative to using a PDO? Thank You!!
John Isaacks
RTFM: http://php.net/manual/en/pdo.installation.php PDO is bundled by default since PHP 5.1. Not all drivers for all databases may be installed, but if your host supports MySQL and PHP later than 5.1 it would be very surprising if it didn't have the MySQL PDO driver installed. Create a page with <?php phpinfo(); ?> and view it in a browser, look for PDO and you will see info on which drivers are installed.
Theo
+1  A: 

Use PDO and prepared queries.

($conn is a PDO object)

$stmt = $conn->prepare("INSERT INTO tbl VALUES(:id, :name)");
$stmt->bindValue(':id', $id);
$stmt->bindValue(':name', $name);
$stmt->execute();
Imran
+2  A: 

I use a combination of sprintf ( http://www.php.net/sprintf ) and mysql_real_escape_string functions..

for example I want to select a row using it's id value:

$query = "SELECT name FROM names WHERE id = %u"; $query = sprintf($query, $id_val);

that way I make sure only positive integers get through the query..

and when I want to insert a string into the query I use "%s" instead of "%u" (you can find more about it on the php documentation I linked above) with a combination of mysql_real_escape_string.

Although I'm sure it's not 100% safe, there is always a way to bypass checks.. One thing that comes to mind is maybe using hex values?

ninuhadida
+2  A: 

Always use parameterized queries. Always. There is no way to get the bad guy's code into your SQL if you don't put it into the source code of the query.

It may take some learning, but it is the only way to go.

Andy Lester
+1  A: 

Whatever you do end up using, make sure that you check your input hasn't already been mangled by magic_quotes or some other well-meaning rubbish, and if necessary, run it through stripslashes or whatever to sanitise it.

Rob
A: 

Maybe its not the best way, but another option would be GreenSQL

Jonathan
A: 

I favor stored procedures (mySQL has sp support since 5.0) from a security point of view - the advantages are -

  1. Most databases (including mySQL) enable user access to be restricted to executing stored procedures. The fine grained security access control is useful to prevent escalation of privileges attacks. This prevents compromised applications from being able to run SQL directly against the database.
  2. They abstract the raw SQL query from the application so less information of the db structure is available to the application. This makes it harder or people to understand the underlying structure of the database and design suitable attacks.
  3. They accept only parameters so the advantages of parameterized queries are there. of course - IMO you still need to sanitize your input - especially if you are using dynamic SQL inside the stored procedure.

The disadvantages are -

  1. They (stored procedures) are tough to maintain and tend to multiply very quickly. This makes managing them an issue.
  2. They are not very suitable for dynamic queries - if they are built to accept dynamic code as parameters then a lot of the advantages are negated.
Nikhil
A: 

You dont need to use the big prepare/execute stuff of PDO, I am using every where the code of Wordpress used for its DB: (Not sure its exactly this one...)

public function query($sql)
{
      if (!(func_num_args() == 1 && is_string($sql)))
      {

       $args = func_get_args();


       $sql = array_shift($args);

       $sql = str_replace("'%s'", '%s', $sql); // in case someone mistakenly already singlequoted it
       $sql = str_replace('"%s"', '%s', $sql); // doublequote unquoting

       array_walk($args, array(&$this, 'escape'));

       $sql = vsprintf($sql, $args);
      }

      echo "SQL: $sql";

      $result = DB::query($sql);

      return $result;
}

protected function escape(&$value)
     {
      if ($value === null) 
      { 
       $value = 'NULL'; 
      } 
      else if (is_bool($value)) 
      { 
       $value = $value ? 1 : 0; 
      }
      /*else if (is_date($values))
      {
       return strtotime($values);
      }*/
      else if ($value === "NOW()")
      {
       return "NOW()";
      }
      else if (!is_numeric($value)) 
      { 
       $value = mysql_real_escape_string($value); 
      }

      return $value; 
     }
Tom
this is a terrible one
Col. Shrapnel
Could you elaborate Col? I am using a very similar approach, though with cleaner api (imho) and I honestly don't see anything bad in it. This method is way more handy than prepared statements. It converts some code block with five lines to one single line which increases readability. Furthermore this effectively prevents you from saying "ah, this value is secure, I can insert it directly and save me the prepared statement", because it is so easy though effective to escape something. So, now please tell my why using a long code is better than using a short one?
nikic
@nikic it's about escape part. All these conditions are useless, and simple `$value = mysql_real_escape_string($value);` may replace them all. Save for ridiculous NOW() part, the only mysql function known to author, lol. And, may be, null part. Honestly, it's full of "dirty" considerations and one have to keep it all in mind. And, after all, **it does not add quotes** to strings, but only remove them, lol. It wasn't even tested before posting. I have a *similar* approach too, but way more usable and sensible one.
Col. Shrapnel
A: 

// connect to the mysql database

$unsafe_variable = mysql_real_escape_string($_POST["user-input"]);

mysql_query("INSERT INTO table (column) VALUES ('$unsafe_variable');

// disconnect from the mysql database

Yaya Jallow