1

I have the following piece of code (inherited from previous dev):

declare(strict_types=1);

function updateWithCurrentTime(PDO $connection, int $id): void{

    $date = date('m/d/Y h:i:s a', time());
    $query= "INSERT INTO timetable (id,time) VALUES (${id},${date})";

    $connection->query($query);
}

$connection = new PDO('sqlite::memory:');
$connection->query("CREATE TABLE timetable (id INT , date TEXT)");

updateWithCurrentTime($connection,1);

And as you can see it does not use, as recommended, prepared statements, instead it passes directly the parameters into the query. But as you also can see the parameters in the function updateWithCurrentTime are type-hinted.

So I wanted to know does by type-hinting a function's input parameters makes it SQL-injection safe? Even though no Prepared Statements are being used.

Keep in mind that I assume that no string-type input parameters will be provided, in string type input parameter using prepared statements is the one way to go.

Dimitrios Desyllas
  • 9,082
  • 15
  • 74
  • 164
  • 2
    When you pass to this function wrong type a `TypeError` exception will be thrown. Do you use `strict_types`? – Michał Haracewiat Jun 27 '19 at 07:36
  • For now no, I do not use it. – Dimitrios Desyllas Jun 27 '19 at 07:37
  • hi, i wouldn't trust type-hinting to prevent sql injection attacks even in specific cases. i hope a white hat answers and let me read too. my guess is "yes, u do not need prepared stmt for this specific data update case". however best practice is having the habit of using the prepared stmts i think. – Andre Chenier Jun 27 '19 at 07:40
  • 1
    you MUST use strict type to make the type hinting functional. – Andre Chenier Jun 27 '19 at 07:41
  • `strict_types` is not required as far as i know. It only allows to pass `1.1245` and `"1asd"` as `$id`. Which is then converted to `1`. – Roland Starke Jun 27 '19 at 07:48
  • I don't see an SQL injection threat with the data used, as the date is created internally and not provided by a third party. Type-hinting will also cause an error to be thrown if the id can't be turned into an integer. – Angel Politis Jun 27 '19 at 07:51

2 Answers2

3

Strictly speaking, the query you show is safe since you have a guarantee that the $id is only an integer value, and it cannot introduce characters to the SQL query that would cause any mischief.

But the type hinting solution won't work for string type hints. Even if you type-hint that your function argument must be a string, 

function updateWithCurrentTime(PDO $connection, int $id, string $value): void{

    $query= "INSERT INTO timetable (id,value) VALUES (${id}, '${value}')"; // UNSAFE

You wonder, does $value contain any quote characters? Does it contain anything else that would cause mischief? You can't prevent it from being an SQL injection vulnerability just by using a type hint.

So type hinting may be effective when using an int type, but not a string type. What about the other types? Hmm, must investigate...

Now you've opened a can of worms. You have to investigate all the types, and try to come up with complete guidelines for which ones are safe to interpolate into SQL and which ones aren't. You have to make these guidelines clear enough so every member of your software developer team can follow them, and can use them during code reviews.

Even if you do write the code perfectly and use the appropriate SQL injection defense method for each type, anyone else reading the code later will be confused. "Why aren't variables combined with SQL queries in the same way in different functions?" They would wonder. Figuring out the reasons would take their time and attention away from doing whatever code maintenance task they were assigned to do. Remember that after you develop this code, it will live on for years, and other software developers need to maintain it. It pays off to make code easy to understand.

Or, you can just use parameters.

SQL query parameters work for all types, and you don't have to rely on type-hinting. They're easy, they're effective, and you can use them consistently.

Please stop trying to find ways to avoid using query parameters.

Let me use an analogy: Suppose you were an electrician instead of a software developer. You heard that electrical wires should be insulated to avoid possibilities of short-circuits. But for some reason you don't feel like dealing with insulated wires.

"I'll just put a shim in between the wires to keep them separated." But the item you use as a shim has to be non-conductive and non-flammable.

What kinds of shims are safe to use? Wood... no. Metal... no. Plastic... depends on the type of plastic. Ceramic... I don't know, have to look it up or something...

Every other electrician would look at you strangely.

"Just use insulated wires, are you trying to burn down this building?"

Bill Karwin
  • 538,548
  • 86
  • 673
  • 828
  • Well you tell that to the original author of the code I came across ;) Now I can tell the confidence why I need to refactor the code and I have a good excuse to do so ;) – Dimitrios Desyllas Jun 27 '19 at 18:02
  • 1
    Yeah, I understand you're dealing with legacy code that was written by someone else who had bad habits. Hopefully the writeup above will be enough to convince other readers of Stack Overflow who are facing a similar choice. – Bill Karwin Jun 27 '19 at 19:04
1

Technically yes. The function will either gracefully cast the argument to an integer, or throw a TypeError exception. This will work.

Depending on your strict_types declaration.

Practically - it's poorly received.

It's not good practice (you mentioned prepared statements - they're the right solution). What's even worse, you might not have access to the server configuration, and you don't know whether the strict mode is or isn't enabled.

Michał Haracewiat
  • 488
  • 1
  • 3
  • 9