CODEWITHSUNDEEP
phpsqlsecuritysql-injection
user1204121
user1204121

Reputation: 385

SQL Injection - is this query secure?

I have a a page that appends different parameters to the URL that are used for the query.

For example

http://www.example.com/search.php?category=Schools&country[]=Belgium&country[]=Czech+Republic

My code is like this

if(isset($_GET['country'])){
$cties = "'" . implode("','", $_GET['country']) . "'";
}
else {
$cties = "'Albania','Andorra','Austria','Belarus','Belgium','Bosnia & Herzegovina','Bulgaria','Croatia','Czech Republic','Denmark','Estonia','Faroe Islands','Finland','France','Germany','Gibraltar','Great Britain','Greece','Hungary','Iceland','Ireland','Isle of Man','Italy','Latvia','Liechtenstein','Lithuania','Luxembourg','Macedonia','Malta','Moldova','Monaco','Montenegro','Netherlands','Norway','Poland','Portugal','Serbia','Romania','San Marino','Slovakia','Slovenia','Spain','Sweden','Switzerland','Ukraine','United Kingdom'";           
}
if(isset($_GET['category'])){
$cat = $_GET['category'];
}
else{
$cat = " ";
}

try{
// create the Prepared Statement

$stmt = $con->prepare("SELECT * FROM MyTable 
WHERE MyDate >= DATE(NOW()) 
AND (Category=:cat or '' = :cat) 
AND Country IN ($cties)
ORDER BY MyDate ASC");
$stmt->bindValue(':cat', $cat, PDO::PARAM_STR);
$stmt->execute();

I was wondering if this query is secure and if not, what I am doing wrong. Thanks in advance!

I finally got it (thanks to Your Common Sense):

if(isset($_GET['country'])){
$arr = $_GET['country']; 
}
else {          
$arr = array('Albania','Andorra','Austria','Belarus','Belgium','Bosnia & Herzegovina','Bulgaria','Croatia','Czech Republic','Denmark','Estonia','Faroe Islands','Finland','France','Germany','Gibraltar','Great Britain','Greece','Hungary','Iceland','Ireland','Isle of Man','Italy','Latvia','Liechtenstein','Lithuania','Luxembourg','Macedonia','Malta','Moldova','Monaco','Montenegro','Netherlands','Norway','Poland','Portugal','Serbia','Romania','San Marino','Slovakia','Slovenia','Spain','Sweden','Switzerland','Ukraine','United Kingdom');
}
if(isset($_GET['category'])){
$cat = $_GET['category'];
}
else{
$cat = " ";
}
// create the Prepared Statement
$in  = str_repeat('?,', count($arr) - 1) . '?';

$sql = "SELECT * FROM MyTable WHERE MyDate >= DATE(NOW()) 
    AND Country IN ($in)
    AND (Category=? or '' = ?) 
    ORDER BY MyDate ASC";
$stmt  = $con->prepare($sql);
$arr[] = $cat;  // adding category to array
$arr[] = $cat;  // we need it twice here
// finally - execute
$stmt->execute($arr);

Upvotes: 1

Views: 174

Answers (2)

Your Common Sense
Your Common Sense

Reputation: 158009

Yeah, Now I see your problem. Well, PDO is not too convenient a library for such a task. So, first of all I'll show you how it can be done with my own library:

$sql = "SELECT * FROM MyTable WHERE MyDate >= CURDATE() 
          AND (Category=?s or '' = ?s) 
          AND Country IN (?a)
          ORDER BY MyDate ASC"
$data = $db->getAll($sql, $cat, $cat, $_GET['country']);

But I quite realize that you all so inclined to familiar methods. Well, let's elaborate with ugly PDO

First of all, what is the goal? The goal is

  1. to create the query that contains placeholders for all the data. I'll stick to positional placeholders as they are easier to implement.
  2. To create an array with all the variables that have to be bound to placeholders

It seems we need two placeholders for category and some unknown number fro cities. All right, this line will create a string of placeholders: 

$in   = str_repeat('?,', count($arr) - 1) . '?';

which we are going to insert into query.

// $arr is array with all the vars to bind. at the moment it contains cities only
$arr = $_GET['country']; 
// creating string of ?s
$in  = str_repeat('?,', count($arr) - 1) . '?';
// building query
$sql = "SELECT * FROM MyTable WHERE MyDate >= DATE(NOW()) 
              AND Country IN ($in)
              AND (Category=? or '' = ?) 
              ORDER BY MyDate ASC";
$stm  = $db->prepare($sql);
$arr[] = $_GET['category'];  // adding category to array
$arr[] = $_GET['category'];  // we need it twice here
// finally - execute
$stm->execute($arr);
$data = $stm->fetchAll();

Upvotes: 2

fracz
fracz

Reputation: 21278

No, the SQL code could be injected in the $_GET['country'] parameter. You don't escape it anywhere.

See PHP PDO: Can I bind an array to an IN() condition?

Upvotes: 0

Related Questions