Better granularity of SQL injection

[gpmcadam]

Is it possible to better break down SQL injection vectors where the examples below are not all treated as low?

Example 1 - Contains no params and is should not be consider a vector or retains its 'low' status

q = """
    SELECT *
    FROM foo
    """

Repo.query(q)

Example 2 - Contains an unescaped param (surely at least a medium priority?)

q = """
    SELECT *
    FROM foo
    WHERE a = #{some_var}
    """

Repo.query(q)

Example 3 - Can infer that the variable is from userland and should be a higher priority?

  def index(conn, params) do
    some_var = Map.get(conn.params, "bad_var", "")


    q = """
        SELECT *
        FROM foo
        WHERE a = #{some_var}
        """

    Repo.query(q)
  end

At the moment all of these vectors are treated the same (Low) and there are legit cases where you might want to accept hard-coded queries with no interpolation, but it feels dangerous to skip these as they may be adapted in future changes and then missed.

[houllette]

Hey @gpmcadam - great question! So the short answer is unfortunately that is not something that Sobelow can do currently.

The longer explanation is that the commonality between your three examples is Repo.query(...) which is what Sobelow is actually triggering off of - essentially just the fact that that particular function which is associated with vulnerability is being used. The "low" that Sobelow is outputting on the detections is confidence (not priority) that this is actually an issue, since we cannot be confident from the onset that what entered into the query comes from userland or not.

The primary method that other SAST tools use to make the sort of inference you're talking about is called Taint Analysis / Checking - which is kind of the holy grail of being able to produce higher confidence detections. Having Taint Analysis would improve more checks than just SQLi for Sobelow, which is precisely why I've been noodling on how best to introduce this functionality into the tool, but I fear that feature-set is still a long ways off since a lot of work would be required to make this accurately work.

[halostatue]

This might be a separate issue, but Sobelow.SQL.Query seems to be excessively loose. I am getting warnings on a module like this:

defmodule GraphQLQueryFunction do
  def customer_id(client, token) do
    query(
      client,
      """
      getCustomerId($customerAccessToken: String!) {
        customer(customerAccessToken: $customerAccessToken) { id }
      }
      """,
      %{customerAccessToken: token}
    )
  end

  def query(client, string, params) do
    execute(client, "query " <> string, params)
  end
end

I’m not sure how this should be fixed, but right now it’s just finding any function call named query or query! without context of whether it’s imported, using Repo.query, or like the example above, defined in the function in question.