6

So I was wondering is this enough to be safe that user won't do any SQL injections and the number will be only and always integer? The $id in getArticle function is binded to SQL query.

<?php $id = (isset($_GET['id']) && is_int((int)$_GET['id'])) ? (int)$_GET['id'] : false ?>
<?php $news = $class->getArticle($id) ?>

As far I tested it worked fine, but as I'm not totally sure I rather ask you guyz! Ok, people say prepared statements would do the trick. They really would? Like, can I be totally sure that if bind param as integer it will be integer nothing else?

Thanks in advance!

Rihards
  • 10,241
  • 14
  • 58
  • 78
  • 2
    I think testing for `is_int((int)[ANYTHING])` will always return `true` and is therefore redundant. Use `is_numeric` to test if something **can be converted to** an `int` and then use `(int)` to carry out the conversion. Or leave the first step out, as (int) will create an `int` of 0 on unrecognizable values. – MvanGeest Jun 30 '10 at 14:50
  • beware if your $_GET parameter comes from a form | bool is_int ( mixed $var ) | Finds whether the type of the given variable is integer. Note: To test if a variable is a number or a numeric string (such as form input, which is always a string), you must use is_numeric(). – Redlab Jun 30 '10 at 15:43

4 Answers4

10

You can simply type cast them to proper type:

$number = intval($_GET['id']);
$string = mysql_real_escape_string(strval($_GET['str']));

To make sure that you get what you are expecting.

The better solution is to use Prepared statements to avoid sql injection.

Sarfraz
  • 377,238
  • 77
  • 533
  • 578
  • 7
    Agree, why not just use *Prepared statements* and forget about parameter checking. – Ivan Nevostruev Jun 30 '10 at 14:53
  • Yes, as I said I later bind the params to sql query, but does it really will do what it has to do? I mean: bind_param('i', $id); will do the trick? – Rihards Jun 30 '10 at 14:59
  • 1
    *"The better solution is to use [Prepared statements](http://php.net/manual/en/function.mysql-real-escape-string.php) to avoid sql injection."* - [`mysql_real_escape_string()`](http://php.net/manual/en/function.mysql-real-escape-string.php) is not considered as a "prepared statement". This http://php.net/manual/en/mysqli.prepare.php and http://php.net/manual/en/pdo.prepared-statements.php are. – Funk Forty Niner May 11 '17 at 21:14
  • 1
    [Escaping the string](http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string) is not safe! – Jay Blanchard May 11 '17 at 21:16
5

Use prepared statements. There is no reason NOT to use them. Then you don't have to ask "Is this good enough?"

Andy Lester
  • 91,102
  • 13
  • 100
  • 152
  • If the input is cast to a number, then I know it is "good enough". And if its easier to skip prepared statements (building queries with WHERE IN) or more performant (doing a massive batch of insert in one query) then there are reasons not to use them. – Rick Jolly Mar 06 '16 at 17:05
0

I can't think of any way how this can be used for an SQL-Injection. So I would say it's secure enough.

jigfox
  • 18,057
  • 3
  • 60
  • 73
0

just use:

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

if $_GET['id'] is not set $id will be 0.
if you want to test if id is correctly set use:

if ($id=(int)@$_GET['id']){
  //
} else {
  //invalid id
}
codez
  • 1,381
  • 1
  • 18
  • 28