0

Is prepared statement necessary when dealing with trusted data fetched from another query?

For example.
When a user is navigating throughout the site, they click named links like this: /?category=health where health is the value that is sent to the database.
In this scenario I of course use prepared statement like this: 

$qry = $dbh->prepare('SELECT category_id, and, other, columns FROM categories WHERE query_value = ?');
$qry->execute([$_GET['category']]);
$get = $qry->fetch();
$qry = null;

But further down the script, I would display content associated with the selected category based on the categories.category_id fetched from the last query. 

$Banners = $dbh->query('SELECT image FROM Banners WHERE category_id = '.$get['category_id'])->fetchAll();

I would like to think that this is a secure query.
That the value could be no other than a trusted value since it has to be a result from the previous query?
And this query won't be executed if the previous query doesn't return true.

Here's how I've done it so far:
It's a 3-liner. But it would speed up the coding part a bit if I was certain that the 1-liner above is fine too.

$qry = $dbh->prepare('SELECT image FROM Banners WHERE category_id = ?');
$qry->execute([$get['category_id']]);
$Banners = $qry->fetchAll();
ThomasK
  • 2,210
  • 3
  • 26
  • 35
  • If you really want to do it in one line, cast the variable to an int – Spoody Aug 22 '18 at 06:55
  • 4
    *is it really necessary?* As far as I know, no. **But**. Just do it. It is two more lines of code and it makes your codebase consistent and safer. Maybe in 9 months someone will have to change this query, make it dependent on user input and then you're wide open for injection attack. If you really think it's a waste of time to write two more lines of code, just use a database abstraction layer or write a wrapper yourself that automatically prepares every statement. Just because it isn't necessary now, doesn't mean breaking up consistency or introducing future vulnerabilities is a good thing. – Loek Aug 22 '18 at 06:58
  • ThomasK, I think this is not really a question that can be answered here since it's gonna be mainly opinion based. That being said: Prepared statements not only prevent SQL injections but make the searches a lot faster because they are cached into MySql. – Rafael Aug 22 '18 at 06:59
  • 1
    @Rafael Mysql does not cache prepared statements between sessions (i.e. HTTP requests). The only speed benefit is if you're executing the exact same query two times in the same script with different variables. Otherwise it is actually slower than putting the variables straight in the query. However I still agree with Loek and would advocate for always binding your variables, even if they come from (currently) trusted sources. – Mike Aug 22 '18 at 07:05
  • On another note, you may want to consider using a JOIN to get the categories and banners in one single query. – Mike Aug 22 '18 at 07:07
  • @Mike Join's are of coure a way to go if there was only one banner image associated. But when there's multiple, the a JOIN is not possible? – ThomasK Aug 22 '18 at 07:09
  • @ThomasK No, the number of rows makes no difference. Give it a try. – Mike Aug 22 '18 at 07:11
  • @Loek All good points. Some of I didn't concider in my scenario. Writing 1 or 3 lines of code is not a problem. It was more a question of the necessity regaring security reasons. – ThomasK Aug 22 '18 at 07:14
  • @ThomasK it all depends on the circumstances of your project and software, but 999 out 1000 times opening a whole can of unforseeable risks is just not worth the 5 seconds you save typing ;-) – Loek Aug 22 '18 at 07:16
  • Possible duplicate of https://stackoverflow.com/questions/24988867/when-should-i-use-prepared-statements – Mike Aug 22 '18 at 07:19
  • If you're really worried about the amount of lines, you could do it in 1 line with native PDO (as martinstoeckli's answer states), but keep in mind you could also use one of the [dozens of PDO wrappers](https://www.google.com/search?q=pdo+wrapper&rlz=1C1GCEA_enUS745US745&oq=pdo+wrapper&aqs=chrome.0.0j69i61j0l4.959j0j4&sourceid=chrome&ie=UTF-8) out there, like my wrapper [GrumpyPDO](https://github.com/GrumpyCrouton/GrumpyPDO) where your query would be done like this: `$db->all('SELECT image FROM Banners WHERE category_id = ?', [$_GET['category']);` – GrumpyCrouton Aug 22 '18 at 16:46

1 Answers1

2

What this all is about largely is about ensuring your SQL syntax is sane and what you think it is. Preventing malicious attacks is just a corollary of that. What if your category ids in the future spawn apostrophes or whatnot as part of the regular value? You need escaping for correct syntax then anyway, or, better, prepared statements.

Secondarily, there is second order injection, in which a value which was previously treated as unsafe is now suddenly treated as safe, even though it still has the potential for attacks. E.g. if your values legitimately contain apostrophes.

More broadly speaking: how do you ensure a value is "safe"?

// $get['category_id'] is safe and doesn't need escaping
'SELECT image FROM Banners WHERE category_id = '.$get['category_id']

Well, how do you know this? What code ensures this value is safe? Where does this value come from? Are you sure that's where it comes from? You're outsourcing the integrity of this query to some other part of the code here. How can you be sure that other part does its job correctly? Now, or in the future, after a lot of refactoring? Simply ensure yourself that you are producing correct queries by writing queries in a secure way, don't outsource your security.

You need to ask yourself: is this query prone to injection the way it is written? And the answer to the above example is a plain yes, it is prone to injection, and the safety merely depends on your trust that $get['category_id'] is what you think it is.

deceze
  • 510,633
  • 85
  • 743
  • 889