Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've this function to list products from database by category. I'm using prepared statements but wouldn't this make it pretty pointless to use them?

function getProducts($category,$search_param,$pdo){
      $query = "SELECT * FROM renkaat 
        INNER JOIN ajoneuvotyypit ON tyyppiID = ajoneuvotyyppiID 
        INNER JOIN vuodenajat ON vuodenaikaID = renkaat.vuodenaika
        INNER JOIN valmistajat ON renkaat.valmistaja = valmistajat.valmistajaID
        WHERE renkaat.$category= ?
        ";
    $smt = $pdo->prepare($query);
    $smt->execute(array($search_param));

      //do stuff
  }

And the function is simply called like this, with no escaping done.

getProducts($_GET['category'],$_GET['search_param'],$pdo);

Should I escape the $category?

share|improve this question

4 Answers 4

up vote 11 down vote accepted

Yes, $category in your query string, coming directly from $_GET is a problem.

You should validate $category, and that's not exactly the same as escaping. I assume you have a limited set of valid categories in your database, so validation should be something like:

$valid_categories = array("sector", "industry", "company");
if (!in_array($category, $valid_categories)) {
    // no no
}

As for the other part of your question, paraphrased:

Is it pointless to use prepared statements if there is a variable part in the query?

It's not pointless, because every time you run the query for the same $category value, it can benefit from the previously prepared statement.

share|improve this answer
    
I meant pointless from a security point of view, not performance, but thanks for the heads up! Luckily this little snippet was on the administration side and not the live site... –  Christian Apr 19 at 18:19

Yes, this code is vulnerable to SQL injection, just like any dynamically composed SQL that is not properly quoted. The quoting mechanism for identifiers is database dependent, though.

A bigger concern is how this situation came about in the first place. If you ever need to programmatically choose a column like that, your schema is probably poorly designed. There should be a small number of columns in the schema, all with fixed names. The fact that you have many columns, all with compatible types such that you can apply the same query over them, indicates that those attributes should all be upgraded to an Attribute entity.

share|improve this answer
    
Yeah, I wasn't the one creating this system in the first place. I'm just "adding features" here, but seems like that I've to go trough the whole code to check for these kinds of incidents. –  Christian Apr 19 at 18:17

Yes, it is a problem, the following will give you all data from all categories:

$category = 'validcategory="searchparam" OR 1 = 1 OR validcategory'

You should check if the $category exists first, maybe dynamic like this is better than defining an array in code:

http://stackoverflow.com/a/15671366

share|improve this answer

More than validating or quoting your queries you should parameterize them. Here's a different SO question which covers the topic.

share|improve this answer
    
It is parameterized? –  Christian Apr 21 at 17:08

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.