Open gpmcadam opened 7 months ago
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.
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.
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
Example 2 - Contains an unescaped param (surely at least a medium priority?)
Example 3 - Can infer that the variable is from userland and should be a higher priority?
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.