quotes in queries

Question

I have a mysql query which requires that parameters be enclosed in either "" or '',

if I have an array passed to this function:

function orderbyfield($column, array $selection)
{
 // will it be alright (secure) to do this?
 foreach ($selection as $s)
 {
  $s = '"' . $s . '"';
 }
 $string = implode(',', $selection)
 return array($column, $string);
}

and pass it to

function generate_sql()
{
 $fields = $this->orderbyfield(); // assuming the code is in a class
 $sql = 'SELECT FIELDS FROM TABLE ORDER BY FIELD (' . $fields[0] . ',' . mysql_real_escape_string($fields[1]));
}

will there be any security issues with this approach?

EDIT assume that code is in a class, made necessary addition of $this->

EDIT typo on the foreach

Answer 1

Because you are using the mysql_real_escape_string function, it is pretty safe as far as strings are concerned. See dealing with sql injection for more info.

Answer 2

You should add quotes arround your string, but there quotes inside your strings themselves should also be escaped -- this can be done using mysql_real_escape_string, mysqli_real_escape_string, or PDO::quote, depending on the kind of functions/methods you are using to connect to your database.

Doing this (As you are already doing -- which is nice) should prevent SQL injections (at least for string : you should also check that numerics are indeed corresponding to numerical data, for instance)

Another solution, maybe a bit easier once you get it, would be to use Prepared statements.
See :

• PDO::prepare
• or mysqli_prepare
• (Those can't be used with the old mysql_* functions)

Answer 3

As others have said you should be using mysql_real_escape_string at the point where you create the query string. Also, although the database may be able to cast between types, not all the variables need to be quoted in queries:

function enclose($val, $dbh)
{
  if (($val==='') || (is_null($val))) {
       return 'NULL';
  }
  // is it a number?
  if (preg_match('/^[\+-]*\d+\.?\d*$/', $val)) {
      return($val);
  }
  // its a string
  return("'" . mysql_real_escape_string($val, $dbh) . "'");
}

The null handling might need to be tweaked. (the above is cut down from a generic interface I use which also reads the structure of the table using DESCRIBE to get hints on when to quote/use nulls etc)

C.

Answer 4

If you're using PDO's prepared statements, you do not have to worry about the escaping yourself. No quotes, no backslashes, no nothing.