Bug 17900: Fix possible SQL injection in patron cards template editing

To recreate:
/cgi-bin/koha/patroncards/edit-template.pl?op=edit&element_id=23%20and%201%3d2+union+all+select+1,user(),@@version+--%20

Look at the Profile dropdown list.

To fix this problem and to make sure it does not appears anywhere else
in the label and patroncards modules, I have refactored the way the
queries are built in C4::Creators::Lib
Now all of the subroutine takes a hashref in parameters with a 'fields'
and 'filters' parameters.
From these 2 parameters the new internal subroutine _build_query will
build the query and use placeholders.

Test plan:
1/ Make sure you do not recreate the vulnerability with this patch
applied.
2/ With decent data in the labels and patroncards modules, compare all
the different view (undef the New and Manage button groups) with and
without this patch applied.
=> You should not see any differences.

This vulnerability has been reported by MDSec.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/C4/Creators/Lib.pm b/C4/Creators/Lib.pm
index bdf712e..c15b3e7 100644 (file)
--- a/C4/Creators/Lib.pm
+++ b/C4/Creators/Lib.pm
@@ -142,6 +142,27 @@ my $output_formats = [
     {type       => 'csv',       desc    => 'CSV File'},
 ];
 
+sub _build_query {
+    my ( $params, $table ) = @_;
+    my @fields = exists $params->{fields} ? @{ $params->{fields} } : ();
+    my $query = "SELECT " . ( @fields ? join(', ', @fields ) : '*' ) . " FROM $table";
+    my @where_args;
+    if ( exists $params->{filters} ) {
+        $query .= ' WHERE 1 ';
+        while ( my ( $field, $values ) = each %{ $params->{filters} } ) {
+            if ( ref( $values ) ) {
+                $query .= " AND $field IN ( " . ( ('?') x scalar( @$values ) ) . " ) ";
+                push @where_args, @$values;
+            } else {
+                $query .= " AND $field = ? ";
+                push @where_args, $values;
+            }
+        }
+    }
+    $query .= (exists $params->{orderby} ? " ORDER BY $params->{orderby} " : '');
+    return ( $query, @where_args );
+}
+
 =head2 C4::Creators::Lib::get_all_templates()
 
   my $templates = get_all_templates();
@@ -151,13 +172,11 @@ This function returns a reference to a hash containing all templates upon succes
 =cut
 
 sub get_all_templates {
-    my %params = @_;
+    my ( $params ) = @_;
     my @templates = ();
-    my $query = "SELECT " . ($params{'field_list'} ? $params{'field_list'} : '*') . " FROM creator_templates";
-    $query .= ($params{'filter'} ? " WHERE $params{'filter'} " : '');
-    $query .= ($params{'orderby'} ? " ORDER BY $params{'orderby'} " : '');
+    my ( $query, @where_args ) = _build_query( $params, 'creator_templates' );
     my $sth = C4::Context->dbh->prepare($query);
-    $sth->execute();
+    $sth->execute( @where_args );
     if ($sth->err) {
         warn sprintf('Database returned the following error: %s', $sth->errstr);
         return -1;
@@ -178,13 +197,11 @@ This function returns a reference to a hash containing all layouts upon success
 =cut
 
 sub get_all_layouts {
-    my %params = @_;
+    my ( $params ) = @_;
     my @layouts = ();
-    my $query = "SELECT " . ($params{'field_list'} ? $params{'field_list'} : '*') . " FROM creator_layouts";
-    $query .= ($params{'filter'} ? " WHERE $params{'filter'} " : '');
-    $query .= ($params{'orderby'} ? " ORDER BY $params{'orderby'} " : '');
+    my ( $query, @where_args ) = _build_query( $params, 'creator_layouts' );
     my $sth = C4::Context->dbh->prepare($query);
-    $sth->execute();
+    $sth->execute( @where_args );
     if ($sth->err) {
         warn sprintf('Database returned the following error: %s', $sth->errstr);
         return -1;
@@ -200,7 +217,7 @@ sub get_all_layouts {
 
   my $profiles = get_all_profiles();
 
-  my $profiles = get_all_profiles(field_list => field_list, filter => filter_string);
+  my $profiles = get_all_profiles({ fields => [@fields], filters => { filters => [$value1, $value2] } });
 
 This function returns an arrayref whose elements are hashes containing all profiles upon success and 1 upon failure. Errors are logged
 to the Apache log. Two parameters are accepted. The first limits the field(s) returned. This parameter should be string of comma separted
@@ -211,13 +228,11 @@ NOTE: Do not pass in the keyword 'WHERE.'
 =cut
 
 sub get_all_profiles {
-    my %params = @_;
+    my ( $params ) = @_;
     my @profiles = ();
-    my $query = "SELECT " . ($params{'field_list'} ? $params{'field_list'} : '*') . " FROM printers_profile";
-    $query .= ($params{'filter'} ? " WHERE $params{'filter'};" : ';');
+    my ( $query, @where_args ) = _build_query( $params, 'printers_profile' );
     my $sth = C4::Context->dbh->prepare($query);
-#    $sth->{'TraceLevel'} = 3 if $debug;
-    $sth->execute();
+    $sth->execute( @where_args );
     if ($sth->err) {
         warn sprintf('Database returned the following error: %s', $sth->errstr);
         return -1;
@@ -262,14 +277,13 @@ NOTE: Do not pass in the keyword 'WHERE.'
 =cut
 
 sub get_batch_summary {
-    my %params = @_;
+    my ( $params ) = @_;
     my @batches = ();
-    my $query = "SELECT batch_id,count(batch_id) as _item_count FROM creator_batches WHERE creator=?";
-    $query .= ($params{'filter'} ? " AND $params{'filter'}" : '');
+    $params->{fields} = ['batch_id', 'count(batch_id) as _item_count'];
+    my ( $query, @where_args ) = _build_query( $params, 'creator_batches' );
     $query .= " GROUP BY batch_id";
     my $sth = C4::Context->dbh->prepare($query);
-#    $sth->{'TraceLevel'} = 3;
-    $sth->execute($params{'creator'});
+    $sth->execute( @where_args );
     if ($sth->err) {
         warn sprintf('Database returned the following error on attempted SELECT: %s', $sth->errstr);
         return -1;

diff --git a/labels/label-edit-profile.pl b/labels/label-edit-profile.pl
index 7e87370..13c9aaf 100755 (executable)
--- a/labels/label-edit-profile.pl
+++ b/labels/label-edit-profile.pl
@@ -50,7 +50,7 @@ my $units = get_unit_values();
 
 if ($op eq 'edit') {
     $profile = C4::Labels::Profile->retrieve(profile_id => $profile_id);
-    $template_list = get_all_templates(table_name => 'creator_templates', field_list => 'template_id,template_code, profile_id');
+    $template_list = get_all_templates( fields => [ qw( template_id template_code profile_id) ] );
 }
 elsif ($op eq 'save') {
     my @params = (

diff --git a/labels/label-edit-template.pl b/labels/label-edit-template.pl
index c06a9f1..bcee714 100755 (executable)
--- a/labels/label-edit-template.pl
+++ b/labels/label-edit-template.pl
@@ -49,7 +49,7 @@ my $units = get_unit_values();
 
 if ($op eq 'edit') {
     $label_template = C4::Labels::Template->retrieve(template_id => $template_id);
-    $profile_list = get_all_profiles(field_list => 'profile_id,printer_name,paper_bin',filter => "template_id=$template_id OR template_id=''");
+    $profile_list = get_all_profiles({ fields => [ qw( profile_id printer_name paper_bin ) ], filters => { template_id => [ $template_id, '' ] } } );
     push @$profile_list, {paper_bin => 'N/A', profile_id => 0, printer_name => 'No Profile'};
     foreach my $profile (@$profile_list) {
         if ($profile->{'profile_id'} == $label_template->get_attr('profile_id')) {
@@ -116,7 +116,7 @@ elsif ($op eq 'save') {
 }
 else {  # if we get here, this is a new layout
     $label_template = C4::Labels::Template->new();
-    $profile_list = get_all_profiles(field_list => 'profile_id,printer_name,paper_bin',filter => "template_id=''");
+    $profile_list = get_all_profiles({ fields => [ qw( profile_id printer_name paper_bin ) ], filters => { template_id => [''] } });
     push @$profile_list, {paper_bin => 'N/A', profile_id => 0, printer_name => 'No Profile'};
     foreach my $profile (@$profile_list) {
         if ($profile->{'profile_id'} == 0) {

diff --git a/labels/label-manage.pl b/labels/label-manage.pl
index 3399c8f..1d8a34e 100755 (executable)
--- a/labels/label-manage.pl
+++ b/labels/label-manage.pl
@@ -87,10 +87,10 @@ if ($op eq 'delete') {
     else                                        {}      # FIXME: Some error trapping code
 }
 
-if      ($label_element eq 'layout')    {$db_rows = get_all_layouts(table_name => 'creator_layouts', filter => 'creator=\'Labels\'');}
-elsif   ($label_element eq 'template')  {$db_rows = get_all_templates(table_name => 'creator_templates', filter => 'creator=\'Labels\'');}
-elsif   ($label_element eq 'profile')   {$db_rows = get_all_profiles(table_name => 'printers_profile', filter => 'creator=\'Labels\'');}
-elsif   ($label_element eq 'batch')     {$db_rows = get_batch_summary(filter => "branch_code=\'$branch_code\' OR branch_code=\'NB\'", creator => 'Labels');}
+if      ($label_element eq 'layout')    {$db_rows = get_all_layouts( { filters => { creator => 'Labels' } });}
+elsif   ($label_element eq 'template')  {$db_rows = get_all_templates( { filters => { creator => 'Labels' } });}
+elsif   ($label_element eq 'profile')   {$db_rows = get_all_profiles( { filters => { creator => 'Labels' } });}
+elsif   ($label_element eq 'batch')     {$db_rows = get_batch_summary( { filters => { branch_code => [$branch_code, 'NB'], creator => 'Labels' } });}
 else                                    {}      # FIXME: Some error trapping code
 
 my $table = html_table($display_columns->{$label_element}, $db_rows);

diff --git a/labels/label-print.pl b/labels/label-print.pl
index a8ba29c..ced9e82 100755 (executable)
--- a/labels/label-print.pl
+++ b/labels/label-print.pl
@@ -115,8 +115,8 @@ elsif ($op eq 'none') {
     @batch_ids = map{{batch_id => $_}} @batch_ids;
     @label_ids = map{{label_id => $_}} @label_ids;
     @item_numbers = map{{item_number => $_}} @item_numbers;
-    $templates = get_all_templates(field_list => 'template_id, template_code', filter => 'creator = "Labels"', orderby => 'template_code' );
-    $layouts = get_all_layouts(field_list => 'layout_id, layout_name', filter => 'creator = "Labels"', orderby => 'layout_name' );
+    $templates = get_all_templates( { fields => [ qw( template_id template_code ) ], filters => { creator => "Labels" }, orderby => 'template_code' } );
+    $layouts = get_all_layouts( { fields => [ qw( layout_id layout_name ) ], filters => { creator => "Labels" }, orderby => 'layout_name' } );
     $output_formats = get_output_formats();
     $template->param(
                     batch_ids                   => \@batch_ids,

diff --git a/patroncards/edit-profile.pl b/patroncards/edit-profile.pl
index b7e61bd..4b0769c 100755 (executable)
--- a/patroncards/edit-profile.pl
+++ b/patroncards/edit-profile.pl
@@ -50,7 +50,7 @@ my $units = get_unit_values();
 
 if ($op eq 'edit') {
     $profile = C4::Patroncards::Profile->retrieve(profile_id => $profile_id);
-    $template_list = get_all_templates(table_name => 'creator_templates', field_list => 'template_id,template_code, profile_id');
+    $template_list = get_all_templates({ fields => [ qw( template_id template_code profile_id ) ] });
 }
 elsif ($op eq 'save') {
     my @params = (

diff --git a/patroncards/edit-template.pl b/patroncards/edit-template.pl
index c7b4708..f37757c 100755 (executable)
--- a/patroncards/edit-template.pl
+++ b/patroncards/edit-template.pl
@@ -50,7 +50,7 @@ my $units = get_unit_values();
 
 if ($op eq 'edit') {
     $card_template = C4::Patroncards::Template->retrieve(template_id => $template_id);
-    $profile_list = get_all_profiles(field_list => 'profile_id,printer_name,paper_bin', filter => "template_id=$template_id OR template_id=''");
+    $profile_list = get_all_profiles({ fields => [ qw( profile_id printer_name paper_bin ) ], filters => {template_id => [ $template_id, '' ]} } );
 }
 elsif ($op eq 'save') {
     my @params = (      profile_id      => scalar $cgi->param('profile_id') || '',

diff --git a/patroncards/manage.pl b/patroncards/manage.pl
index 9190cd3..837fccd 100755 (executable)
--- a/patroncards/manage.pl
+++ b/patroncards/manage.pl
@@ -93,10 +93,10 @@ if ($op eq 'delete') {
     exit;
 }
 elsif ($op eq 'none') {
-    if      ($card_element eq 'layout')    {$db_rows = get_all_layouts(table_name => 'creator_layouts', filter => 'creator=\'Patroncards\'');}
-    elsif   ($card_element eq 'template')  {$db_rows = get_all_templates(table_name => 'creator_templates', filter => 'creator=\'Patroncards\'');}
-    elsif   ($card_element eq 'profile')   {$db_rows = get_all_profiles(table_name => 'printers_profile', filter => 'creator=\'Patroncards\'');}
-    elsif   ($card_element eq 'batch')     {$db_rows = get_batch_summary(filter => "branch_code=\'$branch_code\' OR branch_code=\'NB\'", creator => 'Patroncards');}
+    if      ($card_element eq 'layout')    {$db_rows = get_all_layouts( { filters => { creator => 'Patroncards' } });}
+    elsif   ($card_element eq 'template')  {$db_rows = get_all_templates( { filters => { creator => 'Patroncards' } });}
+    elsif   ($card_element eq 'profile')   {$db_rows = get_all_profiles( { filters => { creator => 'Patroncards' } });}
+    elsif   ($card_element eq 'batch')     {$db_rows = get_batch_summary( { filters => { branch_code => [ $branch_code, 'NB' ], creator => 'Patroncards' } });}
     else                                   {warn sprintf("Unknown card element passed in: %s.",$card_element); $errstr = 202;}
 }
 else { # trap unsupported operations here

diff --git a/patroncards/print.pl b/patroncards/print.pl
index 46f5d32..fdadfc9 100755 (executable)
--- a/patroncards/print.pl
+++ b/patroncards/print.pl
@@ -122,8 +122,8 @@ elsif ($op eq 'none') {
     @batch_ids = grep{$_ = {batch_id => $_}} @batch_ids;
     @label_ids = grep{$_ = {label_id => $_}} @label_ids;
     @borrower_numbers = grep{$_ = {borrower_number => $_}} @borrower_numbers;
-    $templates = get_all_templates(field_list => 'template_id, template_code', filter => 'creator = "Patroncards"');
-    $layouts = get_all_layouts(field_list => 'layout_id, layout_name', filter => 'creator = "Patroncards"');
+    $templates = get_all_templates( { fields => [qw( template_id template_code ) ], filters => { creator => "Patroncards" } });
+    $layouts = get_all_layouts({ fields => [ qw( layout_id layout_name ) ], filters => { creator => "Patroncards" } });
     $output_formats = get_output_formats();
     $template->param(
                     batch_ids                   => \@batch_ids,

diff --git a/t/db_dependent/Creators/Lib.t b/t/db_dependent/Creators/Lib.t
index 6408c23..cd69265 100644 (file)
--- a/t/db_dependent/Creators/Lib.t
+++ b/t/db_dependent/Creators/Lib.t
@@ -314,7 +314,7 @@ is( $templates->[1]->{units},            'POINT',      'units            is good
 is( $templates->[1]->{creator},          'Labels',     'creator          is good' );
 
 # With field_list params --------------
-$templates = get_all_templates( field_list => 'units, cols, rows' );
+$templates = get_all_templates( {fields=> [qw(units cols rows)] } );
 
 $query = '
   SELECT count(*)
@@ -502,7 +502,7 @@ is( $layouts->[1]->{layout_xml},    'layout_xml2', 'layout_xml     is good' );
 is( $layouts->[1]->{creator},       'Labels',      'creator        is good' );
 
 # With field_list params --------------
-$layouts = get_all_layouts( field_list => 'barcode_type, layout_name, font' );
+$layouts = get_all_layouts( { fields => [qw(barcode_type layout_name font)] });
 
 $query = '
   SELECT count(*)
@@ -662,7 +662,7 @@ is( $profiles->[1]->{units},        'POINT',        'units          is good' );
 is( $profiles->[1]->{creator},      'Labels',       'creator        is good' );
 
 # With field_list params --------------
-$profiles = get_all_profiles( field_list => 'printer_name, template_id' );
+$profiles = get_all_profiles( { fields => [qw(printer_name template_id)] });
 
 $query = '
   SELECT count(*)
@@ -696,7 +696,7 @@ isnt( exists $profiles->[1]->{units},         'POINT',        'units          is
 isnt( exists $profiles->[1]->{creator},       'Labels',       'creator        is good' );
 
 # With filter params ------------------
-$profiles = get_all_profiles( filter => 'template_id = 1235' );
+$profiles = get_all_profiles( filters => { template_id => 1235 } );
 
 $query = '
   SELECT count(*)