From 6c2b2d54a8cec60ea320a27e52dde620d37a25ee Mon Sep 17 00:00:00 2001 From: Sanjar Tulkinov Anvar o'g'li Date: Thu, 30 Jul 2026 04:47:43 -0400 Subject: [PATCH] Bug 42736: Prevent SQL injection in reports/cat_issues_top.pl The "Most-circulated items" report built its SQL by concatenating the Criteria and Filter request parameters directly into the query, with no bind placeholders and no whitelist on the Criteria identifier. An authenticated staff user with the reports permission could inject arbitrary SQL and read any data reachable by the Koha database user. This validates the Criteria parameter against a fixed allow-list of the report's own column options (matching reports/cat_issues_top.tt) so it is safe to use as a SQL identifier, and binds every Filter value with a placeholder in both the column-header query and the main calculation query. The Limit parameter is validated as a plain integer. This mirrors the fix already applied to the sibling reports/bor_issues_top.pl (bug 42369). Test plan: 0. Apply the patch 1. koha-plack --restart kohadev 2. Go to Reports > Most-circulated items (reports/cat_issues_top.pl) 3. Run the report with various Criteria and Filter values, confirm the output is correct and no errors appear 4. Before the patch, a Filter value or the Criteria containing SQL (a single quote followed by a subquery) broke out of the query. After the patch, confirm such input is treated as a literal value: you get normal output or an empty result, with no SQL error, no delay, and no data leak Signed-off-by: Sanjar Tulkinov Anvar o'g'li Signed-off-by: David Cook Signed-off-by: Aleisha Amohia (cherry picked from commit b6cdb398237ddfc3d3da97a0246c5a3a69fdaf9b) Signed-off-by: Fridolin Somers --- reports/cat_issues_top.pl | 110 +++++++++++++++++++++++++++++--------- 1 file changed, 84 insertions(+), 26 deletions(-) diff --git a/reports/cat_issues_top.pl b/reports/cat_issues_top.pl index 23068f9b16..195cd262d4 100755 --- a/reports/cat_issues_top.pl +++ b/reports/cat_issues_top.pl @@ -37,10 +37,33 @@ plugin that shows a stats on borrowers my $input = CGI->new; my $do_it = $input->param('do_it'); my $fullreportname = "reports/cat_issues_top.tt"; -my $limit = $input->param("Limit"); -my $column = $input->param("Criteria"); +my $limit; +my $limit_input = $input->param("Limit"); +my $column; +my $column_input = $input->param("Criteria"); my @filters = $input->multi_param("Filter"); +# Allowed values for the Criteria parameter, matching the report's own +# select options in reports/cat_issues_top.tt. Criteria is used unquoted +# as a SQL identifier, so it must be validated against a fixed set. +my %allowed_fields = ( + "branch" => 1, + "categorycode" => 1, + "itemtype" => 1, + "Day" => 1, + "Week" => 1, + "Month" => 1, + "Year" => 1, +); +if ( $column_input && $allowed_fields{$column_input} ) { + $column = $column_input; +} + +# Limit is a numeric row count only +if ( $limit_input && $limit_input =~ /^\d+$/ ) { + $limit = $limit_input; +} + my $output = $input->param("output"); my $basename = $input->param("basename"); @@ -248,6 +271,7 @@ sub calculate { $colorder .= $column; } + my @bind2 = (); my $strsth2; $strsth2 .= "SELECT distinctrow $colfield FROM `old_issues` @@ -258,26 +282,24 @@ sub calculate { if ( ( $column =~ /issuedate/ ) or ( $column =~ /returndate/ ) ) { if ( $colfilter[1] and ( $colfilter[0] ) ) { $strsth2 .= " and $column between ? and ? "; + push( @bind2, $colfilter[0], $colfilter[1] ); } elsif ( $colfilter[1] ) { $strsth2 .= " and $column < ? "; + push( @bind2, $colfilter[1] ); } elsif ( $colfilter[0] ) { $strsth2 .= " and $column > ? "; + push( @bind2, $colfilter[0] ); } } elsif ( $colfilter[0] ) { $colfilter[0] =~ s/\*/%/g; $strsth2 .= " and $column LIKE ? "; + push( @bind2, $colfilter[0] ); } $strsth2 .= " group by $colfield"; $strsth2 .= " order by $colorder"; my $sth2 = $dbh->prepare($strsth2); - if ( (@colfilter) and ( $colfilter[1] ) ) { - $sth2->execute( "'" . $colfilter[0] . "'", "'" . $colfilter[1] . "'" ); - } elsif ( $colfilter[0] ) { - $sth2->execute( $colfilter[0] ); - } else { - $sth2->execute; - } + $sth2->execute(@bind2); while ( my ($celvalue) = $sth2->fetchrow ) { my %cell; @@ -314,40 +336,76 @@ sub calculate { LEFT JOIN borrowers USING(borrowernumber) WHERE 1"; + my @bind3 = (); @$filters[0] =~ s/\*/%/g if ( @$filters[0] ); - $strcalc .= " AND old_issues.issuedate > '" . @$filters[0] . "'" if ( @$filters[0] ); + if ( @$filters[0] ) { + $strcalc .= " AND old_issues.issuedate > ?"; + push( @bind3, @$filters[0] ); + } @$filters[1] =~ s/\*/%/g if ( @$filters[1] ); - $strcalc .= " AND old_issues.issuedate < '" . @$filters[1] . "'" if ( @$filters[1] ); + if ( @$filters[1] ) { + $strcalc .= " AND old_issues.issuedate < ?"; + push( @bind3, @$filters[1] ); + } @$filters[2] =~ s/\*/%/g if ( @$filters[2] ); - $strcalc .= " AND old_issues.returndate > '" . @$filters[2] . "'" if ( @$filters[2] ); + if ( @$filters[2] ) { + $strcalc .= " AND old_issues.returndate > ?"; + push( @bind3, @$filters[2] ); + } @$filters[3] =~ s/\*/%/g if ( @$filters[3] ); - $strcalc .= " AND old_issues.returndate < '" . @$filters[3] . "'" if ( @$filters[3] ); + if ( @$filters[3] ) { + $strcalc .= " AND old_issues.returndate < ?"; + push( @bind3, @$filters[3] ); + } @$filters[4] =~ s/\*/%/g if ( @$filters[4] ); - $strcalc .= " AND old_issues.branchcode like '" . @$filters[4] . "'" if ( @$filters[4] ); + if ( @$filters[4] ) { + $strcalc .= " AND old_issues.branchcode like ?"; + push( @bind3, @$filters[4] ); + } @$filters[5] =~ s/\*/%/g if ( @$filters[5] ); - if ( @$filters[5] ) { if ( C4::Context->preference('item-level_itypes') ) { - $strcalc .= " AND items.itype like "; + $strcalc .= " AND items.itype like ?"; } else { - $strcalc .= " AND biblioitems.itemtype like "; + $strcalc .= " AND biblioitems.itemtype like ?"; } - $strcalc .= "'" . @$filters[5] . "'"; + push( @bind3, @$filters[5] ); } @$filters[6] =~ s/\*/%/g if ( @$filters[6] ); - $strcalc .= " AND itemcallnumber like '" . @$filters[6] . "'" if ( @$filters[6] ); + if ( @$filters[6] ) { + $strcalc .= " AND itemcallnumber like ?"; + push( @bind3, @$filters[6] ); + } @$filters[7] =~ s/\*/%/g if ( @$filters[7] ); - $strcalc .= " AND ccode like '" . @$filters[7] . "'" if ( @$filters[7] ); + if ( @$filters[7] ) { + $strcalc .= " AND ccode like ?"; + push( @bind3, @$filters[7] ); + } @$filters[8] =~ s/\*/%/g if ( @$filters[8] ); - $strcalc .= " AND location like '" . @$filters[8] . "'" if ( @$filters[8] ); + if ( @$filters[8] ) { + $strcalc .= " AND location like ?"; + push( @bind3, @$filters[8] ); + } @$filters[9] =~ s/\*/%/g if ( @$filters[9] ); - $strcalc .= " AND borrowers.categorycode like '" . @$filters[9] . "'" if ( @$filters[9] ); + if ( @$filters[9] ) { + $strcalc .= " AND borrowers.categorycode like ?"; + push( @bind3, @$filters[9] ); + } @$filters[10] =~ s/\*/%/g if ( @$filters[10] ); - $strcalc .= " AND dayname(old_issues.issuedate) like '" . @$filters[10] . "'" if ( @$filters[10] ); + if ( @$filters[10] ) { + $strcalc .= " AND dayname(old_issues.issuedate) like ?"; + push( @bind3, @$filters[10] ); + } @$filters[11] =~ s/\*/%/g if ( @$filters[11] ); - $strcalc .= " AND monthname(old_issues.issuedate) like '" . @$filters[11] . "'" if ( @$filters[11] ); + if ( @$filters[11] ) { + $strcalc .= " AND monthname(old_issues.issuedate) like ?"; + push( @bind3, @$filters[11] ); + } @$filters[12] =~ s/\*/%/g if ( @$filters[12] ); - $strcalc .= " AND year(old_issues.issuedate) like '" . @$filters[12] . "'" if ( @$filters[12] ); + if ( @$filters[12] ) { + $strcalc .= " AND year(old_issues.issuedate) like ?"; + push( @bind3, @$filters[12] ); + } $strcalc .= " group by biblio.biblionumber, biblio.title"; $strcalc .= ", $colfield" if ($column); @@ -355,7 +413,7 @@ sub calculate { $strcalc .= ", $colfield " if ($colfield); my $dbcalc = $dbh->prepare($strcalc); - $dbcalc->execute; + $dbcalc->execute(@bind3); my %indice; while ( my @data = $dbcalc->fetchrow ) { my ( $row, $rank, $id, $col ) = @data;