Rust: SQL Injection Query

[geoffw0]

Adds a query rust/sql-injection to detect SQL injection vulnerabilities in Rust. Note that there's a query, docs, tests, and various wiring for models (including a Concepts.qll) but no actual source or sink models are implemented in this PR - so no results will be found at this time. Nevertheless I'm keen to get reviewers eyes on this and get something merged while I work on those models as a follow-up.

The test is quite slow, due to fetching dependencies. We will want the stub generator before we have a large query suite IMO.

TODO:

• code review
• docs review
• DCA run (there won't be any results)

I'm deferring discussions about autofix until we have sources + sinks and thus results to evaluate.

[github-actions]

QHelp previews:

rust/ql/src/queries/security/CWE-089/SqlInjection.qhelp

Database query built from user-controlled sources

If a database query (such as an SQL query) is built from user-provided data without sufficient sanitization, a user may be able to run malicious database queries. An attacker can craft the part of the query they control to change the overall meaning of the query.

Recommendation

Most database connector libraries offer a way to safely embed untrusted data into a query using query parameters or prepared statements. You should use these features to build queries, rather than string concatenation or similar methods. You can also escape (sanitize) user-controlled strings so that they can be included directly in an SQL command. A library function should be used for escaping, because this approach is only safe if the escaping function is robust against all possible inputs.

Example

In the following examples, an SQL query is prepared using string formatting to directly include a user-controlled value remote_controlled_string. An attacker could craft remote_controlled_string to change the overall meaning of the SQL query.

// with SQLx

let unsafe_query = format!("SELECT * FROM people WHERE firstname='{remote_controlled_string}'");

let _ = conn.execute(unsafe_query.as_str()).await?; // BAD (arbitrary SQL injection is possible)

let _ = sqlx::query(unsafe_query.as_str()).fetch_all(&mut conn).await?; // BAD (arbitrary SQL injection is possible)

A better way to do this is with a prepared statement, binding remote_controlled_string to a parameter of that statement. An attacker who controls remote_controlled_string now cannot change the overall meaning of the query.

// with SQLx

let prepared_query = "SELECT * FROM people WHERE firstname=?";

let _ = sqlx::query(prepared_query_1).bind(&remote_controlled_string).fetch_all(&mut conn).await?; // GOOD (prepared statement with bound parameter)

References

• Wikipedia: SQL injection.
• OWASP: SQL Injection Prevention Cheat Sheet.
• Common Weakness Enumeration: CWE-89.

[sunbrye]

From the docs team—did an editorial review and this looks great! I'll let someone else perform the technical review of this though

[hvitved]

Can't this simply be final class ThreatModelSource = ThreatModelSource::Range?

[geoffw0]

I haven't worked with Range classes before. My understanding is that in the frameworks we're expected to sometimes want to override both, so this can't be final, but .... I will admit I'm not really sure what the point of a separate class and Range is.

[hvitved]

I don't see why one would want to override both ThreatModelSource and ThreatModelSource::Range. I think we should change it like above.

[geoffw0]

OK, I think I see what's going on here. The intent of the "Range" pattern is that you can extend the abstract Range class to expand the class to cover new things (i.e. in this case to add new sources). But if you want to define something in terms of ThreatModelSource (i.e. extend it the non-abstract way, as ActiveThreatModelSource does), you use the non-Range version. Making it final actually doesn't remove the latter possibility, thanks to "final extensions" (which I haven't knowingly used much before).

Change made. The code is quite a lot cleaner as a result. Other languages could implement the same change in their Concepts.qll.

[hvitved]

Shouldn't this be restricted somehow using getSourceType()?

[geoffw0]

I don't think so. ActiveThreatModelSource is a restriction of ThreatModelSource according to user preferences (however that works), which defaults to just the remote sources I think. This sounds like exactly what we want.

[hvitved]

Ah ok, to ActiveThreatModelSource should be used for all queries that already use remote flow sources?

[geoffw0]

I believe that's the idea. Unless you have a special reason for really only wanting remote flow sources, but generally in the past we've generally specified remote only because most users aren't interested in flow from argv

[hvitved]

I think this comment is redundant.

[geoffw0]

As in, it doesn't say much that isn't obvious from the code below it?

I tend to comment nearly every file, class and module (with occasional exceptions). One reason is I think it's a requirement in .qlls. Another is that I personally tend to read comments before I look at code, so a module without a comment is harder to read...

[geoffw0]

I'd be happy to do away with the doc on the ThreadModelSource module and ThreadModelSource::Range class, as it really just duplicates that on ThreadModelSource. I don't think CI would accept that though???

[mchammer01]

I'll review this for Docs, either later today, or tomorrow.

[mchammer01]

LGTM, approving to unblock ✨
Made some minor comments, that you can ignore if you wish.
Sorry for re-reviewing this @sunbrye but it was still unassigned in our security products board, and I only realized now that you'd already given this an editorial review 😞

[mchammer01]

Who does they refer to here, the attacker? The use of this pronoun can be a bit ambiguous sometimes, but I can't find anything better at the moment.

[geoffw0]

Yep, "they" is the attacker, who is kind of the same person as the user in this paragraph anyway.

[geoffw0]

@hvitved are you happy with the changes to this PR?