10
\$\begingroup\$

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?

\$\endgroup\$
10
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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... \$\endgroup\$ – user22886 Apr 19 '14 at 18:19
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – user22886 Apr 19 '14 at 18:17
4
\$\begingroup\$

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:

https://stackoverflow.com/a/15671366

\$\endgroup\$
2
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ It is parameterized? \$\endgroup\$ – user22886 Apr 21 '14 at 17:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy