From 991e7b1671ac8f0c171ba1af3d1d573c03821d79 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 --- 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 | 8 +++++++- 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, 53 insertions(+), 33 deletions(-) diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm index da62c17afeb..988003350d6 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 3d44d46ec39..d02ec43d574 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 eadf9cce0ab..d298dd5f1e1 100644 --- a/C4/Labels/Label.pm +++ b/C4/Labels/Label.pm @@ -113,7 +113,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 69a60f3520f..e83176d2629 100644 --- a/C4/Record.pm +++ b/C4/Record.pm @@ -400,7 +400,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 3ba184d1e28..a7e16e46a67 100644 --- a/Koha/Patrons/Import.pm +++ b/Koha/Patrons/Import.pm @@ -614,7 +614,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 cbded1ca64b..c83be12dfd3 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 e2bb848d424..e3bbfa89801 100755 --- a/circ/overdue.pl +++ b/circ/overdue.pl @@ -401,7 +401,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 8c06a41a0b5..6e7fbea37f3 100755 --- a/labels/label-create-csv.pl +++ b/labels/label-create-csv.pl @@ -66,7 +66,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 19e6aef27b9..b65890827d1 100755 --- a/misc/cronjobs/overdue_notices.pl +++ b/misc/cronjobs/overdue_notices.pl @@ -412,7 +412,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 debcf5a0245..48cd195503f 100755 --- a/misc/cronjobs/runreport.pl +++ b/misc/cronjobs/runreport.pl @@ -306,12 +306,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 7cde608ecef..b6a19d4dd84 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 0d506ccc0f4..c30ec0e4681 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 e88a5d7ab56..0d8011657a3 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 46792bf88c3..d017e382943 100755 --- a/reports/guided_reports.pl +++ b/reports/guided_reports.pl @@ -924,7 +924,13 @@ 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 f9d633251df..fe0c2d4e3a9 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 af83f2c6f64..e2425b82da6 100755 --- a/serials/lateissues-export.pl +++ b/serials/lateissues-export.pl @@ -38,12 +38,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 ab4d5e75a77..349a34f83ec 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 a02f14dc40d..d5501e3d262 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 6038931f62b..b1d7250c040 100755 --- a/tools/inventory.pl +++ b/tools/inventory.pl @@ -374,9 +374,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 9db4e430ed5..a061961a62a 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 -- 2.39.1