4

I've below code in one of my php files to fetch data from DB:

$products = $this->db->get_rows('SELECT * from products WHERE shop_id='.$_SESSION['shop_id'].'AND tags,title,text LIKE \'%'.$_POST['search'].'%\'');

Is it problematic? I mean LIKE operator can be injected?

Edited

please provide examples of injecting in this way

revo
  • 47,783
  • 14
  • 74
  • 117
  • yes it can be injected. – Randy Mar 22 '13 at 21:12
  • 7
    in SQL **ANYTHING** can be injected. You are WIDE OPEN. Just because it's in the `like` section means nothing. there's nothing magical about like that makes it invulnerable. – Marc B Mar 22 '13 at 21:12
  • It is not the operator that is being taken advantage of during an SQL injection attack, it is the parameter. Use prepared statements and you remove this attacking option. – mechanical_meat Mar 22 '13 at 21:13
  • 2
    The 3rd most frequent question have the answer to how you really should query your database: http://stackoverflow.com/questions/60174/how-to-prevent-sql-injection-in-php – baloo Mar 22 '13 at 21:14
  • @MarcB $_POST variable is among to %. How it could be bypassed tho? – revo Mar 22 '13 at 21:17
  • 1
    @rev: `$_POST['search'] = '%\'; DROP TABLE students; --'`. the same way ANY sql injection occurs. Remember: the db cannot possibly see that you've inserted some data from $_POST. it just sees an sql statement. if that statement is properly formed, it'll execute. RTLM: http://bobby-tables.com – Marc B Mar 22 '13 at 21:19

4 Answers4

12

Any operator can be injected without binding.

$_POST['search'] = "1%'; DROP TABLE myTable LIKE '%";

Would make

.... AND tags,title,text LIKE '%1%'; DROP TABLE myTable LIKE '%%'

Read on how to bind parameters.

Kermit
  • 33,827
  • 13
  • 85
  • 121
2

Of course this can be injected, you need to sanitize your input. Right now you are taking raw post data and inserting it into your SQL statement.

You should run your POST data through some sort of data sanitization, something like mysql_real_escape_string or the like

Or at least prepared statements. let server side code do the work for you.

75inchpianist
  • 4,112
  • 1
  • 21
  • 39
1

Never, ever, use database queries like that, don't construct a string with variables and use it for database activities.

Construct a string that will later on be prepared and executed, by inserting the variables into the string, making them not act like "commands" but as "values".

You can do it like this:

$query = "SELECT * from products WHERE shop_id = :shopId;"; // An example, you can finish the rest on your own.

Now, you can prepare the statement (I recommend using PDO for this).

 $statement = $db->prepare($query); // Prepare the query.

Now you can execute variables into the prepared query:

$statement->execute(array(
    ':shopId' => $_SESSION['shop_id']
));

If you're inserting or updating, then you would have wanted to do:

$success = $statement->execute(array(
    ':shopId' => $_SESSION['shop_id']
));

which stores a boolean in $success, or you can fetch the values from a result if you're SELECTing:

$statement->execute(array(
    ':shopId' => $_SESSION['shop_id']
));
$result = $statement->fetch(PDO::FETCH_ASSOC);
if($result )
{
    // You can access $result['userId'] or other columns;
}

Note that you should actually make that be a function, and pass $shopId into the function, but not the session itself, and check if the session actually exists.

I recommend googling on how to use PDO, or take a look on one of my examples: How to write update query using some {$variable} with example

Community
  • 1
  • 1
Jonast92
  • 4,964
  • 1
  • 18
  • 32
1

This is really bad. Pulling vars into an SQL statement without cleaning or checking them is a good way to get pwnd. There are several things that people can inject into code. Another injection method to watch out for, 1=1 always returns true.

$products = $this->db->get_rows('SELECT * from products WHERE shop_id='.$_SESSION['shop_id'].'AND tags,title,text LIKE \'%'.$_POST['search'].'%\'');

//This example expects no result from the table initially so we would blind attack the DB to pull the admin record.
$_POST['search'] = "-1\'; union all select * from users limit 1;";

Someone call pull up the top account in the database (like the admin).

$user_id = $this->db->get_rows('SELECT * from users WHERE email="'.$_POST['email'].'" and password="'.$_POST['password'].'"');

//This always returns true so now I'm the admin again
$_POST['password'] = "x\' or 1=1 limit 1";

You also want to be careful what you print on screen.

$user_id = $this->db->get_rows('SELECT * from users WHERE email="'.$_POST['email'].'" and password="'.$_POST['password'].'"');

A message that you echo that says "No user name exists for $_POST['email']" could be replaced with something else.

$_POST['email']=";

$fp = fopen('index.php', 'w');
fwrite($fp, \"header('Location: http://badwebsite.com;');\"; 
fclose($fp);";

index.php could now people to a different website entirely where an infected page exists or an infected page on the site.

If you're checking IDs do something like:

if(preg_match('!^[0-9]$!',$_POST['id'])){
    $id = $_POST['id'];
} else {
  //flush
}

or count for the number of possible records... if you're only expecting one and you get all of the records in the DB then it's an injection attempt.

 if(is_numeric($_POST['id'])){
    $id = $_POST['id'];
    $count = mysql_result(mysql_query("select count(*) from users where id='$id''),0);
 }
AbsoluteƵERØ
  • 7,816
  • 2
  • 24
  • 35