Bug #50864 for TraitFor-Catalyst-Controller-DBIC-DoesPaging: SQL injection possible

Tue Oct 27 06:57:09 2009 GRAF [...] cpan.org - Ticket created

Subject:

SQL injection possible

I didn't actually tried this package, but skimming over the source code I would say that it is prone for SQL injection attacks, because it embedds HTTP parameter values directly into SQL code. Moreover code like this foreach ( keys %{ $c->request->params } ) { ... $searches->{$_} = { like => q{%} . $c->request->params->{$_} . q{%} }; } fails, if $c->request->params->{$_} is an array reference, which happens when a HTTP key is used more than once: ?term=x&term=y

Tue Oct 27 10:15:11 2009 frioux [...] gmail.com - Correspondence added

On Tue Oct 27 06:57:09 2009, GRAF wrote: Show quoted text

> I didn't actually tried this package, but skimming over the source code > I would say that it is prone for SQL injection attacks, because it > embedds HTTP parameter values directly into SQL code.

That's not true. DBIx::Class automatically uses SQL placeholders for all values. Show quoted text

> Moreover code like this > > foreach ( keys %{ $c->request->params } ) { > ... > $searches->{$_} = { like => q{%} . $c->request->params->{$_} . q{%} }; > } > > fails, if $c->request->params->{$_} is an array reference, which happens > when a HTTP key is used more than once: > > ?term=x&term=y

That's true, that would indeed fail. I will fix that tonight. Thanks for the RT!

Tue Oct 27 10:15:12 2009 The RT System itself - Status changed from 'new' to 'open'

Tue Oct 27 12:53:12 2009 GRAF [...] cpan.org - Correspondence added

On Di. 27. Okt. 2009, 10:15:11, frew wrote: Show quoted text

> On Tue Oct 27 06:57:09 2009, GRAF wrote:

> > I didn't actually tried this package, but skimming over the source code > > I would say that it is prone for SQL injection attacks, because it > > embedds HTTP parameter values directly into SQL code.

> > That's not true. DBIx::Class automatically uses SQL placeholders for > all values.

Ah, ok. Show quoted text

> > Moreover code like this > > > > foreach ( keys %{ $c->request->params } ) { > > ... > > $searches->{$_} = { like => q{%} . $c->request->params->{$_} . q{%} }; > > } > > > > fails, if $c->request->params->{$_} is an array reference, which happens > > when a HTTP key is used more than once: > > > > ?term=x&term=y

> > That's true, that would indeed fail. I will fix that tonight. Thanks > for the RT!

What about masking SQL wildchars, BTW? That affects at least '%' and '_' - maybe some SQL dialects support even more - I dunno. Imagine $c->request->params->{$_} is a '%'... Is masking always done with a backslash? Or does DBIC have a engine-independent masking method?

Tue Oct 27 13:17:43 2009 frioux [...] gmail.com - Correspondence added

Show quoted text

> What about masking SQL wildchars, BTW? > That affects at least '%' and '_' - maybe some SQL dialects support even > more - I dunno. > Imagine $c->request->params->{$_} is a '%'... > Is masking always done with a backslash? > Or does DBIC have a engine-independent masking method?

I don't really know. I haven't thought that through at all. Our current customer actually tells their users how to use % and _ so that they can use them in a search, so really masking should be option. I'd say if you want to get complex, don't use simple_search. It's really made just to get you started. It also won't work for anything that's not a string field. This also makes sense for the original arrayref issue as well. The only time someone will be doing that is if you set up a form for that on purpose, and in that case you'll want to set up a resultset search for that. I'll still fix it so that it will do the right thing as far as simple_search goes, but it's a pretty uncommon use case.

Wed Oct 28 01:27:40 2009 frioux [...] gmail.com - Correspondence added

The multiple values per cgi parameter issue should be fixed in the latest (0.093010) version.

Wed Oct 28 01:27:42 2009 frioux [...] gmail.com - Status changed from 'open' to 'resolved'