Skip to content

Audit all use of push_sql + format! within nexus/db-queries to prevent SQL Injection #4144

Open
@smklein

Description

@smklein

Diesel uses the 'bind' method (See SqlQuery::bind for one example, but there are other variations) to ensure that variables within SQL statements are escaped property, avoiding SQL injection.

This is generally done automatically -- for a query like:

let data = animals
    .select(species)
    .filter(name.eq("; DROP TABLE USERS; "))
    .first::<String>(connection)?;

The argument to .eq is AsExpression, which treats this argument as data to be escaped before placing it in the query.

From the Diesel docs:

Implementations of [AsExpression] will generally...
... Indicate that the type has data which will be sent separately from the query. This is generally referred as a “bind parameter”. Types which implement ToSql will generally implement AsExpression this way.

That works through the ToSql trait, which basically encapsulates "how to shove a value into the protocol which gets sent to the DB".

For vanilla Diesel, this is fine, and this usage should not be susceptible to injection attacks -- preventing this is the responsibility of those ToSql and AsExpression traits.

However, nexus/db-queries has some spots where it constructs more complicated queries, relying on traits like QueryFragment to construct SQL queries from strings. Here's one such example:

impl QueryFragment<Pg> for InsertVpcQuery {
fn walk_ast<'a>(
&'a self,
mut out: AstPass<'_, 'a, Pg>,
) -> diesel::QueryResult<()> {
out.push_sql("SELECT ");
out.push_bind_param::<sql_types::Uuid, Uuid>(&self.vpc.identity.id)?;
out.push_sql(" AS ");
out.push_identifier(dsl::id::NAME)?;
out.push_sql(", ");
out.push_bind_param::<sql_types::Text, Name>(&self.vpc.identity.name)?;
out.push_sql(" AS ");
out.push_identifier(dsl::name::NAME)?;
out.push_sql(", ");

Using this interface correctly means:

Note that "fully static SQL" is not actually enforced by the type system -- if you wanted to add a string constructed via format! here, you could.

  • Audit all usage of push_sql
  • Write property tests to confirm that SQL injection via APIs is unlikely
  • Document this property somewhere?

Metadata

Metadata

Assignees

No one assigned

    Labels

    databaseRelated to database accessnexusRelated to nexus

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions