Skip Menu |

This queue is for tickets about the CGI-Session CPAN distribution.

Report information
The Basics
Id: 18578
Status: resolved
Priority: 0/
Queue: CGI-Session

People
Owner: MARKSTOS [...] cpan.org
Requestors: dmuey [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: (no value)
Fixed in: (no value)



Subject: Possible SQL injection attack
sub remove { my $self = shift; my ($sid) = @_; croak "remove(): usage error" unless $sid; my $dbh = $self->{Handle}; my $sql = sprintf("DELETE FROM %s WHERE id='%s'", $self->table_name, $sid); unless ( $dbh->do($sql) ) { croak "remove(): \$dbh->do failed!"; } return 1; } woudl be better done with place holders or $dbh->quote (and maybe the return value could be more more meaningful and safe) sub remove { my $self = shift; my ($sid) = @_; croak "remove(): usage error" unless $sid; my $sql = sprintf("); my $rc = $self->{Handle}->do( 'DELETE FROM' . $self->{Handle}->quote($self->table_name) . ' WHERE id=' . $self->{Handle}->quote($sid) ); # do(DELETE) usually returns 0 even thouh it worked do or croak() could cause problems... croak "remove(): \$dbh->do failed!" if $rc eq '0E0'; # maybe not croak and return ?? return $rc eq 'E0E' ? 0 : 1; }
Oi! this should preserve indentation, the code looks horrid :)
Subject: Re: [rt.cpan.org #18578] Possible SQL injection attack
Date: Fri, 7 Apr 2006 08:34:15 -0400
To: Guest via RT <bug-CGI-Session [...] rt.cpan.org>
From: Mark Stosberg <mark [...] summersault.com>
On Thu, Apr 06, 2006 at 05:54:07PM -0400, Guest via RT wrote: Show quoted text
> > woudl be better done with place holders or $dbh->quote (and maybe the > return value could be more more meaningful and safe) > > sub remove { > my $self = shift; > my ($sid) = @_; > croak "remove(): usage error" unless $sid; > > my $sql = sprintf("); > my $rc = $self->{Handle}->do( > 'DELETE FROM' > . $self->{Handle}->quote($self->table_name) > . ' WHERE id=' > . $self->{Handle}->quote($sid) > ); > > # do(DELETE) usually returns 0 even thouh it worked do or croak() > could cause problems... > croak "remove(): \$dbh->do failed!" if $rc eq '0E0'; > > # maybe not croak and return ?? > return $rc eq 'E0E' ? 0 : 1; > }
Good idea. We'll work something like this in. I think the 'id' is the field to be concerned most about, as the table name is controlled by the module user. Mark
Show quoted text
> Good idea. We'll work something like this in. > > I think the 'id' is the field to be concerned most about, as the table > name is controlled by the module user.
It'd still be best *and* safer to properly quote/placeholderize *all* data. If you don't there is always a chance it could be exploited, so why not do it right consistently and 100% gaurantee that there is no way to do an SQL injection, even if you tried :) If not, then its a good idea not to use it at all.
Show quoted text
> Good idea. We'll work something like this in. > > I think the 'id' is the field to be concerned most about, as the table > name is controlled by the module user.
It'd still be best *and* safer to properly quote/placeholderize *all* data. If you don't there is always a chance it could be exploited, so why not do it right consistently and 100% gaurantee that there is no way to do an SQL injection, even if the module user does something stupid that lets an attacker control the table name. If not, then its a good idea not to use the module at all and use one that is safe.
Subject: Re: [rt.cpan.org #18578] Possible SQL injection attack
Date: Fri, 7 Apr 2006 17:34:18 -0400
To: Guest via RT <bug-CGI-Session [...] rt.cpan.org>
From: Mark Stosberg <mark [...] summersault.com>
On Fri, Apr 07, 2006 at 05:05:09PM -0400, Guest via RT wrote: Show quoted text
> > Queue: CGI-Session > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=18578 > > >
> > Good idea. We'll work something like this in. > > > > I think the 'id' is the field to be concerned most about, as the table > > name is controlled by the module user.
> > It'd still be best *and* safer to properly quote/placeholderize *all* data.
Not necessarily. I did run the table name through quote() in this case because it passed by tests, but I wasn't sure that it would. This is not valid in PostgreSQL: SELECT * from 'table_name'; That's a case when having quotes around something doesn't work. Bind variables don't work for every kind of SQL construction, and and quoting() can be mis-used. Mark
On Fri Apr 07 17:34:42 2006, mark@summersault.com wrote: Show quoted text
> On Fri, Apr 07, 2006 at 05:05:09PM -0400, Guest via RT wrote:
> > > > Queue: CGI-Session > > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=18578 > > > > >
> > > Good idea. We'll work something like this in. > > > > > > I think the 'id' is the field to be concerned most about, as the
> table
> > > name is controlled by the module user.
> > > > It'd still be best *and* safer to properly quote/placeholderize
> *all* data. > > Not necessarily. I did run the table name through quote() in this case > because it passed by tests, but I wasn't sure that it would. This is > not > valid in PostgreSQL: > > SELECT * from 'table_name'; > > That's a case when having quotes around something doesn't work. Bind > variables don't work for every kind of SQL construction, and and > quoting() can be mis-used.
Hmm, bummer. Maybe at least big huge red warning to make sure their table name is only specified by a rigourously hardcoded/verified\/etc piece of data due to Postgres's lameness. Or do it right by default and let the Postgres driver specify their own "not so safe" remove().
Subject: RE: [rt.cpan.org #18578] Possible SQL injection attack
Date: Sun, 9 Apr 2006 03:42:27 -0400
To: <bug-CGI-Session [...] rt.cpan.org>
From: "Sherzod Ruzmetov" <sherzodr [...] handalak.com>
When table names are quoted, most of the DBI tests failed on my computer. Should we switch the code back to its previous state? Show quoted text
> -----Original Message----- > From: Guest via RT [mailto:bug-CGI-Session@rt.cpan.org] > Sent: Thursday, April 06, 2006 5:54 PM > To: undisclosed-recipients: > Subject: [rt.cpan.org #18578] Possible SQL injection attack > > > > Thu Apr 06 17:54:06 2006: Request 18578 was acted upon. > Transaction: Ticket created by guest > Queue: CGI-Session > Subject: Possible SQL injection attack > Owner: Nobody > Requestors: DMUEY@cpan.org > Status: new > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=18578 > > > > sub remove { > my $self = shift; > my ($sid) = @_; > croak "remove(): usage error" unless $sid; > > my $dbh = $self->{Handle}; > my $sql = sprintf("DELETE FROM %s WHERE id='%s'", > $self->table_name, $sid); > unless ( $dbh->do($sql) ) { > croak "remove(): \$dbh->do failed!"; > } > > return 1; > } > > woudl be better done with place holders or $dbh->quote (and > maybe the return value could be more more meaningful and safe) > > sub remove { > my $self = shift; > my ($sid) = @_; > croak "remove(): usage error" unless $sid; > > my $sql = sprintf("); > my $rc = $self->{Handle}->do( > 'DELETE FROM' > . $self->{Handle}->quote($self->table_name) > . ' WHERE id=' > . $self->{Handle}->quote($sid) > ); > > # do(DELETE) usually returns 0 even thouh it worked do or > croak() could cause problems... > croak "remove(): \$dbh->do failed!" if $rc eq '0E0'; > > # maybe not croak and return ?? > return $rc eq 'E0E' ? 0 : 1; > } > >
CC: undisclosed-recipients: ;
Subject: Re: [rt.cpan.org #18578] Possible SQL injection attack
Date: Mon, 10 Apr 2006 09:53:27 -0400
To: "sherzodr [...] handalak.com via RT" <bug-CGI-Session [...] rt.cpan.org>
From: Mark Stosberg <mark [...] summersault.com>
On Sun, Apr 09, 2006 at 03:42:57AM -0400, sherzodr@handalak.com via RT wrote: Show quoted text
> > Queue: CGI-Session > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=18578 > > > When table names are quoted, most of the DBI tests failed on my computer. > Should we switch the code back to its previous state?
I've made these kind of improvements now, which is to trust the table name, but use a bind variable for the value. That should be portable. However, I'm seeing a lot of test failures, and I don't have time at the moment to track down if they are related to this or something else. Sherzod, if you could do a svn checkout and take a look, that would be great. Using bind variables is certianly portable. Mark
Subject: Re: [rt.cpan.org #18578] Possible SQL injection attack
Date: Mon, 10 Apr 2006 09:55:29 -0400
To: Guest via RT <bug-CGI-Session [...] rt.cpan.org>
From: Mark Stosberg <mark [...] summersault.com>
On Sat, Apr 08, 2006 at 06:37:33PM -0400, Guest via RT wrote: Show quoted text
>
> > SELECT * from 'table_name'; > > > > That's a case when having quotes around something doesn't work. Bind > > variables don't work for every kind of SQL construction, and and > > quoting() can be mis-used.
> > > Hmm, bummer. Maybe at least big huge red warning to make sure their > table name is only specified by a rigourously hardcoded/verified\/etc > piece of data due to Postgres's lameness. Or do it right by default and > let the Postgres driver specify their own "not so safe" remove().
Could you point me to a reference that shows that PostgreSQL behavior here is non-standard? As far as I'm aware using single quotes to denote a string constant is SQL-standard behavior. I would be interested to clarify the matter. You could always put your MySQL server into the SQL-compliant mode and see what happens! Mark
Subject: Re: [rt.cpan.org #18578] Possible SQL injection attack
Date: Mon, 10 Apr 2006 09:25:14 -0500
To: bug-CGI-Session [...] rt.cpan.org
From: SimpleMood Webmaster <webmaster [...] simplemood.com>
On Apr 10, 2006, at 8:55 AM, mark@summersault.com via RT wrote: Show quoted text
> > <URL: http://rt.cpan.org/Ticket/Display.html?id=18578 > > > On Sat, Apr 08, 2006 at 06:37:33PM -0400, Guest via RT wrote:
>>
>>> SELECT * from 'table_name'; >>> >>> That's a case when having quotes around something doesn't work. Bind >>> variables don't work for every kind of SQL construction, and and >>> quoting() can be mis-used.
>> >> >> Hmm, bummer. Maybe at least big huge red warning to make sure their >> table name is only specified by a rigourously hardcoded/verified\/etc >> piece of data due to Postgres's lameness. Or do it right by >> default and >> let the Postgres driver specify their own "not so safe" remove().
> > Could you point me to a reference that shows that PostgreSQL behavior > here is non-standard? As far as I'm aware using single quotes to > denote
Its possible its standard, I just mentioned the need to advertise vehemently that the table name needs specified by a trusted source since its not quoted. That what I meant by "not so safe" :) Doing that and placeholding or quote()ing the value would be great! Especially since an unquoted table name is portable, unless DBI specified a quote_table() method or possibly: # make the table name safe my $table = $dbh->quote($possible_evil_table_name); # remove surrounding quotes to make it portable SQL: $table =~ s{^[`'"]|[`'"]$}g; # untested, proof of concept regex... Now if they do specify anything but a valid table name no other query will run and it will either do nothing or fail with an error. At least the firey FYI would be great :)
CC: undisclosed-recipients: ;
Subject: Re: [rt.cpan.org #18578] Possible SQL injection attack
Date: Mon, 10 Apr 2006 10:33:06 -0400
To: SimpleMood Webmaster via RT <bug-CGI-Session [...] rt.cpan.org>
From: Mark Stosberg <mark [...] summersault.com>
On Mon, Apr 10, 2006 at 10:26:37AM -0400, SimpleMood Webmaster via RT wrote: Show quoted text
> > Its possible its standard, I just mentioned the need to advertise > vehemently that the table name > needs specified by a trusted source since its not quoted. That what I > meant by "not so safe" :)
I think 'taint' mode would already address this, if someone choses to all the table name to be specified from an unsafe source. the session ID usually comes from the user, so is not safe. The table name is given by the module user-- the one with access to write custom Perl code anyway, so is not 'tainted'. Show quoted text
> Doing that and placeholding or quote()ing the value would be great! > > Especially since an unquoted table name is portable, unless DBI > specified a quote_table() method or possibly: > > # make the table name safe > my $table = $dbh->quote($possible_evil_table_name); > > # remove surrounding quotes to make it portable SQL: > $table =~ s{^[`'"]|[`'"]$}g; # untested, proof of concept regex... > > Now if they do specify anything but a valid table name no other query > will run and it will either do nothing or fail with an error.
That looks like an mis-use of quoting to me that I expect coud cause other problems, because it may modify the table name is a way that might break it. Using double quotes around a table name /is/ valid, and may actually be required if the table name has a space in it. Mark
Subject: Re: [rt.cpan.org #18578] Possible SQL injection attack
Date: Mon, 10 Apr 2006 09:51:45 -0500
To: bug-CGI-Session [...] rt.cpan.org
From: SimpleMood Webmaster <webmaster [...] simplemood.com>
On Apr 10, 2006, at 9:33 AM, mark@summersault.com via RT wrote: Show quoted text
> > <URL: http://rt.cpan.org/Ticket/Display.html?id=18578 > > > On Mon, Apr 10, 2006 at 10:26:37AM -0400, SimpleMood Webmaster via > RT wrote:
>> >> Its possible its standard, I just mentioned the need to advertise >> vehemently that the table name >> needs specified by a trusted source since its not quoted. That what I >> meant by "not so safe" :)
> > I think 'taint' mode would already address this, if someone choses to
True but how many people run taint mode? Show quoted text
> all the table name to be specified from an unsafe source. the > session ID > usually comes from the user, so is not safe. The table name is > given by > the module user-- the one with access to write custom Perl code > anyway, > so is not 'tainted'. > >
>> Doing that and placeholding or quote()ing the value would be great! >> >> Especially since an unquoted table name is portable, unless DBI >> specified a quote_table() method or possibly: >>
[snipped so the unwary don't use it] Show quoted text
>> >> Now if they do specify anything but a valid table name no other query >> will run and it will either do nothing or fail with an error.
> > That looks like an mis-use of quoting to me that I expect coud cause > other problems, because it may modify the table name is a way that > might > break it. Using double quotes around a table name /is/ valid, and may > actually be required if the table name has a space in it.
I agree too much trouble lurking, just kind of brain storming of what would be nice.., looks best just advertise the need to specify it from a trusted source and check it very carefully. (they might set a table based on an ENV variable, a cookie, a param, etc and without taint mode) Yes its their fault for using param('table_name') as the value directly and not running taint mode but at least then they couldn't say you didn't warn them :)