Is this correct PDO and sufficient to prevent injection?

Question

Changing from php 5 to php 7 I must change my queries. Now using PDO.

Is this method sufficient to prevent mysql injection etc?

I connect to the db like this:

$conn = new PDO("mysql:host=$hostname;dbname=$db", $username, $password);

my full query is 8 queries joined by union all. My (simplified) query is like this: (there are other cols to be queried, hence the numerous union all)

$query="select * from product_catalogue
         where sub_category like ?
union all
        select * from product_catalogue
         where brand like ? 
union all
        select * from product_catalogue
         where category like ?
union all
        select * from product_catalogue
         where colour like ?
union all
        select * from product_catalogue
         where stock_code like ?
union all
        select * from product_catalogue
         where barcode like ?
union all
        select * from product_catalogue
         where item_name like ?
union all
        select * from product_catalogue
         where department like ?";   

$input = "%".$input."%";


$stmt = $conn->prepare($query);

 
$stmt->bindParam(1, $input, PDO::PARAM_STR, 12); 
$stmt->bindParam(2, $input, PDO::PARAM_STR, 12);
$stmt->bindParam(3, $input, PDO::PARAM_STR, 12);
$stmt->bindParam(4, $input, PDO::PARAM_STR, 12);
$stmt->bindParam(5, $input, PDO::PARAM_STR, 12);
$stmt->bindParam(6, $input, PDO::PARAM_STR, 12);
$stmt->bindParam(7, $input, PDO::PARAM_STR, 12);
$stmt->bindParam(8, $input, PDO::PARAM_STR, 12);

$stmt->execute();


$data = $stmt->fetchAll();

`$conn = new PDO("mysql:host=$hostname;dbname=$db", $username, $password);` Why are you putting your connection details into these variables? This is a needless risk as unless you specifically unset the variables afterwards, these details are now available across the entire script. If you're changing connections (ie to different databases) that's fine, but at the very least you should be unsetting these variables as soon as they've been used to make a successful connection. — Martin, Sep 24 '20 at 17:10
The `12` you are passing as the 4th parameter to `bindParam` is according to the manual: **Length of the data type. To indicate that a parameter is an OUTparameter from a stored procedure, you must explicitly set the length.** It does not pertain here. — Booboo, Sep 25 '20 at 10:52
$conn = null; I did have that at the end of this script. I use variables for the Db connection because I have one config file above root, that has the db credentials within it. One place to manage them across the domain, for all scripts. Do you think there's a potential risk with that? — headache, Sep 25 '20 at 14:46

O. Jones · Accepted Answer · 2020-09-25T12:34:53.017

1

Yes, this is OK for avoiding sql injection.

You would be wise to remove, or escape, all % and underscore characters from $input before using it for your query, though. Doing this will avoid confusion in your LIKE results.

edited Sep 25 '20 at 12:34

answered Sep 24 '20 at 16:23

O. Jones

103,626
17
118
172

Thank you; so str_replace? Substituting with '' or ' ' – headache Sep 24 '20 at 17:02
surely a REGEX qualifier to prepend these characters with `\ ` would be enough? `this 90%` `==>` `this 90\%` ? – Martin Sep 24 '20 at 17:08
1

I suppose that would depend on whether such chars are in the data. Not in my db so I think removal makes more sense. – headache Sep 24 '20 at 17:19
1

NO! If you want to select a column that contains the substring `'a_b'` and you want to use `like`, then you *must* escape the underscore so as to not retrieve spurious results: `like '%a\_b%'`. If you remove underscores, i.e. `like '%ab%'`, you will never get the results at all. – Booboo Sep 25 '20 at 11:03

Is this correct PDO and sufficient to prevent injection?

1 Answers1