[Koha-patches] Fwd: [PATCH] kohabug 2458 Disallowing non-SELECT SQL in reports module
Joshua Ferraro
jmf at liblime.com
Fri Aug 8 13:11:34 CEST 2008
Hey Chris,
Could you rebase your repo and re-submit this one, it didn't apply
cleanly for me.
Thanks,
Josh
On Wed, Aug 06, 2008 at 09:44:08PM -0400, Chris Nighswonger wrote:
> From: Chris Nighswonger <chris.nighswonger at liblime.com>
>
> 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: Andrew Moore <andrew.moore at liblime.com>
> ---
> C4/Reports.pm | 200 +++++++++++---------
> .../en/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 811a919..9a6de77 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 ('</th><th>',@$colnames);
> - $row = "<tr><th>$row</th></tr>";
> - $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( '</td><td>', @data );
> - $row = "<tr><td>$row</td></tr>";
> + $row = join ('</th><th>',@$colnames);
> + $row = "<tr><th>$row</th></tr>";
> $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", at data);
> - $string .="\n" . $row;
> + if ($format eq 'csv') {
> + $string = join(",",@$colnames);
> }
> - if ($format eq 'csv' ){
> - $row = join(",", at 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( '</td><td>', @data );
> + $row = "<tr><td>$row</td></tr>";
> + $temphash{'row'} = $row;
> + if ( $format eq 'text' ) {
> + $string .= "\n" . $row;
> + }
> + if ($format eq 'tab' ){
> + $row = join("\t", at data);
> + $string .="\n" . $row;
> + }
> + if ($format eq 'csv' ){
> + $row = join(",", at 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 8bf572a..38fa9af 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" -->"><!-- TMPL_VAR
> NAME="name"--></label></td><td>
> <!-- TMPL_IF NAME="execute" -->
> <h1><!-- TMPL_VAR NAME="name" --></h1>
> <p><!-- TMPL_VAR NAME="notes" --></p>
> -<!-- TMPL_VAR name='pagination_bar'-->
> +<!-- TMPL_IF name="pagination_bar" --><!-- TMPL_VAR
> name='pagination_bar'--><!-- /TMPL_IF -->
> +<!-- TMPL_UNLESS name="errors" -->
> <table>
> <!-- TMPL_LOOP NAME="results" -->
> <!-- TMPL_VAR NAME="row" -->
> @@ -376,6 +377,21 @@ NAME="name" -->"><!-- TMPL_VAR
> NAME="name"--></label></td><td>
> <input type="hidden" name="phase" value="Export" />
> <input type="submit" name="submit" value="Download" /></fieldset>
> </form>
> +<!-- /TMPL_UNLESS -->
> +<!-- TMPL_IF NAME="errors" -->
> +<form action="/cgi-bin/koha/reports/guided_reports.pl" method="post">
> +<div class="dialog alert">
> +<b>The following error was encountered:</b><br />
> +<!-- TMPL_LOOP name="errors" -->
> +<!-- TMPL_IF NAME="sqlerr" -->This report contains the SQL keyword
> <!-- TMPL_VAR name="sqlerr" -->.<br />Use of this keyword is not
> allowed in Koha reports due to security and data integrity risks.<br
> />Please return to the "Saved Reports" screen and delete this report.
> +<!-- TMPL_ELSIF NAME="queryerr" -->The database returned the
> following error: <br /><!-- TMPL_VAR NAME="queryerr" --><br />Please
> check the log for further details.
> +<!-- /TMPL_IF -->
> +<!-- /TMPL_LOOP -->
> +</div>
> +<fieldset class="action"><input type="hidden" name="phase"
> value="Used saved" />
> +<input type="submit" name="submit" value="Saved Reports" /></fieldset>
> +</form>
> +<!-- /TMPL_IF -->
> <!-- /TMPL_IF -->
>
> <!-- TMPL_IF NAME="create" -->
> @@ -383,10 +399,10 @@ NAME="name" -->"><!-- TMPL_VAR
> NAME="name"--></label></td><td>
> <fieldset class="rows">
> <legend>Create Report From SQL</legend>
> <ol>
> - <li><label for="reportname">Report Name:</label> <input
> type="text" id="reportname" name="reportname" /> </li>
> - <li><label for="notes">Notes:</label> <textarea id="notes"
> name="notes" cols="50" rows="2"></textarea></li>
> - <li><label for="types">Type:</label><select id="types"
> name="types"><!-- TMPL_LOOP NAME="types" --><option value="<!--
> TMPL_VAR NAME="id" -->"><!-- TMPL_VAR NAME="name"--></option><!--
> /TMPL_LOOP --></select></li>
> - <li><label for="sql">SQL: </label><textarea id="sql"
> name="sql" cols="50" rows="10"></textarea></li>
> + <li><label for="reportname">Report Name:</label> <input
> type="text" id="reportname" name="reportname"<!--TMPL_IF
> NAME="reportname" --> value="<!-- TMPL_VAR NAME="reportname"-->"<!--
> /TMPL_IF --> /> </li>
> + <li><label for="notes">Notes:</label> <textarea id="notes"
> name="notes" cols="50" rows="2"><!--TMPL_IF NAME="notes" --><!--
> TMPL_VAR NAME="notes"--><!-- /TMPL_IF --></textarea></li>
> + <li><label for="types">Type:</label><select id="types"
> name="types"><!-- TMPL_LOOP NAME="types" --><option value="<!--
> TMPL_VAR NAME="id" -->"<!-- TMPL_IF NAME="selected" -->
> selected="selected"<!-- /TMPL_IF -->><!-- TMPL_VAR
> NAME="name"--></option><!-- /TMPL_LOOP --></select></li>
> + <li><label for="sql">SQL: </label><textarea id="sql"
> name="sql" cols="50" rows="10"><!--TMPL_IF NAME="sql" --><!-- TMPL_VAR
> NAME="sql"--><!-- /TMPL_IF --></textarea></li>
> </ol>
> </fieldset>
>
> @@ -437,6 +453,7 @@ Sub report:<select name="subreport">
> <!-- /TMPL_IF -->
>
> <!-- TMPL_IF NAME="save_successful" -->
> +<!-- TMPL_UNLESS NAME="errors" -->
> <h2>Your report has been saved</h2>
> <p>The report you have created has now been saved. You can now</p>
> <ul>
> @@ -444,9 +461,25 @@ Sub report:<select name="subreport">
> <li>Schedule this report to run using the: <a
> href="/cgi-bin/koha/tools/scheduler.pl">Scheduler Tool</a></li>
> <li>Return to: <a
> href="/cgi-bin/koha/reports/guided_reports.pl?phase=Used+saved">Guided
> Reports</a></li>
> </ul>
> -
> -
> -
> +<!-- /TMPL_UNLESS -->
> +<!-- TMPL_IF NAME="errors" -->
> +<form action="/cgi-bin/koha/reports/guided_reports.pl" method="post">
> +<div class="dialog alert">
> +<b>The following error was encountered:</b><br />
> +<!-- TMPL_LOOP name="errors" -->
> +<!-- TMPL_IF NAME="sqlerr" -->The SQL statement contains the keyword
> <!-- TMPL_VAR name="sqlerr" -->. Use of this keyword is not allowed in
> Koha reports due to security and data integrity risks.
> +<!-- TMPL_ELSIF NAME="queryerr" -->The SQL statment is missing the
> SELECT keyword.<br />Please check the SQL statement syntax.
> +<!-- /TMPL_IF -->
> +<!-- /TMPL_LOOP -->
> +</div>
> +<input type="hidden" name="sql" value="<!-- TMPL_VAR NAME="sql" -->" />
> +<input type="hidden" name="reportname" value="<!-- TMPL_VAR
> NAME="reportname" -->" />
> +<input type="hidden" name="type" value="<!-- TMPL_VAR NAME="type" -->" />
> +<input type="hidden" name="notes" value="<!-- TMPL_VAR NAME="notes" -->" />
> +<fieldset class="action"><input type="hidden" name="phase"
> value="Create report from SQL" />
> +<input type="submit" name="submit" value="Edit SQL" /></fieldset>
> +</form>
> +<!-- /TMPL_IF -->
> <!-- /TMPL_IF -->
>
> </div>
> diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl
> index b7cb46d..9d80314 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 );
> }
>
> --
> 1.5.6
> _______________________________________________
> Koha-patches mailing list
> Koha-patches at lists.koha.org
> http://lists.koha.org/mailman/listinfo/koha-patches
More information about the Koha-patches
mailing list