3 ms·

The right way to avoid SQL injection is of course to use parameterised queries. Which also has the nice side effect of speeding up the queries too. While atte

The right way to avoid SQL injection is of course to use parameterised queries. Which also has the nice side effect of speeding up the queries too. While attending various security vendor demos of static analysis products, they all used SQL injection as their example, and all of them recommended input validation as the mitigation rather than parameterised queries. You'd think they would know better. It's a cute idea to compare the AST of the query to see if it changes, but then you have to define all valid queries - a whitelisting approach. I'm struggling to see why you would want it unless you had control over a database but had no control over the code that issues the queries.

tialaramex 4y ago
It reminds of the quote about Americans sometimes falsely attributed to Churchill saying they'll always do the right thing, "once they've tried everything else" (since it isn't a real quote you'll find a variety of phrasings) Parametrisation is the Right Thing™ and yet people love inventing new half-arsed workarounds when they could just use parameterisation. Elsewhere in security, historically you would find any number of hacks to try to avoid needing to use a good Mode Of Operation for your block cipher. People would rather expend mental effort and protocol complexity on trying to still do ECB when that's the Wrong Thing™ than just use the Right Thing™ (these days that's an inherently Authenticated Encryption Mode of Operation like GCM or GCM-SIV) Or look at all the work trying to shore up human memorable shared secrets (passwords) when you almost never actually needed this and could have instead deployed stuff like WebAuthn.
- tsimionescu 4y ago
  Parametrization is not enough, there are plenty of scenarios where you need to programmatically construct queries based on user input in more complex ways than parameterized queries allow - including subqueries, lists (IN clauses), sort orders, and perhaps others.
  - lupire 4y ago
    But instead of inventing giant piles of workarounds, put your energy into fixing the database software.
  - qw 4y ago
    > you need to programmatically construct queries based on user input You still need to build the queries and have control of what parametres are in use. Why not build the parameters list dynamically as well?. There are some databases or drivers that have limitations where you need to sanitize data yourself, but I'm not sure if that applies to the majority of the scenarios. This is a simple example of code for constructing dynamic queries: query = "... WHERE country = :country"; params.set("country", country); if (userId != null) { query += " AND user_id = :userId"; params.set("userId", userId); } It may become more complex as you add sub queries or if the database does not support adding a list as a parameter, but the principle is the same. You can always just build the list programatically using parameters instead of concatenating the values given by the user directly. If there are drivers or databases that does not support this, you will need to sanitize the data yourself of course. But I don't think that is the case for most users. (btw, database drivers that support named parameters are much more convenient than using "?")
  - MattPalmer1086 4y ago
    I guess there may be SQL frameworks or ORMs that make that hard. I worked on something that allowed parameterisation of any user supplied value, no matter how complex the query. It wasn't that hard, but we could make it do whatever we wanted.
jsmith45 4y ago
Sadly it is not always possible in general to completely avoid constructing queries programmatically (which always comes down to string concatenation at some level of the stack) as parameterized queries have some limitations. For example engines don’t always support a list parameter, meaning an “in” clause with varying number of entries can’t work. (Some might allow table variable parameters as a workaround, but not all). Another common reason for needing dynamic query construction varying sort by. Once again many databases don’t let you do “order by ? ?” (First being column name, second ASC/DESC). But a good query builder abstraction with knowledge of your specific DBMS, can help avoid injection here, especially if it does a sanity check of reparsing the generated query and comparing it against an AST it built of the target query. Even when using such a system though, it can make sense to have the query builder build a parameterized query, and use that for passing as many parameters as possible, just as defense in depth. (Avoids bugs in escaping code of query builder, for example.)
- vlovich123 4y ago
  ASC/DESC doesn’t technically need parameterization. The point about parameterizing and not query building is about not query building directly from user inputs. It’s fine to build the query fine when you control every part of the input used to build the string. Parameterization is only really for when you need to process a user string, particularly. One mitigation for the column name is to validate it’s in the enumeration of available column names first. Now of course, if you’re trying to be super strict about parameterization so that static tools / linters / code policy during review don’t complain that’s one thing. But it’s not an invalid choice to utilize domain knowledge to simplify the problem if your tooling doesn’t let you do the right thing.
  - jsmith45 4y ago
    Right. Concatenating in insufficiently constrained user inputs are dangerous (and trying to constrain a general string enough is usually too risky). If you can prove that a user input is a harmless value (a valid column name for the table for an order by, an integer, etc) then concatenation is safe. On the other hand, it is still not ideal have a bunch of code where you are directly concatenating strings to generate SQL, since people making later changes are likely to end up modifying code to concatenate unsafe input. Hence why having a good query builder api can be valuable. It can hide away the string concatenation, and if well designed, can force that a value but in the order by position is a column name, etc, ideally while making it hard or impossible to have insufficiently constrained user input thrown in. For example, if a language supports distinguishing string literals (or compile time constant strings), and variable string values, it is possible to create a query builder api where arbitrary user provided strings simply cannot become part of the query. (e.g. The only paths that let non-constant strings into the query are those that highly constrain the strings to provably safe values).

tialaramex 4y ago

It reminds of the quote about Americans sometimes falsely attributed to Churchill saying they'll always do the right thing, "once they've tried everything else" (since it isn't a real quote you'll find a variety of phrasings) Parametrisation is the Right Thing™ and yet people love inventing new half-arsed workarounds when they could just use parameterisation. Elsewhere in security, historically you would find any number of hacks to try to avoid needing to use a good Mode Of Operation for your block cipher. People would rather expend mental effort and protocol complexity on trying to still do ECB when that's the Wrong Thing™ than just use the Right Thing™ (these days that's an inherently Authenticated Encryption Mode of Operation like GCM or GCM-SIV) Or look at all the work trying to shore up human memorable shared secrets (passwords) when you almost never actually needed this and could have instead deployed stuff like WebAuthn.

tsimionescu 4y ago
Parametrization is not enough, there are plenty of scenarios where you need to programmatically construct queries based on user input in more complex ways than parameterized queries allow - including subqueries, lists (IN clauses), sort orders, and perhaps others.
- lupire 4y ago
  But instead of inventing giant piles of workarounds, put your energy into fixing the database software.
- qw 4y ago
  > you need to programmatically construct queries based on user input You still need to build the queries and have control of what parametres are in use. Why not build the parameters list dynamically as well?. There are some databases or drivers that have limitations where you need to sanitize data yourself, but I'm not sure if that applies to the majority of the scenarios. This is a simple example of code for constructing dynamic queries: query = "... WHERE country = :country"; params.set("country", country); if (userId != null) { query += " AND user_id = :userId"; params.set("userId", userId); } It may become more complex as you add sub queries or if the database does not support adding a list as a parameter, but the principle is the same. You can always just build the list programatically using parameters instead of concatenating the values given by the user directly. If there are drivers or databases that does not support this, you will need to sanitize the data yourself of course. But I don't think that is the case for most users. (btw, database drivers that support named parameters are much more convenient than using "?")
- MattPalmer1086 4y ago
  I guess there may be SQL frameworks or ORMs that make that hard. I worked on something that allowed parameterisation of any user supplied value, no matter how complex the query. It wasn't that hard, but we could make it do whatever we wanted.

jsmith45 4y ago

Sadly it is not always possible in general to completely avoid constructing queries programmatically (which always comes down to string concatenation at some level of the stack) as parameterized queries have some limitations. For example engines don’t always support a list parameter, meaning an “in” clause with varying number of entries can’t work. (Some might allow table variable parameters as a workaround, but not all). Another common reason for needing dynamic query construction varying sort by. Once again many databases don’t let you do “order by ? ?” (First being column name, second ASC/DESC). But a good query builder abstraction with knowledge of your specific DBMS, can help avoid injection here, especially if it does a sanity check of reparsing the generated query and comparing it against an AST it built of the target query. Even when using such a system though, it can make sense to have the query builder build a parameterized query, and use that for passing as many parameters as possible, just as defense in depth. (Avoids bugs in escaping code of query builder, for example.)

vlovich123 4y ago
ASC/DESC doesn’t technically need parameterization. The point about parameterizing and not query building is about not query building directly from user inputs. It’s fine to build the query fine when you control every part of the input used to build the string. Parameterization is only really for when you need to process a user string, particularly. One mitigation for the column name is to validate it’s in the enumeration of available column names first. Now of course, if you’re trying to be super strict about parameterization so that static tools / linters / code policy during review don’t complain that’s one thing. But it’s not an invalid choice to utilize domain knowledge to simplify the problem if your tooling doesn’t let you do the right thing.
- jsmith45 4y ago
  Right. Concatenating in insufficiently constrained user inputs are dangerous (and trying to constrain a general string enough is usually too risky). If you can prove that a user input is a harmless value (a valid column name for the table for an order by, an integer, etc) then concatenation is safe. On the other hand, it is still not ideal have a bunch of code where you are directly concatenating strings to generate SQL, since people making later changes are likely to end up modifying code to concatenate unsafe input. Hence why having a good query builder api can be valuable. It can hide away the string concatenation, and if well designed, can force that a value but in the order by position is a column name, etc, ideally while making it hard or impossible to have insufficiently constrained user input thrown in. For example, if a language supports distinguishing string literals (or compile time constant strings), and variable string values, it is possible to create a query builder api where arbitrary user provided strings simply cannot become part of the query. (e.g. The only paths that let non-constant strings into the query are those that highly constrain the strings to provably safe values).