Bugzilla – Attachment 59064 Details for
Bug 17900
Possible SQL injection in patroncards template editing
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 17900: Fix possible SQL injection in patron cards template editing
Bug-17900-Fix-possible-SQL-injection-in-patron-car.patch (text/plain), 16.41 KB, created by
Chris Cormack
on 2017-01-17 07:35:47 UTC
(
hide
)
Description:
Bug 17900: Fix possible SQL injection in patron cards template editing
Filename:
MIME Type:
Creator:
Chris Cormack
Created:
2017-01-17 07:35:47 UTC
Size:
16.41 KB
patch
obsolete
>From 1332196235f6552885669aa911342a9f6b72efe0 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Fri, 13 Jan 2017 17:43:25 +0100 >Subject: [PATCH] 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> >--- > 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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 17900
:
58965
|
59064
|
59603
|
59612
|
59613