Koha-community/Koha
15
7
Fork 0
Code Issues Releases Wiki Activity

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

Browse source 
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>
This commit is contained in:
Jonathan Druart 2017-01-13 17:43:25 +01:00 committed by Kyle M Hall
parent 4ff78a9a0d
commit a70980d825
10 changed files with 56 additions and 42 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

56
C4/Creators/Lib.pm
View file

@ -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;

2
labels/label-edit-profile.pl
View file

@ -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 = (

4
labels/label-edit-template.pl
View file

@ -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) {

8
labels/label-manage.pl
View file

@ -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);

4
labels/label-print.pl
View file

@ -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,

2
patroncards/edit-profile.pl
View file

@ -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 = (

2
patroncards/edit-template.pl
View file

@ -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') || '',

8
patroncards/manage.pl
View file

@ -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

4
patroncards/print.pl
View file

@ -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,

8
t/db_dependent/Creators/Lib.t
View file

@ -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(*)