Like bug 37727 it's possible for malicious formulas to be embedded in CSVs that are downloaded and run via the OPAC.
Created attachment 184698 [details] [review] Bug 40525: Update spreadsheet format to escape formulas This change updates the spreadsheet format to escape formulas using the built-in function if using Buttons 3.2.0+ or using a workaround if not. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=29 2. Click on tab 1 3. For 100$a type in the following and save: =SUM(1+1) 4. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?findborrower=42 5. Checkout barcode 39999000001310 to the patron 6. Go to http://localhost:8080/cgi-bin/koha/opac-user.pl and login as that patron 7. Download the CSV for the summary table and open in Excel 8. Note that the "Author" is "'=SUM(1+1)" and not "=SUM(1+1)" or "2"
Created attachment 187596 [details] [review] Bug 40525: Update spreadsheet format to escape formulas This change updates the spreadsheet format to escape formulas using the built-in function if using Buttons 3.2.0+ or using a workaround if not. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=29 2. Click on tab 1 3. For 100$a type in the following and save: =SUM(1+1) 4. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?findborrower=42 5. Checkout barcode 39999000001310 to the patron 6. Go to http://localhost:8080/cgi-bin/koha/opac-user.pl and login as that patron 7. Download the CSV for the summary table and open in Excel 8. Note that the "Author" is "'=SUM(1+1)" and not "=SUM(1+1)" or "2" Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Rebased for current main. Works as described. Nice!
Created attachment 188052 [details] [review] Bug 40525: Update spreadsheet format to escape formulas This change updates the spreadsheet format to escape formulas using the built-in function if using Buttons 3.2.0+ or using a workaround if not. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=29 2. Click on tab 1 3. For 100$a type in the following and save: =SUM(1+1) 4. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?findborrower=42 5. Checkout barcode 39999000001310 to the patron 6. Go to http://localhost:8080/cgi-bin/koha/opac-user.pl and login as that patron 7. Download the CSV for the summary table and open in Excel 8. Note that the "Author" is "'=SUM(1+1)" and not "=SUM(1+1)" or "2" Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Looks good to me. Seems that tabs and carriage returns are already removed by trim btw.
Can this please be rebased for 22.11?
Created attachment 188494 [details] [review] [22.11] Bug 40525: Update spreadsheet format to escape formulas This change updates the spreadsheet format to escape formulas using the built-in function if using Buttons 3.2.0+ or using a workaround if not. Test plan: 0. Apply the patch 1. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=29 2. Click on tab 1 3. For 100$a type in the following and save: =SUM(1+1) 4. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?findborrower=42 5. Checkout barcode 39999000001310 to the patron 6. Go to http://localhost:8080/cgi-bin/koha/opac-user.pl and login as that patron 7. Download the CSV for the summary table and open in Excel 8. Note that the "Author" is "'=SUM(1+1)" and not "=SUM(1+1)" or "2" Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Hi I attempted a rebase for 22.11 myself. However, I notice I am not getting the expected behaviour for this step: 8. Note that the "Author" is "'=SUM(1+1)" and not "=SUM(1+1)" or "2" I am still getting "=SUM(1+1)" after applying my patch. However, we don't use Microsoft in our organisation and so I am using LibreOffice Calc. I wonder if anyone with Excel could please test my 22.11 patch?
(In reply to Wainui Witika-Park from comment #9) > Hi I attempted a rebase for 22.11 myself. > > However, I notice I am not getting the expected behaviour for this step: > > 8. Note that the "Author" is "'=SUM(1+1)" and not "=SUM(1+1)" or "2" > > I am still getting "=SUM(1+1)" after applying my patch. > > However, we don't use Microsoft in our organisation and so I am using > LibreOffice Calc. I wonder if anyone with Excel could please test my 22.11 > patch? I suspect that your patch is probably failing. If you look at the browser console, I'm sure it'll be full of red error messages. I thought that I'd backported this to 23.11 but it looks like I'd actually backported bug 40522. They use the same concept so hopefully I can provide some clarity... -- For 23.11, I had to create a custom shim. The Buttons library is too old in older versions of Koha, so we can't use "DataTable.Buttons.stripData". The shim I created is about 125 lines long, and is inspired by DataTable.Buttons.stripData I'll look at putting something together. Other bugs with similar issues will need the same thing, so maybe they'll have to depend on this one...
So 22.11 is actually even worse than I expected so it took a bit of time but I think i've got a patch together...
Created attachment 188496 [details] [review] Bug 40525: [22.11] Update spreadsheet format to escape formulas
This has merge conflicts with 24.05.x Please resolve.
Created attachment 188514 [details] [review] Bug 40525: [24.11] Update spreadsheet format to escape formulas
Created 24.11 from 22.11 ; patch from main were not applying. While resolving conflicts, we weren't able to find the changes that should have been made to koha-tmpl/opac-tmpl/bootstrap/js/datatables.js. However we can follow the test plan successfully without.
(In reply to David Cook from comment #12) > Created attachment 188496 [details] [review] [review] > Bug 40525: [22.11] Update spreadsheet format to escape formulas Thanks so much David! This patch works as expected. I am now getting "'=SUM(1+1)" and not "=SUM(1+1)" or "2"
Created attachment 188534 [details] [review] Bug 40525: [24.05] Update spreadsheet format to escape formulas
Hmmm let me take another look at this. I don't think the shim should be needed in 24.11, but I'll do a quick review...
Ok so the original patch will only apply to 25.05 and main because bug 38255 changed how datatables get created. However, the code itself can work in 24.11, as it uses a new enough Buttons library. Here's the breakdown of DataTables and Buttons libraries per version: 22.11: DataTables 1.10.18 Buttons 1.5.2 (./koha-tmpl/opac-tmpl/lib/jquery/plugins/jquery.dataTables.min.js) 24.05: DataTables 1.10.18 Buttons 1.5.2 (./koha-tmpl/opac-tmpl/lib/jquery/plugins/jquery.dataTables.min.js) 24.11: DataTables 2.1.8 Buttons 3.1.2 (./koha-tmpl/opac-tmpl/lib/datatables/datatables.min.js) main: DataTables 2.1.8 Buttons 3.1.2 (./koha-tmpl/opac-tmpl/lib/datatables/datatables.min.js) -- It's tempting to use the buttons shim in 24.11 anyways just for ease of backporting but 24.11 is also a LTS so might be better to get it right, because a short term gain could mean long term pain. DataTables was a bit of a mess before Jonathan cleaned it up, so the 24.11 patch leaving off the datatables.js change is probably fine because I don't think the buttons in datatables.js actually get used pre-25.05.
(In reply to David Cook from comment #19) > Ok so the original patch will only apply to 25.05 and main because bug 38255 > changed how datatables get created. However, the code itself can work in > 24.11, as it uses a new enough Buttons library. Here's the breakdown of > DataTables and Buttons libraries per version: > > 22.11: DataTables 1.10.18 Buttons 1.5.2 > (./koha-tmpl/opac-tmpl/lib/jquery/plugins/jquery.dataTables.min.js) > > 24.05: DataTables 1.10.18 Buttons 1.5.2 > (./koha-tmpl/opac-tmpl/lib/jquery/plugins/jquery.dataTables.min.js) > > 24.11: DataTables 2.1.8 Buttons 3.1.2 > (./koha-tmpl/opac-tmpl/lib/datatables/datatables.min.js) > > main: DataTables 2.1.8 Buttons 3.1.2 > (./koha-tmpl/opac-tmpl/lib/datatables/datatables.min.js) > > -- > > It's tempting to use the buttons shim in 24.11 anyways just for ease of > backporting but 24.11 is also a LTS so might be better to get it right, > because a short term gain could mean long term pain. > > DataTables was a bit of a mess before Jonathan cleaned it up, so the 24.11 > patch leaving off the datatables.js change is probably fine because I don't > think the buttons in datatables.js actually get used pre-25.05. Ah wait nevermind... as per my comment on bug 37727 the formula escape was added in Buttons 3.2.0, so even though stripData is available in Buttons 3.1.2 - it's not quite comprehensive enough.
(In reply to David Cook from comment #18) > Hmmm let me take another look at this. I don't think the shim should be > needed in 24.11, but I'll do a quick review... In conclusion, I think this is all good. I'll keep in mind this experience for bugs 40524 and 40522 so that I can ease the pain of backporting.
Created attachment 188535 [details] [review] Bug 40525: [22.11] Update spreadsheet format to escape formulas
(In reply to Wainui Witika-Park from comment #22) > Created attachment 188535 [details] [review] [review] > Bug 40525: [22.11] Update spreadsheet format to escape formulas I was getting: ``` ok 132 - /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/en/includes/date-format.inc # excluding template /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/en/includes/doc-head-close.inc because it cannot stand on its own # excluding template /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc because it cannot stand on its own not ok 133 - /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/en/includes/columns_settings.inc # Failed test '/kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/en/includes/columns_settings.inc' # at xt/author/valid-templates.t line 141. # undef error - : filter not found ok 134 - /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt ``` so I added the following to columns_settings.inc: ``` [% USE raw %] ```
Good catch, Wainui. I think there might also be a problem in opac-user.pl on main but unsure. Got some more testing to do...
(In reply to David Cook from comment #24) > Good catch, Wainui. > > I think there might also be a problem in opac-user.pl on main but unsure. > Got some more testing to do... Nevermind - looks like an unrelated problem in main :)
I've been deep diving this more (for the first time since late July) and I think I made some mistakes on the backport. Not mistakes that will cause any negative impacts, but there's probably more changes than there need to be on the backports.
(In reply to David Cook from comment #26) > I've been deep diving this more (for the first time since late July) and I > think I made some mistakes on the backport. Not mistakes that will cause any > negative impacts, but there's probably more changes than there need to be on > the backports. Actually, maybe I take that back. It's tough keeping all the different permutations of DataTables straight across so many different Koha versions with different DataTables implementations. I'll be glad when 24.05 and 22.11 go away. Then we can improve 24.11 as the LTS I guess...
I've merged in v24.11.10-2 with my local changes and it's working well :)
Nice work all, pushed to main for 25.11.00