current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 7 down vote favorite

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
add comment

4 Answers

up vote 8 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 yesterday
add comment
up vote 7 down vote

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 yesterday
add comment
up vote 4 down vote

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
add comment
up vote 2 down vote

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
add comment

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.