ansaurus

Question

quote marks in sql causing problems

Answer 1

+1 A:

Try to use mysql_real_escape_string();

Alexander.Plutov 2010-09-28 16:12:46

Answer 2

+2 A:

You must use the function mysql_real_escape_string on all your user inputs before you put them in the database.

For example:

change $_REQUEST['nickname'] to mysql_real_escape_string($_REQUEST['nickname'])

Alternatively use prepared statements.

codaddict 2010-09-28 16:13:32

Not "you can", "you absolutely must".

delnan 2010-09-28 16:15:52

stupid "user input" again. as though no need to escape quotes in admin input. and stupid "user input" instead of strings. use this function on unquoted number and eat your injection.

Col. Shrapnel 2010-09-28 16:41:56

here you can see a fresh example of the consequences of such stupid assumptions: http://stackoverflow.com/questions/3814770/quote-marks-in-sql-causing-problems/3814887#3814887 SQL injection at it's best.

Col. Shrapnel 2010-09-28 16:45:28

@delnan: No, you needn't use it on any and all of a user's input. If you validate the input before you run a query there is absolutely no risk of an injection. @Col: User input includes input by an administrator. It has nothing to do with your rights on a page.

nikic 2010-09-28 17:01:56

@nikic go tell it to all these noobs. And admin input is just a figure. It can be script generated data or anything. Why to mention stupid user input at all, while just every friggin string in the query must be escaped, no matter of it's origin? As for your "no need", **validation rules may change**. Data validation has absolutely nothing to do with database level.

Col. Shrapnel 2010-09-28 17:09:53

@nikic: Yeah, it's not necessary if the input is validated properly. But it's obviously safer to escape unnecessarily than not to escape even though you screwed up validation (or didn't validate at all). So as a rule of thumb, one should indeed escape everything.

delnan 2010-09-28 17:10:12

@Col: No, you are mistaken. You don't *need* to escape all data passed into the query. If you can be infallibly sure that the string may not be malicious (e.g. because it's `ctype_digit`) then you don't *need* to escape it - but obviously you can. I do know that to err is human and thus it may be inadvisable to rely on your assumption being right. This is why I use an automated quoting system which combines security with convenience. But before I did so I escaped only what required escaping - and I don't see why this is wrong.

nikic 2010-09-28 17:18:45

@nikic: Know what is written on the some fellow electrician's grave? "I was sure that power was off".

Col. Shrapnel 2010-09-28 17:24:08

Answer 3

+9 A:

Your code is filled with sql injection opportunities. Look into PDO with prepared statements. Or atleast mysql_real_escape_string. Do not use your code.

Also...

Don't use $_REQUEST. Use the appropriate global $_POST or $_GET. GET requests should not change anything so you should be using $_POST.

Galen 2010-09-28 16:14:14

+1 for throwing code into the trash can! :)

Frankie 2010-09-28 16:21:27

Answer 4

A:

Everyplace where you use a value that can have quotes use the addslashes($mystringwithquotes) method to add slashes before the quotes.

Make sure to not put this around the complete SQL string as you still want to keep the ones you need.

For example in your code:

    $sql = "INSERT INTO `tims`.`pending_profile`" 
                . "(`id`, `nickname`, `location`, `role`, `yog`, `interests`, `favMoment`, `gainThisYr`, `futurePlans`, `bio`) \n" 
                . "VALUES ('" . $_SESSION['id'] . "', '" . $_REQUEST['nickname'] . "', '" . $_REQUEST['town'] . "', '" . $_REQUEST['role'] . "', '" . $_REQUEST['yog'] . "', '" . $_REQUEST['interests'] . "', '" . $_REQUEST['fav_moment'] . "', '" . $_REQUEST['gain'] . "', '" . $_REQUEST['future'] . "', '" . $_REQUEST['bio'] . "')\n" 
                . "ON DUPLICATE KEY UPDATE nickname ='" . $_REQUEST['nickname'] . "', location='" . $_REQUEST['town'] . "', role= '" . $_REQUEST['role'] . "', yog='" . $_REQUEST['yog'] . "', interests='" . $_REQUEST['interests'] . "', favMoment='" . $_REQUEST['fav_moment'] . "', gainThisYr='" . $_REQUEST['gain'] . "', futurePlans='" .       $_REQUEST['future'] . "', bio='" . $_REQUEST['bio'] . "'\n";   
        $qry = mysql_query($sql) or die(mysql_error());

You want to put it around the values addslashes($_SESSION['id']), addslashes($_SESSION['nickname']), etc. but NOT around

   $sql = "INSERT INTO `tims`.`pending_profile`"

mrjohn 2010-09-28 16:15:30

Please don't recommend using addslashes for SQL injection defense. It's not the right solution because it doesn't account for multibyte character sets. Use the escaping function provided by the database API: e.g. mysql_real_escape_string.

Bill Karwin 2010-09-28 16:28:11

Answer 5

+1 A:

You're going to get beat up for not sanitizing the variables from your form before inserting them into your DB.

The PHP Manual has examples for filtering/sanitizing here....

http://php.net/manual/en/filter.filters.sanitize.php

Another popular approach since you're using php and mysql is something like this...

$var = mysql_real_escape_string($_POST['var']);

More info here:

http://php.net/manual/en/function.mysql-real-escape-string.php

Hope that helps.

Don 2010-09-28 16:15:38

Answer 6

+3 A:

Always use parameterized queries when accessing a database. Constructing dynamic SQL statements as you're doing is an invitation to SQL injection attacks (as well as the errors that you're seeing.)

In PHP, the code to do this looks like

$mysqli = new mysqli('localhost', 'my_user', 'my_password', 'world');
$stmt = $mysqli->prepare("INSERT INTO CountryLanguage VALUES (?, ?, ?, ?)");
$stmt->bind_param('sssd', $code, $language, $official, $percent);

(shamelessly taken from the PHP manual).

ngroot 2010-09-28 16:15:51

+1 But I'll add that using parameters is a lot easier in PDO. I find the mysqli API harder to use.

Bill Karwin 2010-09-28 16:31:12

Answer 7

+1 A:

You can also use (int) if $_POST variable is has to be number.

hey 2010-09-28 16:16:37

Answer 8

A:

Using traditional "plain" (not parametrized) queries you have to follow 3 rules

all strings (quoted portions of data) must be escaped
all numbers must be cast to its type explicitly
all identifiers or operators should be chosen from previously defined variants.

you can also use some helper function to assemble your query automatically.

function dbSet($fields,&$source) {
  $set='';
  foreach ($fields as $field) {
    if (isset($source[$field])) {
      $set.="`$field`='".mysql_real_escape_string($source[$field])."', ";
    }
  }
  return substr($set, 0, -2); 
}

but your code is tooo dirty to make a working example out of it.
Here is dummy one

$fields = explode(" ","name surname lastname address zip fax phone");
$query  = "INSERT INTO $table SET ".dbSet($fields, $_POST);

Col. Shrapnel 2010-09-28 16:34:03

Answer 9

+2 A:

As other people have mentioned, you need to handle literal quote characters in your strings or else you're susceptible to SQL injection attacks.

See my presentation SQL Injection Myths and Fallacies for more examples and solutions.

Using query parameters is an effective defense and it's actually easier to write and makes your code look nicer. I recommend using PDO, because it allows you to use named parameters.

Also consider using PHP's heredoc syntax to make it simpler to write SQL code, without worrying about all the double-quotes and concatenating strings.

$sql = <<<END_SQL
INSERT INTO `tims`.`pending_profile`
(`id`, `nickname`, `location`, `role`, `yog`, `interests`, `favMoment`,
 `gainThisYr`, `futurePlans`, `bio`)
VALUES (:id, :nickname, :town, :role:, :yog, :interests, :fav_moment, 
 :gain, :future, :bio)
ON DUPLICATE KEY UPDATE
  nickname    = :nickname,
  location    = :town,
  role        = :role,
  yog         = :yog,
  interests   = :interests,
  favMoment   = :fav_moment,
  gainThisYr  = :gain,
  futurePlans = :future,
  bio         = :bio
END_SQL;

$stmt = $pdo->prepare($sql);
$success = $stmt->execute(array(
    ':id'         => (int) $_SESSION['id'],
    ':nickname'   => $_POST['nickname'],
    ':town'       => $_POST['town'],
    ':role'       => $_POST['role'],
    ':yog'        => $_POST['yog'],
    ':interests'  => $_POST['interests'],
    ':fav_moment' => $_POST['fav_moment'],
    ':gain'       => $_POST['gain'],
    ':future'     => $_POST['future'],
    ':bio'        => $_POST['bio'],
));

note: I changed $_REQUEST to $_POST because you should never use a GET request that modifies your data. Search engines follow links, so you could find Google crawling your site and inadvertently modifying your database. Always use POST requests when the action is going to store or modify something.

For simple cases you don't even have to use parameters, if you can coerce your variable into something that is safe to use, like a plain integer:

$id = (int) $_SESSION['id'];
$sql = "SELECT * FROM `pending_profile` WHERE id = {$id}";
$stmt = $pdo->query($sql);
$row = $stmt->fetch();

Aside from the SQL injection risk your code is also full of a different type of security risk, called Cross-Site Scripting, or XSS. This happens when you echo user-supplied content to the HTML output, without escaping characters that are special in HTML, like < or &. Attackers can cause mischief by entering their name including javascript code.

The fix for XSS is basically to use htmlentities() on anything you echo, if it might have come from user input or any other non-trusted source (like reading from a database or a file or a web service).

<td><input type="text" name="nickname" value="<?php echo htmlentities($row['nickname']); ?>"/><td>

You really need to study these security flaws if you write code for the web. They are a source of a lot of trouble on the internet. See OWASP or the SANS Top 25 Most Dangerous Software Errors.

Bill Karwin 2010-09-28 17:12:12

+1 but I find it incredible boring to type the same set of fields **six** times. Not to mention a case what one field being added to the table... There ought to be some helper function to make programmer's job a bit more intellectual.

Col. Shrapnel 2010-09-28 17:20:17

@Col Shrapnel: If you use positional parameters with `?` placeholders, you'd only have to type the names three times instead of six. But in this example of INSERT...ON DUPLICATE KEY UPDATE, you'd have to pass each value twice (one can use `array_merge()` to help with that).

Bill Karwin 2010-09-28 17:42:55

ansaurus

tags:

views:

answers:

quote marks in sql causing problems

related questions