Having hard time solving sql injection php

Question

The following code I used for a image thumbnail when clicked it gets executed by taking the it "ID " from the database.

echo '<a class="thumbnail" href="view.php?id='.$row['id'] .'"">';

The code below actuality handle the GET variable passed through the above code.

<?php
require '../header.php';
if (isset($_GET['id']))
{
require '../../functions/function_db.php';
$id =mysql_real_escape_string (htmlentities($_GET['id']));
$sql = "SELECT * FROM `site_products` WHERE `id` = $id LIMIT 1";
$result = mysql_query($sql);
while ($row = mysql_fetch_assoc($result)) 
{
$product_name = $row['product_name'];
$price = $row['final_price'];
$desc = $row['short_description'];

}
}
?>

In spite of using mysql_real_escape_string the URL becomes SQL injection vulnerable in following scenario .

http://localhost/cart/pages/men/view.php?id=1'
http://localhost/cart/pages/men/view.php?id=1 orderby 1

and the webpage gives following mysql error.

 Warning: mysql_fetch_assoc() expects parameter 1 to be resource, boolean given

How to solve this ???

Don't use [`mysql_*`](http://www.php.net/manual/en/function.mysql-query.php) functions, they are deprecated as of PHP 5.5.0. Use [mysqli](http://www.php.net/manual/en/function.mysqli-query.php) or [PDO](http://www.php.net/manual/en/function.pdo-prepare.php) instead. Using prepared statements correctly removes (Or minimalizes at least) the chance of SQL Injections. — MisterBla, Aug 31 '13 at 17:43
Personally I prefer PDO, I prepare statements like this: `$statement = $db->prepare("SELECT * FROM site_products WHERE id = :id LIMIT 1");` (assuming `$db` is my PDO connection) and then bind it later like so: `$statement->bindParam(":id", $id, PDO::PARAM_INT);` — MisterBla, Aug 31 '13 at 17:46
Duplicate of [How can I prevent SQL injection in PHP?](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php). — Jason McCreary, Aug 31 '13 at 18:03
possible duplicate of [mysql\_fetch\_array() expects parameter 1 to be resource, boolean given in select](http://stackoverflow.com/questions/2973202/mysql-fetch-array-expects-parameter-1-to-be-resource-boolean-given-in-select) — John Conde, Sep 03 '13 at 21:59

Raymond Nijland · Accepted Answer · 2013-08-31T18:15:59.537

0

Updated because off an comment from Jason McCreary prepared statements are more safe but always force an type when you bind the values. But you still need to watch out for second order SQL injections they are still possible then even if you make use off prepared statements

Id should be an integer just cast it to an int and filter out NULL bytes, NULL bytes are also evil things

$id = (int)(str_replace("\0", "", $_GET['id']));

edited Aug 31 '13 at 18:15

answered Aug 31 '13 at 17:51

Raymond Nijland

11,488
2
22
34

Yeah that just worked thanks a lot man but can jst show me how to filter out null bytes perhaps an example will be gr8 thanks again – REX Aug 31 '13 at 17:55
"\0" is the NULL byte so you have already an example what will remove them – Raymond Nijland Aug 31 '13 at 17:57
Thanks I better take some more wiki search now – REX Aug 31 '13 at 18:00
This code makes no sense and *null bytes* are only one type of vulnerability. For all future readers, **do not** use this. Please take the time to [read real answers](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php?rq=1). – Jason McCreary Aug 31 '13 at 18:00
@Jason McCreary true prepared statements isnt the silver bullet also. second order SQL injection is still possible with prepared statements en null byte are an serious treat in webapplications and i always remove them before working with strings – Raymond Nijland Aug 31 '13 at 18:17
I am not denying null bytes are a security concern. My point is they are not the only concern and your code does nothing more than `(int)$_GET['id']`. Future readers should be aware of the other vulnerabilities and write better code than you've provided. – Jason McCreary Sep 01 '13 at 01:06
@Jason McCreary true but my code wont be an security hole if there no charset defined.. PDO defualts to emulate prepard statements for MySQL clients so it make uses off the function mysqli_real_escape_string to protect against SQL injections and this function is not safe without an charset defined. but iam going to stop this discussion because i know what iam doing and you know what you are doing so it would be pointless to continue this discussion about mine answer.. – Raymond Nijland Sep 01 '13 at 23:27

score 0 · Answer 2 · edited May 23 '17 at 12:05

0

$id =mysql_real_escape_string (htmlentities($_GET['id']));
$sql = "SELECT * FROM `site_products` WHERE `id` = $id LIMIT 1";

This is a common misuse of mysql_real_escape_string(). The function is only for escaping single quoted strings for MySQL queries. Single quotes (apostrophes) should always go around the return value. And why htmlentities() here?

Cast the value to an integer instead (e.g. $id = (int)$_GET['id'];, having the effect of keeping only digits 0-9), or put single quotes around the escaped value, or better yet, switch to mysqli or PDO prepared statements.

See also: How can I prevent SQL injection in PHP?

edited May 23 '17 at 12:05

Community

1
1

answered Aug 31 '13 at 17:54

PleaseStand

31,641
6
68
95

Thnks I'll give mysqli or PDO a try . – REX Aug 31 '13 at 17:59
prepare statements are known to cause an extra overhead when you execute an query only once.. security comes with an cost – Raymond Nijland Aug 31 '13 at 17:59

Having hard time solving sql injection php

2 Answers2