From c9ffc538de3cb4972d126407d1d251b2090ed8c8 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 "empty" which replaces formulas by empty string. 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. Signed-off-by: Chris Cormack Signed-off-by: Marcel de Rooy [EDIT] Change none to empty in the commit message ! None is the default, doing nothing. Empty clears the formulas. Signed-off-by: Lucas Gass (cherry picked from commit e588f810a521065bf46dc037edeac5c83d537714) Signed-off-by: Fridolin Somers --- 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 370030895e..f23f7f4a8e 100644 --- a/C4/Acquisition.pm +++ b/C4/Acquisition.pm @@ -279,7 +279,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 799499ec86..440a8950ce 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 026edbab51..9e3a0c20c9 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 306d998f08..98a7f6d4b3 100755 --- a/circ/overdue.pl +++ b/circ/overdue.pl @@ -421,7 +421,7 @@ sub build_csv { my @keys = qw ( duedate title author borrowertitle firstname surname phone barcode email address address2 zipcode city country branchcode 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 1dde63b114..35525a7d2e 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 fad5670944..fbf2c58343 100755 --- a/reports/guided_reports.pl +++ b/reports/guided_reports.pl @@ -948,7 +948,7 @@ elsif ($phase 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 .= Encode::decode('UTF-8', $csv->string()) . "\n"; diff --git a/reports/itemslost.pl b/reports/itemslost.pl index f9d633251d..fe0c2d4e3a 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 '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 fa9812918c..a6d142923c 100755 --- a/tools/import_borrowers.pl +++ b/tools/import_borrowers.pl @@ -77,7 +77,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 a76beb6be4..8e2baf8637 100755 --- a/tools/inventory.pl +++ b/tools/inventory.pl @@ -370,9 +370,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 9db4e430ed..a061961a62 100755 --- a/tools/viewlog.pl +++ b/tools/viewlog.pl @@ -232,7 +232,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