From 860f1f70e50a247cd12d60762db51a2e05dfd86b Mon Sep 17 00:00:00 2001 From: Chris Nighswonger Date: Fri, 8 Aug 2008 08:22:49 -0500 Subject: [PATCH] kohabug 2458 Disallowing non-SELECT SQL in reports module This patch enforces SELECT-only SQL in the reports module. It introduces code to check SQL in two places. The first is when a save is attempted on a user constructed SQL statement. If a non-SELECT SQL statement is entered, the user will be presented with an error message and a button giving the option of editing the SQL. The second is when any SQL is executed. If execution of a non-SELECT SQL statement is attempted, the user is presented with an error message and instructed to delete that report as the SQL is invalid. The second check is intended as a safety net as no non-SELECT SQL should ever be saved. It may be well to document the proper usage of the direct SQL entry type report. Signed-off-by: Joshua Ferraro --- C4/Reports.pm | 200 ++++++++++-------- .../modules/reports/guided_reports_start.tmpl | 49 ++++- reports/guided_reports.pl | 90 ++++++-- 3 files changed, 224 insertions(+), 115 deletions(-) diff --git a/C4/Reports.pm b/C4/Reports.pm index 811a9191fc..9a6de77155 100644 --- a/C4/Reports.pm +++ b/C4/Reports.pm @@ -335,10 +335,17 @@ sub get_criteria { When passed C<$sql>, this function returns an array ref containing a result set suitably formatted for display in html or for output as a flat file when passed in - C<$format> and C<$id>. Valid values for C<$format> are 'text,' 'tab,' 'csv,' or 'url. - C<$sql>, C<$type>, C<$offset>, and C<$limit> are required parameters. If a valid - C<$format> is passed in, C<$offset> and C<$limit> are ignored for obvious reasons. - A LIMIT specified by the user in a user-supplied SQL query WILL apply in any case. + C<$format> and C<$id>. It also returns the C<$total> records available for the + supplied query. If passed any query other than a SELECT, or if there is a db error, + C<$errors> an array ref is returned containing the error after this manner: + + C<$error->{'sqlerr'}> contains the offending SQL keyword. + C<$error->{'queryerr'}> contains the native db engine error returned for the query. + + Valid values for C<$format> are 'text,' 'tab,' 'csv,' or 'url. C<$sql>, C<$type>, + C<$offset>, and C<$limit> are required parameters. If a valid C<$format> is passed + in, C<$offset> and C<$limit> are ignored for obvious reasons. A LIMIT specified by + the user in a user-supplied SQL query WILL apply in any case. =cut @@ -347,96 +354,117 @@ sub execute_query ($$$$;$$) { my @params; my $total = 0; my ($useroffset, $userlimit); - my $dbh = C4::Context->dbh(); - unless ($format eq 'text' || $format eq 'tab' || $format eq 'csv' || $format eq 'url'){ - # Grab offset/limit from user supplied LIMIT and drop the LIMIT so we can control pagination - if ($sql =~ /LIMIT/i) { - $sql =~ s/LIMIT\W?(\d+)?\,?\W+?(\d+)//ig; - $debug and warn "User has supplied LIMIT\n"; - $useroffset = $1; - $userlimit = $2; - $debug and warn "User supplied offset = $useroffset, limit = $userlimit\n"; - $offset += $useroffset if $useroffset; - # keep track of where we are if there is a user supplied LIMIT - if ( $offset + $limit > $userlimit ) { - $limit = $userlimit - $offset; - } - } - my $countsql = $sql; - $sql .= " LIMIT ?, ?"; - $debug and warn "Passing query with params offset = $offset, limit = $limit\n"; - @params = ($offset, $limit); - # Modify the query passed in to create a count query... (I think this covers all cases -crn) - $countsql =~ s/\bSELECT\W+(?:\w+\W+){1,}?FROM\b|\bSELECT\W\*\WFROM\b/SELECT count(*) FROM /ig; - $debug and warn "original query: $sql\n"; - $debug and warn "count query: $countsql\n"; - my $sth1 = $dbh->prepare($countsql); - $sth1->execute(); - $total = $sth1->fetchrow(); - $debug and warn "total records for this query: $total\n"; - $total = $userlimit if defined($userlimit) and $userlimit < $total; # we will never exceed a user defined LIMIT and... - $userlimit = $total if defined($userlimit) and $userlimit > $total; # we will never exceed the total number of records available to satisfy the query + my @errors = (); + my $error = {}; + my $sqlerr = 0; + if ($sql =~ /;?\W?(UPDATE|DELETE|DROP|INSERT|SHOW|CREATE)\W/i) { + $sqlerr = 1; + $error->{'sqlerr'} = $1; + push @errors, $error; + } elsif ($sql !~ /^(SELECT)/i) { + $sqlerr = 1; + $error->{'queryerr'} = 'Missing SELECT'; + push @errors, $error; } - my $sth = $dbh->prepare($sql); - $sth->execute(@params); - my $colnames=$sth->{'NAME'}; - my @results; - my $row; - my %temphash; - $row = join ('',@$colnames); - $row = "$row"; - $temphash{'row'} = $row; - push @results, \%temphash; - my $string; - if ($format eq 'tab') { - $string = join("\t",@$colnames); - } - if ($format eq 'csv') { - $string = join(",",@$colnames); - } - my @xmlarray; - while ( my @data = $sth->fetchrow_array() ) { - # if the field is a date field, it needs formatting - foreach my $data (@data) { - next unless $data =~ C4::Dates->regexp("iso"); - my $date = C4::Dates->new($data, "iso"); - $data = $date->output(); + if ($sqlerr == 0) { + my $dbh = C4::Context->dbh(); + unless ($format eq 'text' || $format eq 'tab' || $format eq 'csv' || $format eq 'url'){ + # Grab offset/limit from user supplied LIMIT and drop the LIMIT so we can control pagination + if ($sql =~ /LIMIT/i) { + $sql =~ s/LIMIT\W?(\d+)?\,?\W+?(\d+)//ig; + $debug and warn "User has supplied LIMIT\n"; + $useroffset = $1; + $userlimit = $2; + $debug and warn "User supplied offset = $useroffset, limit = $userlimit\n"; + $offset += $useroffset if $useroffset; + # keep track of where we are if there is a user supplied LIMIT + if ( $offset + $limit > $userlimit ) { + $limit = $userlimit - $offset; + } + } + my $countsql = $sql; + $sql .= " LIMIT ?, ?"; + $debug and warn "Passing query with params offset = $offset, limit = $limit\n"; + @params = ($offset, $limit); + # Modify the query passed in to create a count query... (I think this covers all cases -crn) + $countsql =~ s/\bSELECT\W+(?:\w+\W+){1,}?FROM\b|\bSELECT\W\*\WFROM\b/SELECT count(*) FROM /ig; + $debug and warn "original query: $sql\n"; + $debug and warn "count query: $countsql\n"; + my $sth1 = $dbh->prepare($countsql); + $sth1->execute(); + $total = $sth1->fetchrow(); + $debug and warn "total records for this query: $total\n"; + $total = $userlimit if defined($userlimit) and $userlimit < $total; # we will never exceed a user defined LIMIT and... + $userlimit = $total if defined($userlimit) and $userlimit > $total; # we will never exceed the total number of records available to satisfy the query } - # tabular + my $sth = $dbh->prepare($sql); + $sth->execute(@params); + my $colnames=$sth->{'NAME'}; + my @results; + my $row; my %temphash; - my $row = join( '', @data ); - $row = "$row"; + $row = join ('',@$colnames); + $row = "$row"; $temphash{'row'} = $row; - if ( $format eq 'text' ) { - $string .= "\n" . $row; + push @results, \%temphash; + my $string; + if ($format eq 'tab') { + $string = join("\t",@$colnames); } - if ($format eq 'tab' ){ - $row = join("\t",@data); - $string .="\n" . $row; + if ($format eq 'csv') { + $string = join(",",@$colnames); } - if ($format eq 'csv' ){ - $row = join(",",@data); - $string .="\n" . $row; + my @xmlarray; + while ( my @data = $sth->fetchrow_array() ) { + # if the field is a date field, it needs formatting + foreach my $data (@data) { + next unless $data =~ C4::Dates->regexp("iso"); + my $date = C4::Dates->new($data, "iso"); + $data = $date->output(); + } + # tabular + my %temphash; + my $row = join( '', @data ); + $row = "$row"; + $temphash{'row'} = $row; + if ( $format eq 'text' ) { + $string .= "\n" . $row; + } + if ($format eq 'tab' ){ + $row = join("\t",@data); + $string .="\n" . $row; + } + if ($format eq 'csv' ){ + $row = join(",",@data); + $string .="\n" . $row; + } + if ($format eq 'url'){ + my $temphash; + @$temphash{@$colnames}=@data; + push @xmlarray,$temphash; + } + push @results, \%temphash; } - if ($format eq 'url'){ - my $temphash; - @$temphash{@$colnames}=@data; - push @xmlarray,$temphash; + if (defined($sth->errstr)) { + $error->{'queryerr'} = $sth->errstr; + push @errors, $error; + warn "Database returned: $sth->errstr"; } - push @results, \%temphash; - } - if ( $format eq 'text' || $format eq 'tab' || $format eq 'csv' ) { - return $string; - } - elsif ($format eq 'url') { - my $url = "/cgi-bin/koha/reports/guided_reports.pl?phase=retrieve%20results&id=$id"; - my $dump = new XML::Dumper; - my $xml = $dump->pl2xml( \@xmlarray ); - store_results($id,$xml); - return $url; - } - else { - return ( \@results, $total ); + if ( $format eq 'text' || $format eq 'tab' || $format eq 'csv' ) { + return $string, $total, \@errors; + } + elsif ($format eq 'url') { + my $url = "/cgi-bin/koha/reports/guided_reports.pl?phase=retrieve%20results&id=$id"; + my $dump = new XML::Dumper; + my $xml = $dump->pl2xml( \@xmlarray ); + store_results($id,$xml); + return $url, $total, \@errors; + } + else { + return \@results, $total, \@errors; + } + } else { + return undef, undef, \@errors; } } diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tmpl b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tmpl index 8bf572a2de..38fa9af6cc 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tmpl +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/reports/guided_reports_start.tmpl @@ -359,7 +359,8 @@ NAME="name" -->">

- + + @@ -376,6 +377,21 @@ NAME="name" -->">
+ + +
+
+The following error was encountered:
+ +This report contains the SQL keyword .
Use of this keyword is not allowed in Koha reports due to security and data integrity risks.
Please return to the "Saved Reports" screen and delete this report. +The database returned the following error:

Please check the log for further details. + + +
+
+
+
+ @@ -383,10 +399,10 @@ NAME="name" -->">
Create Report From SQL
    -
    2. -
    3. -
    4. -
    5. +
  5. value="" />
    6. +
    7. +
    8. +
@@ -437,6 +453,7 @@ Sub report:
  • Schedule this report to run using the: Scheduler Tool
  • Return to: Guided Reports
    • - - - + + +
    +
    +The following error was encountered:
    + +The SQL statement contains the keyword . Use of this keyword is not allowed in Koha reports due to security and data integrity risks. +The SQL statment is missing the SELECT keyword.
    Please check the SQL statement syntax. + + +
    +" /> +" /> +" /> +" /> +
    +
    +
    + diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl index b7cb46d3bc..9d80314876 100755 --- a/reports/guided_reports.pl +++ b/reports/guided_reports.pl @@ -323,12 +323,34 @@ elsif ( $phase eq 'Save Report' ) { # save the sql pasted in by a user my $sql = $input->param('sql'); my $name = $input->param('reportname'); - my $type = $input->param('type'); - my $notes = $input->param('notes'); - save_report( $sql, $name, $type, $notes ); - $template->param( - 'save_successful' => 1, - ); + my $type = $input->param('types'); + my $notes = $input->param('notes'); + my @errors = (); + my $error = {}; + if ($sql =~ /;?\W?(UPDATE|DELETE|DROP|INSERT|SHOW|CREATE)\W/i) { + $error->{'sqlerr'} = $1; + push @errors, $error; + } + elsif ($sql !~ /^(SELECT)/i) { + $error->{'queryerr'} = 1; + push @errors, $error; + } + if (@errors) { + $template->param( + 'save_successful' => 1, + 'errors' => \@errors, + 'sql' => $sql, + 'reportname'=> $name, + 'type' => $type, + 'notes' => $notes, + ); + } + else { + save_report( $sql, $name, $type, $notes ); + $template->param( + 'save_successful' => 1, + ); + } } # This condition is not used currently @@ -336,7 +358,7 @@ elsif ( $phase eq 'Save Report' ) { # # run the sql, and output results in a template # my $sql = $input->param('sql'); # my $type = $input->param('type'); -# my $results = execute_query($sql, $type); +# my ($results, $total, $errors) = execute_query($sql, $type); # $template->param( # 'results' => $results, # 'sql' => $sql, @@ -346,18 +368,19 @@ elsif ( $phase eq 'Save Report' ) { elsif ($phase eq 'Run this report'){ # execute a saved report - # FIXME: The default limit should not be hardcoded... + # FIXME The default limit should not be hardcoded... my $limit = 20; my $offset; my $report = $input->param('reports'); # offset algorithm if ($input->param('page')) { $offset = ($input->param('page') - 1) * 20; - } else { + } + else { $offset = 0; } my ($sql,$type,$name,$notes) = get_saved_report($report); - my ($results, $total) = execute_query($sql, $type, $offset, $limit); + my ($results, $total, $errors) = execute_query($sql, $type, $offset, $limit); my $totpages = int($total/$limit) + (($total % $limit) > 0 ? 1 : 0); my $url = "/cgi-bin/koha/reports/guided_reports.pl?reports=$report&phase=Run%20this%20report"; $template->param( @@ -367,25 +390,50 @@ elsif ($phase eq 'Run this report'){ 'name' => $name, 'notes' => $notes, 'pagination_bar' => pagination_bar($url, $totpages, $input->param('page'), "page"), + 'errors' => $errors, ); } elsif ($phase eq 'Export'){ # export results to tab separated text - my $sql = $input->param('sql'); - $no_html=1; - print $input->header( -type => 'application/octet-stream', - -attachment=>'reportresults.csv'); - my $format=$input->param('format'); - my $results = execute_query($sql,1,0,0,$format); - print $results; - + my $sql = $input->param('sql'); + my $format = $input->param('format'); + my ($results, $total, $errors) = execute_query($sql,1,0,0,$format); + if (!defined($errors)) { + $no_html=1; + print $input->header( -type => 'application/octet-stream', + -attachment=>'reportresults.csv' + ); + print $results; + } else { + $template->param( + 'results' => $results, + 'sql' => $sql, + 'execute' => 1, + 'name' => 'Error exporting report!', + 'notes' => '', + 'pagination_bar' => '', + 'errors' => $errors, + ); + } } -elsif ($phase eq 'Create report from SQL'){ - # alllow the user to paste in sql +elsif ($phase eq 'Create report from SQL') { + # allow the user to paste in sql + if ($input->param('sql')) { + $template->param( + 'sql' => $input->param('sql'), + 'reportname' => $input->param('reportname'), + 'notes' => $input->param('notes'), + ); + } $template->param('create' => 1); - my $types = C4::Reports::get_report_types(); + my $types = C4::Reports::get_report_types(); + if (my $type = $input->param('type')) { + for my $i ( 0 .. $#{@$types}) { + @$types[$i]->{'selected'} = 1 if @$types[$i]->{'id'} eq $type; + } + } $template->param( 'types' => $types ); } -- 2.39.5