4

I understand the basic idea of how mysql statements can be vulnerable, but every time I try to find a useful guide, the ways to achieve this with PDO looks different from eachother. Also, I´m sometimes being told here at stackoverflow that my code is vulnerable such as the other day where it was said about the following (which doesn´t work btw, but I was taught how to make it so:

$search = $_GET["search"];
$searcharray = explode('|', $search);
$query=("SELECT username,sender,message,subject,timestamp,threadid,msgtype 

FROM Messages WHERE  ('" . implode("'|'",$searcharray) . "') IN CONCAT 
(message,subject)  ORDER BY timestamp");

.. but why? Would it not be enough to have:

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

before the code and 

$result = $conn->query($query)->fetchAll(PDO::FETCH_OBJ);

afterwards?

Are people automatically assuming that I don´t have these parts because I only post the part which is relevant for my question, or is there a part of my SELECT statement that in itself is vulnerable? Also, do I need to PDO-ify all mysql statement, so not only SELECT but also UPDATE, INSERT etc. needs to be updated?

Thanks in advance!

Corey Hart
  • 173
  • 1
  • 2
  • 15
  • 1
    It's not enough to simply build strings and send them to PDO, most of the protection granted comes from using prepared statements with bound variables -- [this popular q/a](https://stackoverflow.com/a/60496/4096667) has some good info for you. – A C Jun 12 '17 at 13:28
  • Thanks A C, I´ll check it out! – Corey Hart Jun 12 '17 at 13:37
  • 2
    If I provided your page with `?search=');%20DROP%20TABLE%20Messages;%20--`, your script would happily send that to the database. – castis Jun 12 '17 at 13:39

1 Answers1

5

Your query is vulnerable because you directly use input send to the server without escaping it (e.g. $_GET) You should use prepared statements and bind the variables you're using:

$search = $_GET["search"];
$searcharray = explode('|', $search);
$query=("SELECT username,sender,message,subject,timestamp,threadid,msgtype 

FROM Messages WHERE  :searchParams IN CONCAT 
(message,subject)  ORDER BY timestamp");

$query = $conn->prepare($query);
$query->execute(['searchParams' => implode("'|'",$searcharray)]);

This way, the user input gets escaped.

Sepultura
  • 997
  • 1
  • 9
  • 28