Consider Reducing Risk of SQL Injection via &'static str?

[smklein]

I really like the type safety guarantees of Diesel, but also acknowledge that there are situations where I need to extend my usage of Diesel "outside the norms" of what's supported. Concretely, I've written a fair bit of code which re-uses Diesels type system, but constructs CTEs using judicious use of QueryFragments, emitting their fair share of raw SQL.

When in this situation, I want to ensure I protect users of my code from SQL injection. Googling "Diesel SQL injection" brings up this closed issue (#229) and not much else. I think it's pretty clear that "if you supply a string as a bind parameter, there is minimal risk of SQL injection", but there are some rules that need to be respected.

Here's a concrete use-case where I audited my own codebase for potential concerns: oxidecomputer/omicron#4144

Areas of concern that jumped out to me:

• Any calls to AstPass::to_sql which act on dynamically constructed strings.
• Any calls to sql_query which act on dynamically generated strings.
• Any calls to sql which act on dynamically generated strings.

I know that "dynamically constructed strings" isn't the real issue here - the issue is more subtle. It's any "attacker-controlled inputs", for some threat model, which could allow unescaped SQL to enter a query.

• The most obvious case here is "a parameter was passed via format! argument interpolation, when it should have been passed via a bind argument."
• A more subtle case would be something like "you wanted to dynamically select from one of many columns, and for some reason, you forgot to validate that the 'column' argument wasn't arbitrary SQL".

For better or for worse, Diesel happily allows either one of these to happen with the arguments to push_sql and sql_query. These patterns certainly are not encouraged, but there aren't really docs providing warning against them either (I do see warnings about type safety, but not about measures to prevent SQL injection).

---

So here's my observation: All these functions accept dynamic strings.

• pub fn push_sql(&mut self, sql: &str) (where self is AstPass)
• pub fn sql_query<T: Into<String>>(query: T) -> SqlQuery
• pub fn sql<ST>(sql: &str) -> SqlLiteral<ST>

But that kinda seems like an anti-pattern! If there existed variants of these functions which acted upon static strings, it would be (in my opinion) much more difficult to unintentionally inject arbitrary SQL. The usage of &'static str would basically scope down the "set of possible SQL emitted to the DB" to "only the set of SQL constructed at compile time".

Something like the following:

• pub fn push_sql_static(&mut self, sql: &'static str) (where self is AstPass)
• pub fn sql_query_static(query: &'static str) -> SqlQuery
• pub fn sql_static<ST>(sql: &'static str) -> SqlLiteral<ST>

Would make it significantly harder to hit these cases. Notably -- this doesn't prevent the usage of dynamic parameters! Rather, it encourages it even more, as it forces callers to pass user-supplied portions of their queries through separate bind calls.

[weiznich]

Thanks for writing this proposal.
You are right in that these three can be problematic.
Unfortunately its not as simple as just restricting the allowed arguments to &'static str for the following reasons:

• Its a breaking change, so we would need a major release for that. We could somehow sidestep that by deprecating the existing methods and providing new ones with the new signature.
• It would drastically restrict what sql and sql_query can express. That's especially problematic for cases where you need dynamical string building, like for example dynamic bind parameter lists for IN expressions or user provided identifiers that cannot be expressed as bind parameters.

I believe the second point does not apply to to AstPass::push_sql although I didn't check the whole codebase yet. If that works for all of diesel I'm open to consider deprecating + replacing the existing function.

For sql and sql_query it might be worth to consider a format like macro based approach instead, that automatically does the bind parameter expansion for IN, etc. That's something I want to try at some point in the future, but that is likely some while away.

[davepacheco]

I think adding such functions (without a breaking change) could be a useful tool for some users to address their risk, even if it doesn't solve the problem for everyone.

[weiznich]

@davepacheco As written above, I'm generally open to improvements in this area as long as it addresses the underlying problem. Can you provide some details on how this would help you to address the risk of SQL injections? I'm not sure how only adding these functions will migrate the risk of the existing functions.

[davepacheco]

I believe that in the codebases we work on, in the cases where we need to generate SQL with these functions, we're always putting together fragments of SQL that are themselves &'static str (literals). If the static versions of the functions were available to us, we would consider using them exclusively and auditing (or maybe just banning) the versions that accept dynamic strings, similar to how we treat unsafe. I understand this does not help projects that use more dynamically-generated queries. But I suspect many projects are like ours. (Even in the cases where I've seen format strings used to generate SQL, those SQL strings could usually be expressed instead as a sequence of &'static str and bound parameters instead. People don't have to do that, but if they choose to, then when combined with the proposed functions, one could greatly reduce the risk of SQL injection.)

[weiznich]

If the static versions of the functions were available to us, we would consider using them exclusively and auditing (or maybe just banning) the versions that accept dynamic strings, similar to how we treat unsafe

My concerns are mostly about this part of the proposal. For unsafe there is quite a lot tooling that would allow you to enforce that. For the sql* function such tooling does mostly not exist. You could probably use the
disallowed-methods clippy lint for banning, although depending on your project setup you would like to have different levels (deny, warn) for different methods.
As for the auditing approach, to play devils advocate: How would that be different to just auditing the existing function usages to only use &'static str as argument?

That all shouldn't imply that I'm not interested in adding such methods, I just want to make sure that such a change would really fit your usecase or whether there might be a better solution for that.

[davepacheco]

If the static versions of the functions were available to us, we would consider using them exclusively and auditing (or maybe just banning) the versions that accept dynamic strings, similar to how we treat unsafe

My concerns are mostly about this part of the proposal. For unsafe there is quite a lot tooling that would allow you to enforce that. For the sql* function such tooling does mostly not exist. You could probably use the disallowed-methods clippy lint for banning, although depending on your project setup you would like to have different levels (deny, warn) for different methods. As for the auditing approach, to play devils advocate: How would that be different to just auditing the existing function usages to only use &'static str as argument?

Comparing "prefer this function to that function" with "prefer calling this function only in this specific way": I think the former is (1) a simpler message to communicate, (2) easier to notice when it's being violated (because you're more likely to notice push_sql than you are "this argument is not &'static str), and (3) easier for someone whose trying to verify it to verify it (because you don't have to work out what type the argument is). Another way to look at it: when writing Rust code, we like to define interfaces so that the compiler can help ensure that they're used safely. We try to avoid rules that the compiler can't help with, that people have to just know and enforce themselves.

Plus, as you said, we could use disallowed-methods to ban it (overriding where necessary).

[weiznich]

Given that discussion I'm in favor of accepting a PR that adds these methods as long as

• it includes some documentation on the new and old functions that explain when to prefer which variants
• there are tests for the new variants
• it also updates diesel to prefer using the push_sql_static variant where possible.