Selecting a table with multiple ID on MySQL using PHP PDO

Question

I have made a MySQL script to query a table with a set of id using an IN clause and after some reading I found out that there are security issues with it like prone to SQL injections and the likes.

Are there other ways to write this code with a more secure way? Although I'm using PDO, is that really enough?

 public function GetAllCustomerFilter($search, $cat_id)
 { 
 $stmt = $this->db->prepare("SELECT * from users_cards WHERE cat_id IN ($cat_id) AND username LIKE :search ORDER BY id DESC LIMIT 60";
 $stmt->bindParam(":search", $search);
 if($stmt->execute()) {
 $products = $stmt->fetchAll(PDO::FETCH_ASSOC);
 }
 }

Data is received from an Ajax:

 loadProductsListPerCat = function(form,data_serve) {
 var data_fed = new Array();
 $.each(data_serve, function(i,obj)
 {
 data_fed[i] = obj.cat_id;
 });
 var raw = data_fed.join();
 var encoded = encodeURIComponent(raw);
 var formData = "data_serve="+encoded+"&"+ $(form).serialize(); 
 var requestURL = 'searchUsers';
 console.log(formData);
 $.ajax({
 url: requestURL,
 data: formData,
 crossDomain: true,
 dataType: 'json',
 cached: false,
 async: true,
 success: function(data) {
 console.log(data);
 },
 complete: function() {
 if(isAppend) {
 $('.btn-toggle-load-more-users').attr('disabled', false);
 }
 },
 error: function(XMLHttpRequest, textStatus, errorThrown) {
 }
 }); 
};

The variable data_serve is taken by another jQuery function on which it counts input elements that has data-on set to true:

var PCatVal = (function(){
 var cats = new Array();
 $("#main_cat").find(".option_").each(function(index) {
 $(this).removeClass("option_");
 $(this).addClass("ctrls option_"+index);
 $(".option_"+index).on("click", function(e){
 var opt = $(this).data('parentname');
 var opt_id = $(this).val();
 var data_serve = [];
 //array check if exists
 var idx = $.inArray(opt_id, cats);
 if (idx == -1) {
 cats.push(opt_id); 
 } else {
 cats.splice(idx, 1);
 }
 //loop the array so to encode to a json format
 $.each(cats, function(i,obj){
 data_serve.push({"cat_id": obj});
 });
 //Switch for the category buttons
 var swtch_var = $(this).attr('data-on');
 if(swtch_var == 0)
 {
 swtch_var = 1;
 }
 else
 {
 swtch_var = 0;
 }
 $(this).attr('data-on',swtch_var); 
loadUsers($('#frmSearchUsers'),'',data_serve);
 e.preventDefault();
 });
 });
});

This is how the $_GET parameters are received:

 public function searchUser() 
 {
 $cat_id = (isset($_GET['data_serve']) && !empty($_GET['data_serve'])) ? $_GET['data_serve'] : null;
$search_filter = (isset($_GET['search_filter']) && !empty($_GET['search_filter'])) ? $_GET['search_filter'] : null;
 $products = $this->model->GetAllCustomerFilter($search_filter, $cat_id);
 return $products;
 }

• mdfst13: We need to know how $cat_id is put together. Please add more context. My natural advice now would be to replace the $cat_id string with an array and work with that. It's possible that that should be done outside the current function. Or perhaps the current function should be rewritten not to use $cat_id or ...
• Kim Oliveros: hi @mdfst13 thru ajax wait I'll add it
• Steve: Please show the php code that calls this function - how is the $_POST data retrieved and then fed to the function?
• Kim Oliveros: @Steve did I mentioned anything about $_POST? if so Im so sorry.. I'm using $_GET for this one right now

Answer 1

When I changed from the mysqli_functions to PDO with prepared statements, I had the same question. I found the answers to this question to be very usefull: https://stackoverflow.com/questions/134099/are-pdo-prepared-statements-sufficient-to-prevent-sql-injection.

Let's say you have messaging functionality and someone types SQL injection code in the title field when sending a message. You use a prepared statement, and the SQL injection code gets saved as the title. the SQL injection code didn't get executed, you have been saved from 1st order SQL injection.

Now let's say you have a function to reply to messages that uses the previous message title and adds "RE: " before it. If this reply function assumes the new title is safe since it's not coming from a user input field and doesn't use a prepared statement on the reply title, the SQL injection code will get executed. This is called 2nd order SQL injection.

You're 1st order safe if you use prepared statements for queries requiring user input. You're 2nd order safe if you use prepared statements for every query.

Answer 2

You're still vulnerable to SQL Injection. Take another look at loadProductsListPerCat. This function receives the data_serve parameter, sends its contents to the server which in turn puts it straight into an SQL query as the cat IDs:

$.each(data_serve, function(i,obj)
{
 data_fed[i] = obj.cat_id;
});
var raw = data_fed.join();
var encoded = encodeURIComponent(raw);
var formData = "data_serve="+encoded+"&"+ $(form).serialize(); 
var requestURL = 'searchUsers';
 $.ajax({
 url: requestURL,
 data: formData,
...

Remember that server code should never trust incoming data as safe, even if you wrote the front end (website) code.

If I wanted to inject SQL into your queries, I would go on your website and watch what legitimate HTTP requests look like. Once I know the URL and parameters sent by your AJAX call, I would craft my custom request but replace the valid cat IDs with SQL. I don't even need to use your website to make these requests to your server so good website code does not protect you.

Addendum

One simple fix: in GetAllCustomerFilter; if your cat_id DB field holds integers, use:

$ids_array = explode(',',$cat_id);
foreach ($ids_array as &$id): // "&" allows us to modify $ids_array in the loop
 $id = (int)$id; //cast as integer
endforeach;
$cat_id = implode(',',$ids_array);

Now you can safely put $cat_id in your query. Suppose an attacker sent in a dangerous parameter as the $cat_id such as 1,2,3,5) OR 1; DROP DATABASE myDb; The code below would transform these ids from the dangerous:

Array
(
 [0] => 1
 [1] => 2
 [2] => 3
 [3] => 5) OR 1; DROP DATABASE myDb;
)

Into the safe ids:

Array
(
 [0] => 1
 [1] => 2
 [2] => 3
 [3] => 5
)

• Kim Oliveros: I see.. so what can I do then to prevent this from happening?
• BeetleJuice: Expanded my answer

Answer 3

For situations where Patrick's example doesn't work because the thing you need many of aren't numbers:

$ids_array = explode(',',$cat_id);
$slots = [];
$bindings = [];
foreach ($ids_array as $id) {
 $slots[] = '?';
 $bindings[] = $id;
}
$slotString = implode(',', $slots);
$stmt = $this->db->prepare("SELECT * from users_cards WHERE cat_id IN ($slotString) AND username LIKE ? ORDER BY id DESC LIMIT 60";
foreach ($bindings as $index => $binding) {
 $stmt->bindParam($index, $binding);
}
$stmt->bindParam($index + 1, $search);
if($stmt->execute()) {
 $products = $stmt->fetchAll(PDO::FETCH_ASSOC);
}

This results in a $slotString that looks like ?,?,?, which will be bound to the ids (or potentially strings if you needed them to be) and therefore properly protected from injection.