nccgroup / sobelow

Security-focused static analysis for the Phoenix Framework
Apache License 2.0
1.66k stars 92 forks source link

Misses vector if function contains bang operator (!) #153

Closed gpmcadam closed 6 months ago

gpmcadam commented 7 months ago

The following will detect a low-confidence SQL Injection vector:

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

Repo.query(q)

However simply replacing Repo.query(q) with Repo.query!(q) (to force a raise) will prevent sobelow from detecting this attack vector.

houllette commented 7 months ago

Great callout, @gpmcadam! I will add this to the backlog and try to get to making the adjustments to get this fixed if/when I can.

In the meantime, as a general Call to Action for anyone - this is a great first issue! Currently the query.ex file is where the logic happens and currently it just looks at the single function call :query on line 37 to make the association.

I believe in order to translate it to support another similar function, one could take either of the following options:

  1. Duplicate and slightly tweak lines 26-28 and lines 36-38 to support query!
    • The benefit of this approach is it's probably the easiest, but may not hold up longevity wise and may not be the most performant
  2. Change the functionality of the entire detection to be more similar to that of something like our File traversal rule which supports detecting a plethora of sub-functions
    • This is probably the more accurate approach to better support long term if more similar functions are discovered needing support. e.g. query_many
  3. Another solution that I'm just not thinking about right now, open to other suggestions - just trying to be helpful / document what I know if someone wants to pick this up first!

My gut feeling is the first option is probably okay to pursue since there aren't nearly as many functions within the Repo/SQL module that we need to support compared to something like File - but take that with a grain of salt l since I have yet to make a rule tweak like this myself since taking on maintenance of Sobelow 🙂

PRs welcome!