From 438640b7a3108c44944177d40bc4dfcaafd224bd Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Thu, 27 Jul 2023 12:30:54 -0400 Subject: [PATCH] Bug 33339: Prevent Formula Injection (CSV Injection) in CSV files The system is vulnerable to Formula Injection attacks as the data stored within the database and exported as CSV/Excel is not being sanitized or validated against implanted formula payloads This patch modifies all uses of Text::CSV and derived classes to pass the "formula" parameter with value of "none" which prevents those formula from being exported. Test Plan: 1) Apply this patch 2) For guided_reports.pl, attempt to export CSV where you've set a column to a formula somehow ( such as "=1+3" ) 3) Export that CSV file 4) Note the formula has not been exported 5) Repeat this plan for the remaining scripts that export CSV files where users can define the outputted data Signed-off-by: Magnus Enger Fixed two conflicts. I have tested that this works as advertised on: - Reports (Download > Comma separated text (.csv)) [Text::CSV::Encoded] - Circulation > Overdues > Download file of all overdues [Text::CSV_XS] - misc/export_borrowers.pl [Text::CSV] This covers all modules used, and both GUI and command line. --- C4/Acquisition.pm | 10 +++++++++- C4/ImportExportFramework.pm | 2 +- C4/Labels/Label.pm | 2 +- C4/Record.pm | 2 +- Koha/Patrons/Import.pm | 2 +- admin/aqplan.pl | 4 +++- circ/overdue.pl | 2 +- labels/label-create-csv.pl | 2 +- misc/cronjobs/overdue_notices.pl | 2 +- misc/cronjobs/runreport.pl | 15 +++++++++------ misc/export_borrowers.pl | 2 +- misc/migration_tools/import_lexile.pl | 2 +- reports/cash_register_stats.pl | 1 - reports/guided_reports.pl | 2 +- reports/itemslost.pl | 3 +-- serials/lateissues-export.pl | 15 +++++++++------ t/db_dependent/Record/marcrecord2csv.t | 2 +- tools/import_borrowers.pl | 2 +- tools/inventory.pl | 6 +++--- tools/viewlog.pl | 2 +- 20 files changed, 47 insertions(+), 33 deletions(-) diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm index 6824bd8637..296e9d3219 100644 --- a/C4/Acquisition.pm +++ b/C4/Acquisition.pm @@ -277,7 +277,15 @@ sub GetBasketAsCSV { my $delimiter = $csv_profile->csv_separator; $delimiter = "\t" if $delimiter eq "\\t"; - my $csv = Text::CSV_XS->new({'quote_char'=>'"','escape_char'=>'"','sep_char'=>$delimiter,'binary'=>1}); + my $csv = Text::CSV_XS->new( + { + quote_char => '"', + escape_char => '"', + sep_char => $delimiter, + binary => 1, + formula => "empty", + } + ); my $csv_profile_content = $csv_profile->content; my ( @headers, @fields ); while ( $csv_profile_content =~ / diff --git a/C4/ImportExportFramework.pm b/C4/ImportExportFramework.pm index 3d44d46ec3..d02ec43d57 100644 --- a/C4/ImportExportFramework.pm +++ b/C4/ImportExportFramework.pm @@ -965,7 +965,7 @@ sub _import_table_csv shift @fieldsPK; my $ok = 0; my $pos = 0; - my $csv = Text::CSV_XS->new ({ binary => 1 }); + my $csv = Text::CSV_XS->new( { binary => 1, formula => "empty" } ); while ( my $row = $csv->getline($dom) ) { my @fields = @$row; @arrData = @fields; diff --git a/C4/Labels/Label.pm b/C4/Labels/Label.pm index 1d6ea20e8b..702f566169 100644 --- a/C4/Labels/Label.pm +++ b/C4/Labels/Label.pm @@ -115,7 +115,7 @@ sub _get_label_item { sub _get_text_fields { my $format_string = shift; - my $csv = Text::CSV_XS->new({allow_whitespace => 1}); + my $csv = Text::CSV_XS->new( { allow_whitespace => 1, formula => "empty" } ); my $status = $csv->parse($format_string); my @sorted_fields = map {{ 'code' => $_, desc => $_ }} map { $_ && $_ eq 'callnumber' ? 'itemcallnumber' : $_ } # see bug 5653 diff --git a/C4/Record.pm b/C4/Record.pm index 29f105a9f1..e4aac130dd 100644 --- a/C4/Record.pm +++ b/C4/Record.pm @@ -401,7 +401,7 @@ sub marc2csv { my ($biblios, $id, $itemnumbers) = @_; $itemnumbers ||= []; my $output; - my $csv = Text::CSV::Encoded->new(); + my $csv = Text::CSV::Encoded->new( { formula => "empty" } ); # Getting yaml file my $configfile = "../tools/csv-profiles/$id.yaml"; diff --git a/Koha/Patrons/Import.pm b/Koha/Patrons/Import.pm index 5c8dc6b876..f4b5c2538e 100644 --- a/Koha/Patrons/Import.pm +++ b/Koha/Patrons/Import.pm @@ -627,7 +627,7 @@ sub generate_patron_attributes { push (@$feedback, { feedback => 1, name => 'attribute string', value => $string }); return [] unless $string; # Unit tests want the feedback, is it really needed? - my $csv = Text::CSV->new({binary => 1}); # binary needed for non-ASCII Unicode + my $csv = Text::CSV->new( { binary => 1, formula => "empty" } ); # binary needed for non-ASCII Unicode my $ok = $csv->parse($string); # parse field again to get subfields! my @list = $csv->fields(); my @patron_attributes = diff --git a/admin/aqplan.pl b/admin/aqplan.pl index a6d863e1c2..3b5c5d46d5 100755 --- a/admin/aqplan.pl +++ b/admin/aqplan.pl @@ -429,8 +429,10 @@ sub _print_to_csv { binmode STDOUT, ':encoding(UTF-8)'; my $csv = Text::CSV_XS->new( - { sep_char => $del, + { + sep_char => $del, always_quote => 'TRUE', + formula => "empty", } ); print $input->header( diff --git a/circ/overdue.pl b/circ/overdue.pl index dacc8a9301..7af624600b 100755 --- a/circ/overdue.pl +++ b/circ/overdue.pl @@ -434,7 +434,7 @@ sub build_csv { my @keys = qw ( duedate title author borrowertitle firstname surname phone barcode email address address2 zipcode city country branchcode datelastissued itemcallnumber biblionumber borrowernumber itemnum issuedate replacementprice itemnotes_nonpublic streetnumber streettype); - my $csv = Text::CSV_XS->new(); + my $csv = Text::CSV_XS->new( { formula => "empty" } ); $csv->combine(@keys); push @lines, $csv->string(); diff --git a/labels/label-create-csv.pl b/labels/label-create-csv.pl index a6c6f943ee..fe4600a80e 100755 --- a/labels/label-create-csv.pl +++ b/labels/label-create-csv.pl @@ -73,7 +73,7 @@ else { $items = $batch->get_attr('items'); } -my $csv = Text::CSV_XS->new(); +my $csv = Text::CSV_XS->new( { formula => "empty" } ); foreach my $item (@$items) { my $label = C4::Labels::Label->new( diff --git a/misc/cronjobs/overdue_notices.pl b/misc/cronjobs/overdue_notices.pl index 6f78b2838a..5e24361699 100755 --- a/misc/cronjobs/overdue_notices.pl +++ b/misc/cronjobs/overdue_notices.pl @@ -415,7 +415,7 @@ our $csv; # the Text::CSV_XS object our $csv_fh; # the filehandle to the CSV file. if ( defined $csvfilename ) { my $sep_char = C4::Context->csv_delimiter; - $csv = Text::CSV_XS->new( { binary => 1 , sep_char => $sep_char } ); + $csv = Text::CSV_XS->new( { binary => 1, sep_char => $sep_char, formula => "empty" } ); if ( $csvfilename eq '' ) { $csv_fh = *STDOUT; } else { diff --git a/misc/cronjobs/runreport.pl b/misc/cronjobs/runreport.pl index ee807d53ca..f1a51d6975 100755 --- a/misc/cronjobs/runreport.pl +++ b/misc/cronjobs/runreport.pl @@ -329,12 +329,15 @@ foreach my $report_id (@ARGV) { } $message = $cgi->table(join "", @rows); } elsif ($format eq 'csv') { - my $csv = Text::CSV::Encoded->new({ - encoding_out => 'utf8', - binary => 1, - quote_char => $quote, - sep_char => $separator, - }); + my $csv = Text::CSV::Encoded->new( + { + encoding_out => 'utf8', + binary => 1, + quote_char => $quote, + sep_char => $separator, + formula => 'empty', + } + ); if ( $csv_header ) { my @fields = map { decode( 'utf8', $_ ) } @{ $sth->{NAME} }; diff --git a/misc/export_borrowers.pl b/misc/export_borrowers.pl index 7cde608ece..b6a19d4dd8 100755 --- a/misc/export_borrowers.pl +++ b/misc/export_borrowers.pl @@ -94,7 +94,7 @@ unless ( $separator ) { $separator = C4::Context->csv_delimiter; } -my $csv = Text::CSV->new( { sep_char => $separator, binary => 1 } ); +my $csv = Text::CSV->new( { sep_char => $separator, binary => 1, formula => 'empty' } ); # If the user did not specify any field to export, we assume they want them all # We retrieve the first borrower informations to get field names diff --git a/misc/migration_tools/import_lexile.pl b/misc/migration_tools/import_lexile.pl index 0d506ccc0f..c30ec0e468 100755 --- a/misc/migration_tools/import_lexile.pl +++ b/misc/migration_tools/import_lexile.pl @@ -96,7 +96,7 @@ if ( $help || !$file || !$confirm ) { my $schema = Koha::Database->new()->schema(); -my $csv = Text::CSV->new( { binary => 1, sep_char => "\t" } ) +my $csv = Text::CSV->new( { binary => 1, sep_char => "\t", formula => 'empty' } ) or die "Cannot use CSV: " . Text::CSV->error_diag(); open my $fh, "<:encoding(utf8)", $file or die "test.csv: $!"; diff --git a/reports/cash_register_stats.pl b/reports/cash_register_stats.pl index f0e3161ac3..2a7de38101 100755 --- a/reports/cash_register_stats.pl +++ b/reports/cash_register_stats.pl @@ -23,7 +23,6 @@ use C4::Reports qw( GetDelimiterChoices ); use C4::Output qw( output_html_with_http_headers ); use DateTime; use Koha::DateUtils qw( dt_from_string ); -use Text::CSV::Encoded; use List::Util qw( any ); use Koha::Account::CreditTypes; diff --git a/reports/guided_reports.pl b/reports/guided_reports.pl index b7c722945a..d13063c96c 100755 --- a/reports/guided_reports.pl +++ b/reports/guided_reports.pl @@ -639,7 +639,7 @@ elsif ($op eq 'export'){ if ( $format eq 'csv' ) { my $delimiter = C4::Context->csv_delimiter; $type = 'application/csv'; - my $csv = Text::CSV::Encoded->new({ encoding_out => 'UTF-8', sep_char => $delimiter}); + my $csv = Text::CSV::Encoded->new({ encoding_out => 'UTF-8', sep_char => $delimiter, formula => 'empty' }); $csv or die "Text::CSV::Encoded->new({binary => 1}) FAILED: " . Text::CSV::Encoded->error_diag(); if ( $csv->combine( header_cell_values($sth) ) ) { $content .= $scrubber->scrub( Encode::decode( 'UTF-8', $csv->string() ) ) . "\n"; diff --git a/reports/itemslost.pl b/reports/itemslost.pl index 9f1a990f47..80c84bb633 100755 --- a/reports/itemslost.pl +++ b/reports/itemslost.pl @@ -28,7 +28,6 @@ This script displays lost items. use Modern::Perl; use CGI qw ( -utf8 ); -use Text::CSV_XS; use C4::Auth qw( get_template_and_user ); use C4::Output qw( output_html_with_http_headers ); use Text::CSV::Encoded; @@ -91,7 +90,7 @@ if ( $op eq 'cud-export' ) { my $delimiter = $csv_profile->csv_separator; $delimiter = "\t" if $delimiter eq "\\t"; - my $csv = Text::CSV::Encoded->new({ encoding_out => 'UTF-8', sep_char => $delimiter}); + my $csv = Text::CSV::Encoded->new( { encoding_out => 'UTF-8', sep_char => $delimiter, formula => 'empty' } ); $csv or die "Text::CSV::Encoded->new({binary => 1}) FAILED: " . Text::CSV::Encoded->error_diag(); $csv->combine(@headers); my $content .= Encode::decode('UTF-8', $csv->string()) . "\n"; diff --git a/serials/lateissues-export.pl b/serials/lateissues-export.pl index dff0e5a7ac..12130d4434 100755 --- a/serials/lateissues-export.pl +++ b/serials/lateissues-export.pl @@ -46,12 +46,15 @@ die "There is no valid csv profile given" unless $csv_profile; my $delimiter = $csv_profile->csv_separator; $delimiter = "\t" if $delimiter eq "\\t"; -my $csv = Text::CSV_XS->new({ - 'quote_char' => '"', - 'escape_char' => '"', - 'sep_char' => $delimiter, - 'binary' => 1 -}); +my $csv = Text::CSV_XS->new( + { + quote_char => '"', + escape_char => '"', + sep_char => $delimiter, + binary => 1, + formula => 'empty', + } +); my $content = $csv_profile->content; my ( @headers, @fields ); diff --git a/t/db_dependent/Record/marcrecord2csv.t b/t/db_dependent/Record/marcrecord2csv.t index 53d80918ab..bf3df2101f 100755 --- a/t/db_dependent/Record/marcrecord2csv.t +++ b/t/db_dependent/Record/marcrecord2csv.t @@ -30,7 +30,7 @@ my ( $biblionumber, $biblioitemnumber ) = AddBiblio( $record, $frameworkcode ); my $csv_content = q(Title=245$a|Author=245$c|Subject=650$a); my $csv_profile_id_1 = insert_csv_profile({ csv_content => $csv_content }); -my $csv = Text::CSV::Encoded->new(); +my $csv = Text::CSV::Encoded->new( { formula => 'empty' } ); # Test bad biblionumber case my $csv_output = C4::Record::marcrecord2csv( -1, $csv_profile_id_1, 1, $csv ); diff --git a/tools/import_borrowers.pl b/tools/import_borrowers.pl index 048d053d47..25686c205e 100755 --- a/tools/import_borrowers.pl +++ b/tools/import_borrowers.pl @@ -78,7 +78,7 @@ $template->param( categories => \@patron_categories ); $template->param( borrower_fields => Koha::Database::Columns->columns->{borrowers} ); if ( $input->param('sample') ) { - our $csv = Text::CSV->new( { binary => 1 } ); # binary needed for non-ASCII Unicode + our $csv = Text::CSV->new( { binary => 1, formula => 'empty' } ); # binary needed for non-ASCII Unicode print $input->header( -type => 'application/vnd.sun.xml.calc', # 'application/vnd.ms-excel' ? -attachment => 'patron_import.csv', diff --git a/tools/inventory.pl b/tools/inventory.pl index e38d7c4b65..1c94183596 100755 --- a/tools/inventory.pl +++ b/tools/inventory.pl @@ -372,9 +372,9 @@ $template->param( # Export to csv if (defined $input->param('CSVexport') && $input->param('CSVexport') eq 'on'){ - eval {use Text::CSV ();}; - my $csv = Text::CSV->new or - die Text::CSV->error_diag (); + eval { use Text::CSV (); }; + my $csv = Text::CSV->new( { formula => 'empty' } ) + or die Text::CSV->error_diag(); binmode STDOUT, ":encoding(UTF-8)"; print $input->header( -type => 'text/csv', diff --git a/tools/viewlog.pl b/tools/viewlog.pl index 91cd65df25..b1c27b0c52 100755 --- a/tools/viewlog.pl +++ b/tools/viewlog.pl @@ -234,7 +234,7 @@ if ($do_it) { my $content = q{}; if (@data) { my $delimiter = C4::Context->csv_delimiter; - my $csv = Text::CSV::Encoded->new( { encoding_out => 'utf8', sep_char => $delimiter } ); + my $csv = Text::CSV::Encoded->new( { encoding_out => 'utf8', sep_char => $delimiter, formula => 'empty' } ); $csv or die "Text::CSV::Encoded->new FAILED: " . Text::CSV::Encoded->error_diag(); # First line with heading -- 2.34.1