Bug 37727 - CVE-2024-24337 - Fix CSV formula injection - client side (DataTables)
Summary: CVE-2024-24337 - Fix CSV formula injection - client side (DataTables)
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-08-26 07:59 UTC by Magnus Enger
Modified: 2025-01-20 10:47 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00,24.11.01,24.05.06,23.11.11,22.11.23
Circulation function:


Attachments
Bug 37727: Prevent CSV Formula injection via DataTables (4.47 KB, patch)
2024-10-17 05:38 UTC, David Cook
Details | Diff | Splinter Review
Bug 37727: Prevent CSV Formula injection via DataTables (4.59 KB, patch)
2024-10-18 09:18 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 37727: Prevent CSV Formula injection via DataTables (4.66 KB, patch)
2024-11-14 20:15 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2024-08-26 07:59:31 UTC
See "Bug 33339 - Formula Injection (CSV Injection) in Export Functionality" for background.

DataTables can export CSV from tables, and the resulting CSV can include malicious formulas.
Comment 1 Magnus Enger 2024-08-27 08:44:27 UTC
In bug 33339, comment 3 David Cook said: 

> (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...)

Could you share it here, as a first step? 

I have found that commenting out this code in /home/magnus/code/kohaclone/koha-tmpl/intranet-tmpl/prog/en/includes/columns_settings.inc removes some of the CSV and Excel options from tables, but not from all of them:

 65     var export_buttons = [
 66         // {
 67         //     extend: 'csvHtml5',
 68         //     text: _("CSV"),
 69         //     exportOptions: {
 70         //         columns: exportColumns,
 71         //         rows: exportRows,
 72         //         format:  export_format
 73         //     },
 74         // },
 75         {
 76             extend: 'copyHtml5',
 77             text: _("Copy"),
 78             exportOptions: {
 79                 columns: exportColumns,
 80                 rows: exportRows,
 81                 format:  export_format
 82             },
 83         },
 84         {
 85             extend: 'print',
 86             text: _("Print"),
 87             exportOptions: {
 88                 columns: exportColumns,
 89                 rows: exportRows,
 90                 format:  export_format
 91             },
 92         }
 93     ];
 94 
 95     [% IF Koha.Preference("CurrencyFormat") != 'FR' %]
 96         // export_buttons.unshift (
 97         //     {
 98         //         extend: 'excelHtml5',
 99         //         text: _("Excel"),
100         //         exportOptions: {
101         //             columns: exportColumns,
102         //             rows: exportRows,
103         //             format:  export_format
104         //         },
105         //     }
106         // );
107     [% ELSE %]
108         // export_buttons.unshift (
109         //     {
110         //         extend: 'excelHtml5',
111         //         text: _("Excel"),
112         //         exportOptions: {
113         //             columns: exportColumns,
114         //             rows: exportRows,
115         //             format:  export_numeric
116         //         },
117         //     }
118         // );
119     [% END %]

Excel and CSV are still options on the item table on record detail view (e.g. /cgi-bin/koha/catalogue/detail.pl?biblionumber=360).
Comment 2 Jonathan Druart 2024-08-27 09:22:41 UTC
(In reply to Magnus Enger from comment #1)
> Excel and CSV are still options on the item table on record detail view
> (e.g. /cgi-bin/koha/catalogue/detail.pl?biblionumber=360).

DTs using the Koha REST API are using kohaTable that is defined in koha-tmpl/intranet-tmpl/prog/js/datatables.js

Same at the OPAC you will have koha-tmpl/opac-tmpl/bootstrap/en/includes/columns_settings.inc and koha-tmpl/opac-tmpl/bootstrap/js/datatables.js
Comment 3 David Cook 2024-08-28 01:05:07 UTC
(In reply to Magnus Enger from comment #1)
> In bug 33339, comment 3 David Cook said: 
> 
> > (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...)
> 
> Could you share it here, as a first step? 

It looks like I already did it as bug 33377. I'll add you on that one.
Comment 4 Magnus Enger 2024-08-28 12:50:22 UTC
(In reply to David Cook from comment #3)
> It looks like I already did it as bug 33377. I'll add you on that one.

Cool, thanks! But that is a fix just for that one table. 

What is the change we would like to see upstream, in a nutshell? Any cell that would start with an equals sign (=) should be left empty?
Comment 5 David Cook 2024-08-28 23:10:10 UTC
(In reply to Magnus Enger from comment #4)
> (In reply to David Cook from comment #3)
> > It looks like I already did it as bug 33377. I'll add you on that one.
> 
> Cool, thanks! But that is a fix just for that one table. 
> 
> What is the change we would like to see upstream, in a nutshell? Any cell
> that would start with an equals sign (=) should be left empty?

So bug 33377 isn't about formula injection. It's just about preventing the export of patron data via that particular UI.  (Other places that patron data get exported have other protections locally.)

For bug 37727 I think we'd need to actually fix DataTables. I remember writing something about this previously on a different bug...
Comment 6 David Cook 2024-10-17 01:06:00 UTC
Note: A CVE was opened for this on February 12 2024: https://app.opencve.io/cve/CVE-2024-24337
Comment 7 David Cook 2024-10-17 01:12:14 UTC
Ah right, I think the problem isn't with "DataTables" itself but rather with the "Buttons" extension.
Comment 8 David Cook 2024-10-17 01:21:18 UTC
./koha-tmpl/intranet-tmpl/lib/datatables/datatables.js comes with a rebuilder URL but since DataTables 1.x is no longer supported and DataTables 2.x is the new hotness, that rebuilder only works with 1.x.

However, this should work:

https://datatables.net/legacy/v1/download/#dt/jszip-3.10.1/pdfmake-0.2.7/dt-1.13.6/b-2.4.2/b-colvis-2.4.2/b-html5-2.4.2/b-print-2.4.2/fh-3.4.0/r-2.5.0

Unfortunately, it looks like we're already using the newest version of everything except DataTables which is now 1.13.11 and we're on 1.13.6.
Comment 9 David Cook 2024-10-17 01:39:58 UTC
This is one of the conversations about this topic but I think there's another one...

https://datatables.net/forums/discussion/57856/excel-command-injection
Comment 10 David Cook 2024-10-17 03:06:15 UTC
I've created a new discussion on the DataTables forum for this with lots of information: https://datatables.net/forums/discussion/80069/csv-formula-injection-vulnerability-in-buttons-extension

In the meantime, I'm going to look to see if there's any workarounds that we can put in place in the short-term...
Comment 11 David Cook 2024-10-17 03:46:12 UTC
(In reply to Magnus Enger from comment #1)
> I have found that commenting out this code in
> /home/magnus/code/kohaclone/koha-tmpl/intranet-tmpl/prog/en/includes/
> columns_settings.inc removes some of the CSV and Excel options from tables,
> but not from all of them:

Oh man... it looks like there's duplicated code in koha-tmpl/intranet-tmpl/prog/js/datatables.js as well as in ./koha-tmpl/intranet-tmpl/prog/en/includes/columns_settings.inc

I'm looking at implementing a workaround while we wait on upstream, and this... was driving me crazy.
Comment 12 David Cook 2024-10-17 03:50:52 UTC
Going to have a late lunch then keep working on a hopefully robust workaround.

I've got a PoC in progress that fixes /cgi-bin/koha/members/member.pl at least...
Comment 13 David Cook 2024-10-17 05:15:13 UTC
Ok, so the Buttons extension already auto-escapes double quotes in the cell value, so that's awesome: https://github.com/DataTables/Buttons/blob/master/js/buttons.html5.js#L274

So the workaround I've got should do the trick. I've shared it and some info with upstream, so we'll see what they say.
Comment 14 David Cook 2024-10-17 05:18:21 UTC
Ugh koha-tmpl/intranet-tmpl/prog/en/includes/columns_settings.inc has some additional weirdness with the "export_numeric" variable, so France needs some extra handling...
Comment 15 David Cook 2024-10-17 05:38:55 UTC Comment hidden (obsolete)
Comment 16 David Cook 2024-10-17 05:39:37 UTC
It's not a DRY solution, but it's easy to read and hopefully easy to understand, which I figured is more important in the short-term.
Comment 17 Magnus Enger 2024-10-18 09:18:11 UTC
Created attachment 172942 [details] [review]
Bug 37727: Prevent CSV Formula injection via DataTables

This change prevents CSV Formula injection on DataTables exports
by escaping formula with a single quote prefix as per OWASP recommendations.

Test plan:
0. Apply patch
1. Go to http://localhost:8081/cgi-bin/koha/members/memberentry.pl
?op=edit_form&destination=circ&borrowernumber=51
2. Add the following in a "Circulation note"
=SUM(1+1)
3. Go to http://localhost:8081/cgi-bin/koha/members/member.pl
?quicksearch=1&circsearch=1&searchmember=koha
4. Click "Export" and choose "Excel" and "CSV"
5. Open those downloaded files in Excel
6. Note that the =SUM(1+1) function is prefixed with a single quote,
and is not automatically executed

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised. The problematic "cell" is exported as "'=SUM(1+1)".
Comment 18 Chris Cormack 2024-11-14 20:15:31 UTC
Created attachment 174535 [details] [review]
Bug 37727: Prevent CSV Formula injection via DataTables

This change prevents CSV Formula injection on DataTables exports
by escaping formula with a single quote prefix as per OWASP recommendations.

Test plan:
0. Apply patch
1. Go to http://localhost:8081/cgi-bin/koha/members/memberentry.pl
?op=edit_form&destination=circ&borrowernumber=51
2. Add the following in a "Circulation note"
=SUM(1+1)
3. Go to http://localhost:8081/cgi-bin/koha/members/member.pl
?quicksearch=1&circsearch=1&searchmember=koha
4. Click "Export" and choose "Excel" and "CSV"
5. Open those downloaded files in Excel
6. Note that the =SUM(1+1) function is prefixed with a single quote,
and is not automatically executed

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Works as advertised. The problematic "cell" is exported as "'=SUM(1+1)".
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 19 Katrin Fischer 2024-11-22 13:16:41 UTC
We will have to schedule this with RMaints for the next maintenance release.
Comment 20 Katrin Fischer 2025-01-14 14:50:04 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 21 Katrin Fischer 2025-01-17 11:47:54 UTC
These patches have been included in all maintained versions, see "Version(s) released in".
Comment 22 Jonathan Druart 2025-01-20 09:01:03 UTC
Now we should upgrade to the last version of DataTables and remove this from Koha I guess...

It's fixed there.
Comment 23 Katrin Fischer 2025-01-20 10:47:37 UTC
(In reply to Jonathan Druart from comment #22)
> Now we should upgrade to the last version of DataTables and remove this from
> Koha I guess...
> 
> It's fixed there.

Shoudl we open a separate bug report for the update?