Attachment 170719 Details for Bug 33339 – Bug 33339: Prevent Formula Injection (CSV Injection) in CSV files

[patch] Bug 33339: Prevent Formula Injection (CSV Injection) in CSV files

Bug-33339-Prevent-Formula-Injection-CSV-Injection-.patch (text/plain), 14.92 KB, created by Magnus Enger on 2024-08-26 13:34:43 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Magnus Enger

Created: 2024-08-26 13:34:43 UTC

Size: 14.92 KB

patch

obsolete

>From 438640b7a3108c44944177d40bc4dfcaafd224bd 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
>
>Signed-off-by: Magnus Enger <magnus@libriotech.no>
>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

Actions: View | Diff | Splinter Review

Attachments on bug 33339: 148717 | 153997 | 170719 | 170720 | 171810 | 171811 | 171819 | 171820 | 171961 | 172088 | 172089