Bugzilla – Attachment 153997 Details for
Bug 33339
Formula injection (CSV Injection) in export functionality
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 33339: Prevent Formula Injection (CSV Injection) in CSV files
Bug-33339-Prevent-Formula-Injection-CSV-Injection-.patch (text/plain), 14.71 KB, created by
Kyle M Hall (khall)
on 2023-07-27 16:37:40 UTC
(
hide
)
Description:
Bug 33339: Prevent Formula Injection (CSV Injection) in CSV files
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2023-07-27 16:37:40 UTC
Size:
14.71 KB
patch
obsolete
>From 991e7b1671ac8f0c171ba1af3d1d573c03821d79 Mon Sep 17 00:00:00 2001 >From: Kyle M Hall <kyle@bywatersolutions.com> >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
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 33339
:
148717
|
153997
|
170719
|
170720
|
171810
|
171811
|
171819
|
171820
|
171961
|
172088
|
172089