search

guillaumeaubert / Perl-Critic-Policy-ValuesAndExpressions-PreventSQLInjection

PerlCritic policy that attempts to detect the most common sources of SQL injection in manually crafted SQL statements, by detecting the use of variables inside interpolated strings that look like SQL statements.
https://metacpan.org/pod/Perl::Critic::Policy::ValuesAndExpressions::PreventSQLInjection
Other
6 stars 8 forks source link

detect sql string modification #11

Open vsespb opened 10 years ago

vsespb commented 10 years ago

this

my $sql = "select from ";
$sql.=$table;

and even that:

my $sql = "select from ";
$sql ="$sql $table";

(possibly 1st case more common and 2nd is pretty complex)

p.s. Great project ! Thanks!

guillaumeaubert commented 10 years ago

Hi Victor,

The difficult part of this problem is the scope of variables. It's very easy to find all the occurrences of a variable named $x using a PPI tree, however PPI does not distinguish reused variable names or variables with the same name but in separate scopes.

I have not seen any CPAN modules where I could pass a PPI tree and a variable name, and get a list of all the occurrences of that variable broken down by scope. I think the most promising path to solving this problem is to create such a module, then use the output to look at the context of each occurrence within the same scope to determine whether the modifications represent an injection risk.

On the other hand, maybe the variable reuse problem is addressed by Perl::Critic::Policy::Variables::ProhibitReusedNames. As long as both plugins are used then maybe ignoring variable reuse is an acceptable trade-off?

Your second case is beyond the scope of PPI, but fortunately Perl::Critic::Policy::ValuesAndExpressions::PreventSQLInjection::extract_variables() makes it easy to solve.

I'm going to keep this ticket open and think a bit further on how to divide the problem space.

vsespb commented 10 years ago

I think the most promising path to solving this problem is to create such a module, then use the output to look at the context of each occurrence within the same scope

another idea. if we can find all assigments to particular variable, then:

$y = "$a $b $c"; # SQL Safe ($a, $c)
$sql = "select $x $y $z"; ## SQL Safe ($x, $z)

current behaviour: will warn about $y unsafe. expected behaviour: warn about $b unsafe.

(that should be recursive)

My real use case: very often (in more than 50% case) I have the following code:

$sql = "select abc from table $where_cond"

and $where_cond is a variable, with many many assigments and modifications above this line. so most work to determine if sql safe or no is to determine how safe $where_cond is. after that determining how safe $sql is is trivial, thus SQL Safe pragmas wanted in assigments to $where_cond actually.