(In reply to Magdy Moustafa from comment #0)
> Recommendations
> - Ensure Data Validation and Sanitization:  Ensure input data is validated
> and sanitized against malicious user inputs including Excel formulas during
> the data insertion and data export operations.

Are you aware of a recommended way of doing that?

(In reply to Jonathan Druart from comment #1)
> (In reply to Magdy Moustafa from comment #0)
> > Recommendations
> > - Ensure Data Validation and Sanitization:  Ensure input data is validated
> > and sanitized against malicious user inputs including Excel formulas during
> > the data insertion and data export operations.
> 
> Are you aware of a recommended way of doing that?

OWASP has some info on the topic: https://owasp.org/www-community/attacks/CSV_Injection

Worth noting that Koha multiple ways of creating CSV files. 

n the "PoC screenshot", it's using a client-side CSV download, which I think is provided by the "Buttons" extension to the DataTables Javascript library, so it would not be a trivial fix.

I haven't checked the server-side CSV generation.

Escaping exports is probably a lot more doable than validating inputs since there a thousands of different inputs. That said, we could potentially put some extra checking on OPAC inputs, especially borrower self-registrations and purchase suggestions.

(On a side note, locally we've actually removed the data export options shown in the "PoC screenshot" due to the ease of downloading borrower personal information. At some point, I might post a patch to remove those buttons...)

any update on the state of this issue?

https://affinity-it-security.com/how-to-prevent-csv-injection/ has a regular expression we could run on the raw CSV output before sending/writing it.

Text::CSV ( and CSV_XS ) can prevent formulas from being entered.
my $csv = Text::CSV_XS->new({ formula => "none" });

(In reply to Kyle M Hall from comment #5)
> Text::CSV ( and CSV_XS ) can prevent formulas from being entered.
> my $csv = Text::CSV_XS->new({ formula => "none" });

This sounds good, although it won't stop the issue from the PoC. 

I see someone mentioning it in 2019 but looks like the DataTables creator didn't understand the problem: https://datatables.net/forums/discussion/57856/excel-command-injection

Looks like it's open source so we could potentially look at fixing it at the source...

https://github.com/DataTables/Buttons

How do we want to deal with this one?

We have a patch to fix server-side CSV processing (at least where we're using the CSV library), but we don't have a solution for client-side CSV processing...

(In reply to David Cook from comment #8)
> How do we want to deal with this one?
> 
> We have a patch to fix server-side CSV processing (at least where we're
> using the CSV library), but we don't have a solution for client-side CSV
> processing...

Wouldn't fixing part of the issue be better than not fixing anything at all?

A description of the problem has already been made public: https://www.cve.org/CVERecord?id=CVE-2024-24337

(In reply to Magnus Enger from comment #9)
> (In reply to David Cook from comment #8)
> > How do we want to deal with this one?
> > 
> > We have a patch to fix server-side CSV processing (at least where we're
> > using the CSV library), but we don't have a solution for client-side CSV
> > processing...
> 
> Wouldn't fixing part of the issue be better than not fixing anything at all?

Yeah, I think so. Let's fix the server-side first. 

At some point, can look at at least raising an issue in DataTables if it hasn't been fixed yet. We might've updated that library since this was raised. Will need to check.

The existing patch updates all uses of Text::CSV*->new, and adds "formula => 'empty'" to it. (We use a mix of Text::CSV, Text::CSV_XS and Text::CSV::Encode). 

Will we remember to use "formula => 'empty'" next time we use Text::CSV* in a new place? Or would it make sense to wrap it in something like Koha::Exporter::CSV, so we don't have to remember? Could we standardize on one module, or are there specific features we need from the different modules?

(In reply to Magnus Enger from comment #12)
> The existing patch updates all uses of Text::CSV*->new, and adds "formula =>
> 'empty'" to it. (We use a mix of Text::CSV, Text::CSV_XS and
> Text::CSV::Encode). 
> 
> Will we remember to use "formula => 'empty'" next time we use Text::CSV* in
> a new place? Or would it make sense to wrap it in something like
> Koha::Exporter::CSV, so we don't have to remember? Could we standardize on
> one module, or are there specific features we need from the different
> modules?

Ohh touché. 

Yeah, it probably wouldn't be a bad idea to standardize with Text::CSV_XS and have Koha::CSV be a subclass of Text::CSV_XS. (I say Koha::CSV as it wouldn't always be an exporter. There would be times where it parses incoming CSV data as well.)

(In reply to David Cook from comment #13)
> Ohh touché. 
> 
> Yeah, it probably wouldn't be a bad idea to standardize with Text::CSV_XS
> and have Koha::CSV be a subclass of Text::CSV_XS. (I say Koha::CSV as it
> wouldn't always be an exporter. There would be times where it parses
> incoming CSV data as well.)

Next question: Will it be better to do a quick fix along the lines of the current patch, and then refactor things to use a Koha:: module later, or is anyone ready to jump on that and do it right away? 

There are also at least two places where we generate CSV without a module: 

- reports/orders_by_fund.pl
- C4::ImportExportFramework::_export_table_csv()

Not sure if those are somehow too complext to use a module, or if they could be refactored.

Also, I found three places where "formula => 'empty'" is added, but it is not necessary, because Text::CSV* is only used for reading CSV, not writing: 

- C4::ImportExportFramework::_import_table_csv
- Koha::Patrons::Import::generate_patron_attributes()
- misc/migration_tools/import_lexile.pl

Should we take those out again, or do we just leave them, because they do no harm?

All good questions!

My hands are a bit full at the moment... 

I'm hoping to do a bit of a review of all the open security bugs soon, and then see what I'm best equipped to tackle.

I have added two separate bugs for fixing problems that are not related to standard Perl modules: 

- Bug 37726 - Fix CSV formula injection - hand crafted CSV
- Bug 37727 - Fix CSV formula injection - client side (DataTables)

QA: Looking here

Created attachment 171819 [details] [review]
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 <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.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Change none to empty in the commit message ! None is the default,
doing nothing. Empty clears the formulas.

(In reply to Kyle M Hall from comment #5)
> https://affinity-it-security.com/how-to-prevent-csv-injection/ has a regular
> expression we could run on the raw CSV output before sending/writing it.
> 
> Text::CSV ( and CSV_XS ) can prevent formulas from being entered.
> my $csv = Text::CSV_XS->new({ formula => "none" });

Should be empty as you did in the patches.
Just noting. Corrected commit message.

(In reply to Magnus Enger from comment #19)
> Created attachment 170720 [details] [review] [review]
> Bug 33339: (Follow up) Fix more cases of formula=empty
> 
> This patch adds formula=empty to:
> - Koha::ERM::EUsage::SushiCounter::_build_COUNTER_report_file()
> 
> It also removes formula=empty from:
> - C4::ImportExportFramework::_import_table_csv()
> - Koha::Patrons::Import::generate_patron_attributes()
> because in these places CSV is only read, not generated. Please
> FQA if this is unwanted.

Please do not include the occurrences where you remove empty here. Thats kind of odd in this report and out of scope. It probably does not make a difference, but if you really feel that they should be removed there, please move them to a new report.

Ignoring btw:

 WARN   reports/cash_register_stats.pl
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 76, now: 77)
 WARN   tools/import_borrowers.pl
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 27, now: 28)

Created attachment 171961 [details] [review]
Bug 33339: (Follow up) Fix more cases of formula=empty

This patch adds formula=empty to:
- Koha::ERM::EUsage::SushiCounter::_build_COUNTER_report_file()

Updated 2024-09-26: Earlier version also removed unnecessary
use of formula=empty from two places.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

> Please do not include the occurrences where you remove empty here.

Should be fixed now. Not sure if status should be NSO or SO. Setting to SO.

Created attachment 172088 [details] [review]
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 <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.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Change none to empty in the commit message ! None is the default,
doing nothing. Empty clears the formulas.

Created attachment 172089 [details] [review]
Bug 33339: (Follow up) Fix more cases of formula=empty

This patch adds formula=empty to:
- Koha::ERM::EUsage::SushiCounter::_build_COUNTER_report_file()

Updated 2024-09-26: Earlier version also removed unnecessary
use of formula=empty from two places.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Ignoring:
 WARN   reports/cash_register_stats.pl
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 76, now: 77)
 WARN   tools/import_borrowers.pl
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 27, now: 28)

No big deal

Hi, is there any chance I could please get a more detailed test plan? :)

(In reply to Wainui Witika-Park from comment #33)
> Hi, is there any chance I could please get a more detailed test plan? :)

Don't worry, I managed to figure it out :)

APPLYING PATCHES

PATCH 1 - I have applied "172088 - Bug 33339: Prevent Formula Injection (CSV Injection) in CSV files" to 23.05.x-security, there were a couple of conflicts:

1. circ/overdue.pl

<<<<<<< HEAD
      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();
=======
        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( { formula => "empty" } );
>>>>>>> Bug 33339: Prevent Formula Injection (CSV Injection) in CSV files

- KEPT THE FOLLOWING (tail without "datelastissued"):
=======
        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( { formula => "empty" } );
>>>>>>> Bug 33339: Prevent Formula Injection (CSV Injection) in CSV files

2. reports/guided_reports.pl

<<<<<<< HEAD
elsif ($phase eq 'Run this report'){
=======

elsif ($op eq 'export'){

	# export results to tab separated text or CSV
    my $report_id   = $input->param('id');
    my $report      = Koha::Reports->find($report_id);
    my $sql         = $report->savedsql;
    my @param_names = $input->multi_param('param_name');
    my @sql_params  = $input->multi_param('sql_params');
    my $format      = $input->param('format');
    my $reportname  = $input->param('reportname');
    my $reportfilename =
        $reportname ? "$report_id-$reportname-reportresults.$format" : "$report_id-reportresults.$format";
    my $scrubber = C4::Scrubber->new();

    ($sql, undef) = $report->prep_report( \@param_names, \@sql_params );
    my ( $sth, $q_errors ) = execute_query( { sql => $sql, report_id => $report_id } );
    unless ($q_errors and @$q_errors) {
        my ( $type, $content );
        if ($format eq 'tab') {
            $type = 'application/octet-stream';
            $content .= join("\t", header_cell_values($sth)) . "\n";
            $content = $scrubber->scrub( Encode::decode( 'UTF-8', $content ) );
            while ( my $row = $sth->fetchrow_arrayref() ) {
                $content .= join( "\t", map { $_ // '' } @$row ) . "\n";
                $content = $scrubber->scrub( $content );
            }
        } else {
            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, 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";

                } else {
                    push @$q_errors, { combine => 'HEADER ROW: ' . $csv->error_diag() };
                }
                while ( my $row = $sth->fetchrow_arrayref() ) {
                    if ( $csv->combine(@$row) ) {
                        $content .= $scrubber->scrub( $csv->string() ) . "\n";

                    } else {
                        push @$q_errors, { combine => $csv->error_diag() };
                    }
                }
            }
            elsif ( $format eq 'ods' ) {
                $type = 'application/vnd.oasis.opendocument.spreadsheet';
                my $ods_fh = File::Temp->new( UNLINK => 0 );
                my $ods_filepath = $ods_fh->filename;
                my $ods_content;

                # First line is headers
                my @headers = header_cell_values($sth);
                push @$ods_content, \@headers;

                # Other line in Unicode
                my $sql_rows = $sth->fetchall_arrayref();
                foreach my $sql_row (@$sql_rows) {
                    my @content_row;
                    foreach my $sql_cell (@$sql_row) {
                        push @content_row, $scrubber->scrub( Encode::encode( 'UTF8', $sql_cell ) );

                    }
                    push @$ods_content, \@content_row;
                }

                # Process
                generate_ods($ods_filepath, $ods_content);

                # Output
                binmode(STDOUT);
                open $ods_fh, '<', $ods_filepath;
                $content .= $_ while <$ods_fh>;
                unlink $ods_filepath;
            }
            elsif ( $format eq 'template' ) {
                my $template_id     = $input->param('template');
                my $notice_template = Koha::Notice::Templates->find($template_id);
                my $data            = $sth->fetchall_arrayref( {} );
                $content = process_tt(
                    $notice_template->content,
                    {
                        data         => $data,
                        report_id    => $report_id,
                        for_download => 1,
                    }
                );
                $reportfilename = process_tt(
                    $notice_template->title,
                    {
                        data      => $data,
                        report_id => $report_id,
                    }
                );
            }
        }
        print $input->header(
            -type => $type,
            -attachment=> $reportfilename
        );
        print $content;

        foreach my $err (@$q_errors, @errors) {
            print "# ERROR: " . (map {$_ . ": " . $err->{$_}} keys %$err) . "\n";
        }   # here we print all the non-fatal errors at the end.  Not super smooth, but better than nothing.
        exit;
    }
    $template->param(
        'sql'           => $sql,
        'execute'       => 1,
        'name'          => 'Error exporting report!',
        'notes'         => '',
        'errors'        => $q_errors,
    );
}

elsif ( $op eq 'add_form_sql' || $op eq 'duplicate' ) {

    my ($group, $subgroup, $sql, $reportname, $notes);
    if ( $input->param('sql') ) {
        $group      = $input->param('report_group');
        $subgroup   = $input->param('report_subgroup');
        $sql        = $input->param('sql') // '';
        $reportname = $input->param('reportname') // '';
        $notes      = $input->param('notes') // '';
    }
    elsif ( my $report_id = $input->param('id') ) {
        my $report = Koha::Reports->find($report_id);
        $group      = $report->report_group;
        $subgroup   = $report->report_subgroup;
        $sql        = $report->savedsql // '';
        $reportname = $report->report_name // '';
        $notes      = $report->notes // '';
    }

    my $tables = get_tables();

    $template->param(
        sql        => $sql,
        reportname => $reportname,
        notes      => $notes,
        'create' => 1,
        'groups_with_subgroups' => groups_with_subgroups($group, $subgroup),
        'public' => '0',
        'cache_expiry' => 300,
        'usecache' => $usecache,
        'tables' => $tables,

    );
}

if ($op eq 'run'){
>>>>>>> Bug 33339: Prevent Formula Injection (CSV Injection) in CSV files

- KEPT HEAD, and then also did the following:

In this block: 
elsif ($phase eq 'Export'){

changed this:
my $csv = Text::CSV::Encoded->new({ encoding_out => 'UTF-8', sep_char => $delimiter});

to this:
my $csv = Text::CSV::Encoded->new({ encoding_out => 'UTF-8', sep_char => $delimiter, formula => 'empty' });

PATCH 2 - I did not apply "172089 - Bug 33339: (Follow up) Fix more cases of formula=empty" to 23.05.x-security 

The conflict was "deleted by us: Koha/ERM/EUsage/SushiCounter.pm" - so 23.05.x doesn't use SushiCounter.pm?

Therefore, I left the follow up patch out.

> there were a couple of conflicts

You can get from my branch 23.11.x

Oh I see you managed to resolv the conflicts :
71673935ae Bug 33339: Prevent Formula Injection (CSV Injection) in CSV files

Great ;)