Bug 37654 - XSS in batch record import for the citation column
Summary: XSS in batch record import for the citation column
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: unspecified
Hardware: All All
: P3 major
Assignee: Phil Ringnalda
QA Contact: David Cook
URL:
Keywords: rel_22_11_candidate
Depends on:
Blocks:
 
Reported: 2024-08-15 21:00 UTC by Phil Ringnalda
Modified: 2025-03-31 09:51 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.04,23.11.09,23.05.15
Circulation function:


Attachments
Sample record for HTML escaping (271 bytes, application/marc)
2024-08-15 21:00 UTC, Phil Ringnalda
Details
Better sample for HTML escaping (440 bytes, application/marc)
2024-08-16 03:03 UTC, Phil Ringnalda
Details
Bug 37654: XSS in Batch record import for Citation column (2.68 KB, patch)
2024-08-16 03:10 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37654: XSS in Batch record import for Citation column (2.74 KB, patch)
2024-08-16 15:48 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37654: XSS in Batch record import for Citation column (2.79 KB, patch)
2024-08-21 05:24 UTC, David Cook
Details | Diff | Splinter Review
Bug 37654: XSS in Batch record import for Citation column (2.85 KB, patch)
2024-08-21 07:26 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Phil Ringnalda 2024-08-15 21:00:09 UTC
Created attachment 170412 [details]
Sample record for HTML escaping

After you stage a batch of records for import, they are displayed with a Citation column which contains the title and author. Those need to be HTML-escaped, both so that things like < will display correctly and so that your compromised vendor won't deliver a batch of XSS to you.

Steps to reproduce:
1. Download the attached "Sample record for HTML escaping"
2. Cataloging - Stage records for import - Select the downloaded file - Upload file - Stage for import
3. When the background job completes, View batch - get an alert() from the bib title, note that the author name "Person" is a link.
Comment 1 Phil Ringnalda 2024-08-16 01:37:42 UTC
It's AJAXed into a DataTable from /tools/batch_records_ajax.pl so that's probably the place to escape it, but what do we use in the rare cases we're escaping in Perl? HTML::Entities::encode()?
Comment 2 David Cook 2024-08-16 01:56:37 UTC
If it's the rest of AJAX, I'd say the place to escape is in the Javascript. 

This is a good read on how to do that well: https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html#safe-sinks

In Koha, we use escape_str() in Javascript, but it's... suboptimal imho. Still... we widely use it, so that's always an option. 

For Perl, yeah HTML::Entities would be the way to go I think.
Comment 3 Phil Ringnalda 2024-08-16 03:03:50 UTC
Created attachment 170418 [details]
Better sample for HTML escaping

I can't actually manage to get anything into ISBN, I think because it's the result of using Business::ISBN to get rid of cruft and validate the value, but I'm not going to skip escaping it so we might as well be trying to attack it in the test record anyway.
Comment 4 Phil Ringnalda 2024-08-16 03:10:45 UTC Comment hidden (obsolete)
Comment 5 David Cook 2024-08-16 03:22:08 UTC
Looks good at a glance. Might let someone else sign this one off. But I'll keep it in my queue and look back on this next week.
Comment 6 Phil Ringnalda 2024-08-16 15:48:13 UTC
Created attachment 170457 [details] [review]
Bug 37654: XSS in Batch record import for Citation column

Viewing a staged MARC record batch loads a DataTable from
/tools/batch_records_ajax.pl, and both batch_records_ajax.pl and the
DataTable just trust the author/title/isbn/issn to be free of HTML. They
shouldn't.

Test plan:
1. Without this patch applied, download attachment 170418 [details], then Cataloging
   - Stage records for import - Select the downloaded file - Upload file -
   Stage for import
2. When the background job completes, View batch - you'll get three alert()s
   from the title, author, and ISSN, and the author and ISSN displayed huge
3. Apply patch, restart_all
4. Manage staged records - click HTMLescapingimporttestrecord.mrc - get zero
   alerts and no <h2> display

Sponsored-by: Chetco Community Public Library
Comment 7 David Cook 2024-08-21 05:24:10 UTC
Created attachment 170517 [details] [review]
Bug 37654: XSS in Batch record import for Citation column

Viewing a staged MARC record batch loads a DataTable from
/tools/batch_records_ajax.pl, and both batch_records_ajax.pl and the
DataTable just trust the author/title/isbn/issn to be free of HTML. They
shouldn't.

Test plan:
1. Without this patch applied, download attachment 170418 [details], then Cataloging
   - Stage records for import - Select the downloaded file - Upload file -
   Stage for import
2. When the background job completes, View batch - you'll get three alert()s
   from the title, author, and ISSN, and the author and ISSN displayed huge
3. Apply patch, restart_all
4. Manage staged records - click HTMLescapingimporttestrecord.mrc - get zero
   alerts and no <h2> display

Sponsored-by: Chetco Community Public Library
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 8 Martin Renvoize (ashimema) 2024-08-21 07:26:43 UTC
Created attachment 170524 [details] [review]
Bug 37654: XSS in Batch record import for Citation column

Viewing a staged MARC record batch loads a DataTable from
/tools/batch_records_ajax.pl, and both batch_records_ajax.pl and the
DataTable just trust the author/title/isbn/issn to be free of HTML. They
shouldn't.

Test plan:
1. Without this patch applied, download attachment 170418 [details], then Cataloging
   - Stage records for import - Select the downloaded file - Upload file -
   Stage for import
2. When the background job completes, View batch - you'll get three alert()s
   from the title, author, and ISSN, and the author and ISSN displayed huge
3. Apply patch, restart_all
4. Manage staged records - click HTMLescapingimporttestrecord.mrc - get zero
   alerts and no <h2> display

Sponsored-by: Chetco Community Public Library
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize (ashimema) 2024-08-21 07:28:33 UTC
I'm happy with this as is.. I'm interested in Davids through about escape_string but I think we can handle that discussion on another bug.

Passing QA on this one for now to keep these important bugs moving.
Comment 10 Wainui Witika-Park 2024-09-11 05:25:38 UTC
Applied to 23.05.x-security
Comment 11 Fridolin Somers 2024-10-01 13:11:24 UTC
Backported to 23.11.x-security
Comment 12 Lucas Gass (lukeg) 2024-10-09 14:34:52 UTC
Backported to 24.05.x for 24.05.04
Comment 13 Katrin Fischer 2024-10-18 10:17:17 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 14 Jesse Maseto 2025-03-31 09:51:12 UTC
Pushed to 22.11.x. Will be in next point release. 22.11.25