From 31051e547fbc824645f631de98365a498d182502 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 13 Jan 2017 17:43:25 +0100 Subject: [PATCH] Bug 17900: Fix possible SQL injection in patron cards template editing Content-Type: text/plain; charset=utf-8 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 Signed-off-by: Marcel de Rooy --- C4/Creators/Lib.pm | 56 +++++++++++++++++++++++++++---------------- labels/label-edit-profile.pl | 2 +- labels/label-edit-template.pl | 4 ++-- labels/label-manage.pl | 8 +++---- labels/label-print.pl | 4 ++-- patroncards/edit-profile.pl | 2 +- patroncards/edit-template.pl | 2 +- patroncards/manage.pl | 8 +++---- patroncards/print.pl | 4 ++-- t/db_dependent/Creators/Lib.t | 8 +++---- 10 files changed, 56 insertions(+), 42 deletions(-) diff --git a/C4/Creators/Lib.pm b/C4/Creators/Lib.pm index bdf712e..c15b3e7 100644 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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(*) -- 2.1.4