CODEWITHSUNDEEP
phpmysqlicastingsql-injection
Doktor J
Doktor J

Reputation: 1118

Is casting user input to an integer sufficient to sanitize it?

Quoting from this SO answer:

Everything submitted is initially treated like a string, so forcing known-numeric data into being an integer or float makes sanitization fast and painless.

This was the sanitization method I had independently come up with for a quick and dirty query (looking up a name in a table from a numeric ID); the only variable being plugged into the query is the ID, and I know the ID should be greater than zero and less than 255, so my sanitization is as follows (with a little bit of validation thrown in too):

$id = (int)$_REQUEST['id'];
if ($id < 1 || $id > 255) errmsg("Invalid ID specified!");
$name = $DB->query("SELECT name FROM items WHERE id=${id}")->fetch_all()[0][0];

Is this sufficient to prevent SQL injection attacks or any other malicious attacks based on the user-specified value of $id, or can it still be exploited?

NOTE: the ID/name are not "sensitive", so if some input inadvertently casts to "1" or another valid ID value, I don't care. I just want to avoid antics along the lines of Little Bobby Tables.

Upvotes: 6

Views: 3729

Answers (8)

Machavity
Machavity

Reputation: 31654

The TL;DR answer is yes. When you cast to (int), you cannot get anything except an integer back.

The catch is you might have a use case where this can produce undesirable behavior. Let's take your code

$id = (int)$_REQUEST['id'];

Now, if we call this with

page.php?id=lolsqlinjection

Then the value of $id is 0 (because the string starts with a character, it will cast to 0 by default, see the PHP manual for various oddities with casting strings to integer). As such, any SQL injection is removed, making it safe from that attack vector. But you might have a use case where 0 is a special case, or another record. This is the reason prepared statements tend to be considered superior (showing MySQLi but you can do this with PDO as well)

$prep = $DB->prepare("SELECT name FROM items WHERE id=?");
$prep->bind_param('i', $_REQUEST['id']);
$prep->execute();

What this does is it tells your DB that you want the record that matches the input. As such, with my SQL injection, MySQL is now looking for an item with an integer id of "lolsqlinjection". No such record exists. Thus we avoid any potential edge cases where 0 would be a valid input.

Upvotes: 5

Bill Karwin
Bill Karwin

Reputation: 562661

it seems excessive to me to have to initialize, prepare, bind, execute, and fetch, all to get the value of a single column out of a single result.

Here's an example using PDO:

$stmt = $DB->prepare("SELECT name FROM items WHERE id=?");
$stmt->execute([$_REQUEST['id']);
$name = $stmt->fetchColumn();

Not so excessive, right? No need to initialize. No need to bind.

It's true that casting to an int is safe to prevent SQL injection. But I prefer to use query parameters all the time, so my code is consistent. I don't need to think about whether the parameter I'm searching for is actually an integer. I don't have to think about whether casting to (int) is safe for SQL injection.

Just use parameters.

It's safe for any data type.

It's easy to write the code.

It's easy for people to read your code.

Upvotes: 4

Your Common Sense
Your Common Sense

Reputation: 157900

Technically - yes.

It doesn't mean, though, that you should have any idea to use it anywhere in production code.

Instead of the non-optimal code you posted here, try to improve it. For example, even a primitive database wrapper could make your code this

$name = $DB->run("SELECT name FROM items WHERE id=?", [$_REQUEST['id']])->fetchColumn();

which would be industry-standard safe, quick and dirty enough to fit your taste, and also flexible enough to be used with any other query or return format.

Upvotes: -1

user2314737
user2314737

Reputation: 29387

Yes, casting to an integer will sanitize your user input but it's an overkill and can lead to errors if you consider PHP's type juggling (all strings are translated to 0, but you can have also unexpected results if the input is a decimal number, e.g. (int) ((0.1+0.7) * 10); yields 7).

The Hitchhiker's Guide to SQL Injection prevention recommends to sanitize numbers by

  • using prepared statements

or

  • formatting them to contain only numbers, a decimal delimiter and a sign

Upvotes: 1

apokryfos
apokryfos

Reputation: 40680

Your use case seems to be a step further form a simple "make sure this is an integer" test. I suggest you use the PHP tool for the job, filter_input():

filter_input(INPUT_GET, 'id', FILTER_VALIDATE_INT, [ 
     "options" => [
        "min_range" => 1,
        "max_range" => 255
    ]    
]);

This will save you an if (and an isset check) but at the cost of if it fails you don't know if it failed because it wasn't set or it wasn't in range.

As far as SQL injection is concerned then this should be sufficient.

Upvotes: 0

azjezz
azjezz

Reputation: 3987

In this case, this cannot be exploited since you are casting the the value of the request id to int, you are always going to get an integer, even if somephrase is sent, casting it to int will result 0 so this cannot be exploited.

However, using prepared statements is better, (not safer - both methods are safe), and the reason is to get used to it, so you don't need to cast or sanitize any given variable, by running a prepared statement you are sure that the values are being sanitized by the database driver and everything is safe. Again, this method of casting variables to int cannot be exploited.

Here's an example of how to validate the input in your case : 

<?php


$id = (int) $_GET['id'];

if($id === 0 || !in_array($id,range(1,255)) 
{
   if($id === 0 && (string) $_GET['id'] !== '0') {
      // sql injection attempt ! ( or not ? )
   } else {
      // maybe an error  
   }
} else {
  $result = $DB->query(...);
  echo $result;
}

Upvotes: 1

Darko Miletic
Darko Miletic

Reputation: 1356

You could do it like that but PHP already offers a facility for input param validation. Take a look at http://php.net/manual/en/filter.filters.php .

Here is an example:

$options = [
    'options' => [
        'default'   => null,
        'min_range' => 1,
        'max_range' => 255,
    ]
];

if (isset($_REQUEST['id'])) {
    $id = filter_var($_REQUEST['id'], FILTER_VALIDATE_INT, $options);
    if ($id) {
        // TODO:
    }
}

Even with all this filtering you MUST use SQL parameter binding for whatever library you use to access database.

Upvotes: -1

Marco
Marco

Reputation: 7287

Yes, casting the input to integer is sufficient to prevent against SQL injection attacks.

You should still validate the range of the result, as you did.

Upvotes: 0

Related Questions