45

In my company's codebase, we hardcode sql queries without using an ORM.

Here's an example of a query we would run:

UPDATE client SET status="active" WHERE client_id=123

Since the query is hardcoded and the parameters are passed in, the code would look like this (Python):

'UPDATE client SET status="{}" WHERE client_id={}'.format('active', 123)

The only place that the parameters are coming from is the code where they are set. There is no case where external users can pass in arguments.

The codebase is not large but is quite sprawling and interrelated, so sanitizing the SQL would require a legitimate reason for the time spent.

Therefore, is there a reason to worry about sanitizing the SQL?

16
  • 21
    Your code would immediately break the moment someone makes a status that uses the " character.
    – T. Sar
    Mar 7, 2023 at 11:34
  • 35
    Programming involves wrangling complexity, more complexity than we can reliably track. Thus, we create abstractions and tools to hide and automate parts. We program defensively, not just to guard against unexpected inputs but to guard against our own fallibility. It is possible to use query interpolation correctly, but it is easy to accidentally mess up. Thus, we prefer fixed query strings or parametrized queries whenever possible. Similarly, it is possible to write secure code in C, but it's easy to accidentally make grave errors. Thus, we prefer memory-safe languages like Python.
    – amon
    Mar 7, 2023 at 13:03
  • 61
    As a data scientist my threat model of SQL injection attacks consists entirely of my own stupidity and ignorance. That is not a trivial threat. Mar 7, 2023 at 15:24
  • 27
    Community, if someone asks a question about a bad idea which inspires good answers, the question deserves upvotes, not downvotes, regardless how bad the idea is.
    – Doc Brown
    Mar 7, 2023 at 18:15
  • 13
    You don't need to sanitise input at all. You need to use parametrized queries.
    – OrangeDog
    Mar 7, 2023 at 21:03

5 Answers 5

59

If there is never any user input at all, or the program is only used internally, the importance of sanitizing should be reduced.

But there are still a few possible reasons to always do so

  1. If all queries are sanitized there is no risk for some developer to copy paste code to some place where it does matter.
  2. It is possible that future changes may take input from some user. If this is done it will be much more expensive to go through all the code base and fix all the queries. Or worse, this is forgotten about.
  3. Parametrized queries should only be marginally more complex to write, and may have other advantages with regards to performance and/or type safety. With some languages/libraries/frameworks it may be more complex to write a non-parametrized query, and intentionally so.
  4. Figuring out if a query may take user input or not can be costly. It is probably easier to just use the safe version everywhere.
  5. There is less chance of formatting issues, the format of many data types depend on the localization, and there is no guarantee that this will match the localization of the database.
  6. Parametrized queries might be easier to read and maintain, but this might depend on programming language etc. But it will almost certainly be easier to read by someone used to and expecting all queries to be parametrized.
7
  • 17
    5. Parametrized queries are more readable and maintainable. WHERE Foo = @Bar is much more meaningful than Foo = {0}. String format with dozen parameters is not fun. As a bonus, string interpolation combined with nameof() will keep query up to date during automatic refactor. 6. Parametrized queries are not bound to peculiarities of String.Format of language/framework/runtime. Not only query will work elsewhere, you won't have to deal with parameter order. 7. Parametrized query won't break when using float/date on system with different locale settings than database.
    – PTwr
    Mar 7, 2023 at 21:24
  • 4
    @PTwr: 8. Parameterized queries' execution plans are better cached by databases. Mar 8, 2023 at 8:42
  • 2
    @PTwr: Your 5 and 6 are possibly a little dubious: some database libraries (*cough* Perl DBI *cough*) have parameterised syntax like WHERE Foo = ?; conversely some string format libraries (eg Python) allow WHERE Foo = {foo}. (Not that this should discourage anyone from using parameterised queries - the advantages still massively outweigh the disadvantages!)
    – psmears
    Mar 8, 2023 at 10:27
  • 2
    @Matthieu M. Query plan caching depends on the RDBMS, and it might even have ill side effects. I gave a hunch that it was Oracle which will happily ignore the parameters (and the corresponding statistics) when it hits a parameterized query where the plan is already cached. A WHERE clause like "status = :status" might give a full table scan with :status = 'closed', while :status = 'open' uses an index (table has a few dozen rows with status 'open', billions with status 'closed'). Additional fun, like when a SORT BY is present, might use a suboptimal index. Using the same plan sucks.
    – Klaws
    Mar 8, 2023 at 12:17
  • 1
    @Klaws: There are indeed corner cases, in general though it's beneficial. Mar 8, 2023 at 12:32
41

Parameterised queries should be your standard approach to all SQL. If you are trying to find reasons why you don't have to use them, then you are doing yourself a disservice.

In your example you have no parameters at all to pass, the very fact that you have written the sql as a formatted string rather than just putting the parameters in the string suggested that you do pass these in from somewhere and you should be using a parameterised query.

Hackers are sneaky, they don't need a text box to type 'bobby drop tables' in and have it passed right to the database, these tricks can be added to files, other databases, urls etc etc. No parameter is safe.

35

By using parameterized queries your SQL server doesn't have to recalculate the query execution plan each time you use a query.

This can improve performance for queries ran often.

5
  • 6
    This is a benefit that people often overlook. I was about to comment on JonasH's answer about this, until I read your answer. Especially since parameterized queries are pretty easy to write. Most database libraries and ORMs support this intuitively. Mar 7, 2023 at 17:59
  • Skipping an ORM should only be done for prototypes or something that is actually temporary. The long-term consequence of not having an ORM is always a disaster.
    – Nelson
    Mar 8, 2023 at 0:43
  • It's risky if the data is skewed. Check my comment in response to Matthieu M.'s comment.
    – Klaws
    Mar 8, 2023 at 12:19
  • 8
    @Nelson It might be perspective but I have always felt that ORMs were a disaster. Using parameters is a no brainer but an ORM is more situational.
    – stoj
    Mar 8, 2023 at 12:58
  • I think whether or not one should use an ORM is outside the scope of this answer.
    – Pieter B
    Mar 10, 2023 at 7:55
10

I just want to point out that your example is actually extremely, extremely nasty, because somebody not familiar or not careful enough would glance at these parentheses and assume that you're using parametrized queries and that the input is already safe.

Your example:

'UPDATE client SET status="{}" WHERE client_id={}'.format('active', 123)

Somebody comes in 5 years later with a task to accept the status from an user input, sees something like this and just changes it to

'UPDATE client SET status="{}" WHERE client_id={}'.format(userInput['status'], 123)

and boom, you've got yourself a fully-fledged SQL injection.

If you're insistent of doing things your way, at least make it obvious that something weird is going on.

'UPDATE client SET status="' + 'active' + '" WHERE client_id=' + '123'

Or even better, just put the values directly so it's known that they're constant.

'UPDATE client SET status="active" WHERE client_id=123

Or, the best way, just use the damn parameters instead of wasting your time thinking about this stuff.

cursor.execute('UPDATE client SET status=? WHERE client_id=?', ('active', 123))

Humans fail all the time. Just try to make it a bit harder for them.

1
  • 1
    It might be worth noting that the correct approach is also shortest! (it's not obvious at first sight because the wrong examples don't include the cursor.execute call, but it will be there in the end.
    – Jan Hudec
    Mar 9, 2023 at 9:39
3

I'm going to contradict some of the other answers and say there's little reason to parametrize a query where all the values are hard-coded.

However, you generally shouldn't be hard-coding so much. In your query, 123 is a magic number, and these should generally be avoided. There should be a named variable (or constant, in languages that support them) that describes the meaning of the number.

And once you move that value into a variable, then you should use a query parameter to substitute it. While there may not be a security purpose for this, since the number isn't supplied by the user, it's still a good practice for the reasons given in the other answers.

client_foo = 123
cursor.execute('UPDATE client SET status="active" WHERE client_id=?', (client_foo,))

The meaning of active is self-evident, so hard-coding it is less of a problem. However, if the script has multiple queries that set different statuses, you may want to create a generic SQL query or procedure that can be used to set different statuses, and this would be best done with another parameter.

update_status = 'UPDATE client SET status=? WHERE client_id=?'
...
cursor.execute(update_status, ("active", client_foo))
cursor.execute(update_status, ("completed", client_bar))

Not the answer you're looking for? Browse other questions tagged or ask your own question.