3

My question of to day is. Do i need to escape PDO in my script?

$columns = implode(", ",$column);
$query = ''.$query.' '.$columns.' FROM '.$table.'';
$dbh_query = $dbh->prepare($query);
$dbh_query->execute();
$dbh_querys = $dbh_query->fetchAll();

return $dbh_querys;

The whole script can be found at. https://github.com/joshuahiwat/crud/blob/master/control/query_connector.class.php

Can someone explain why do i need a escape at this time or why not.

I like to hear from you, thanks a lot!

Pieter Dijkstra
  • 121
  • 11
  • 3
    https://phpdelusions.net/pdo/sql_injection_example – zerkms Sep 01 '16 at 23:47
  • You should escape any data that isn't trusted. `$foo = "bar";` can be trusted because you're the one who set it (and it can't be overriden). `$_GET['foo']` can be anything, so don't trust it! I escape untrusted data, not everything. – Blake Sep 01 '16 at 23:49
  • http://stackoverflow.com/questions/3716373/real-escape-string-and-pdo – Andrej Sep 01 '16 at 23:50
  • Thanks for your comment, but do i need to escape here or, becasue PDO take most of the time care of those issues. – Pieter Dijkstra Sep 01 '16 at 23:57
  • 2
    The only thing PDO takes care of automatically are prepared statement parameters that are filled in with `bindParam/bindValue`. – Barmar Sep 02 '16 at 00:03
  • It won't make any changes to the rest of the query. – Barmar Sep 02 '16 at 00:03
  • 2
    "becasue PDO take most of the time care of those issues." --- you better don't treat it like that. PDO is just a tool. It does not make anything secure automagically. You still must understand how and what should be done. – zerkms Sep 02 '16 at 00:05
  • So i need to create before the excute a bindparam to escape? – Pieter Dijkstra Sep 02 '16 at 00:08
  • 2
    You need to read the article I referred to and understand the threats first. – zerkms Sep 02 '16 at 00:08
  • @zerkms thanks a lot! – Pieter Dijkstra Sep 02 '16 at 00:16
  • @zerkms i have tested you way to protect, but it isn't possible in my script, because i can't decide wich key the system will espect, it is and it we be dynamic. And when i test to inject my script, it will only return a array eror and no result. So i think i can only escape that thing with a bindparam? – Pieter Dijkstra Sep 02 '16 at 00:33
  • Any time you're putting something in a query it **must** be escaped or validated against a white list of acceptable values. There are no exceptions. – tadman Sep 02 '16 at 02:09
  • What answer you expect from us? You came here asking how to protect your query. Now you say you can't / don't want to do what you've been told. So what you expect? We'd say, "ok, don't do it, you're safe"? We won't. And of course it won't work for your code out of the box, each injection exploit have to be forged specifically. But the injection remains in place. – Your Common Sense Sep 02 '16 at 03:12
  • @YourCommonSense I was looking for the right answer, every answer is good but without any example of how i need to get the protection or what part of the code i need to protect. Some say bind, but other say execute. – Pieter Dijkstra Sep 02 '16 at 10:36
  • @PieterDijkstra bind end execute do exactly the same. But your problem is related to none of them. Most of your query cannot be protected neither by bind not execute – Your Common Sense Sep 02 '16 at 11:37

2 Answers2

3

The parts of your query that are dynamic are the table name and column names. You can't use bind functions for these parts of the query. Bind functions can be used only for the parts of the query that would otherwise be a simple value in an SQL query. Like a numeric constant, or a quoted string or quoted date literal.

To avoid SQL injection from dynamic table names or column names, you have the following choices:

  • Use values that are predefined in your class, or otherwise certain to be safe. Don't use external content from users or any other source.
  • Use escaping. Note that the function PDO::quote() doesn't do the kind of escaping you need for table names or column names.
  • Create a "allowlist" of known table names and the column names for the respective table, and compare the dynamic input to the allowlist. If it doesn't match the allowlist, raise an error.
Bill Karwin
  • 538,548
  • 86
  • 673
  • 828
2

First of all you need to understand that the word you are using - "escape" - is meaningless.

What you probably mean is "to make your query safe from SQL injection". But, unfortunately, there is no such magic "escaping" that will make some abstract query safe.

The traditional query building assumes that all the query parts beside data values are hard-coded, while data values are bound via placeholders, like this:

$query = 'SELECT col1, col2 FROM some_table WHERE id = ?';
$stmt = $dbh->prepare($query);
$stmt->execute([$id]);
$row = $stmt->fetch();

This kind of a query considered safe.

In your case of a dynamically constructed query, every part is potentially vulnerable.

And here it is very important to understand that a burden of sanitizing all the query parts is entirely on this function. You cannot dismiss the danger simply claiming that your data is coming from the trusted source. That's a slippery ground because people often have no idea whether their source is trusted or not.

So, if take your question as "Do I have to protect this code from SQL injection", than the answer is - YES, YOU HAVE.

In the meantime you are protecting only a small part of your query - the data values. So you still have to protect (this term is much better than "escape") all other parts.

On a side note, your code is connecting to database every time it runs a query, which is highly inefficient and makes it impossible to use some database features.

Community
  • 1
  • 1
Your Common Sense
  • 156,878
  • 40
  • 214
  • 345