Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

detect sql string modification #11

Open
vsespb opened this issue Mar 15, 2014 · 2 comments
Open

detect sql string modification #11

vsespb opened this issue Mar 15, 2014 · 2 comments

Comments

@vsespb
Copy link
Contributor

vsespb commented Mar 15, 2014

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
Copy link
Owner

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
Copy link
Contributor Author

vsespb commented Mar 18, 2014

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants