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.
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()?
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.
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.
Created attachment 170419 [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 the patch, download the test record, 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
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.
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
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>
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>
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.
Applied to 23.05.x-security
Backported to 23.11.x-security
Backported to 24.05.x for 24.05.04
Pushed for 24.11! Well done everyone, thank you!
Pushed to 22.11.x. Will be in next point release. 22.11.25