The XSS is fairly unlikely: you need to import records with items, and then print labels, but I'd expect if your vendor knows enough to create an item record with a callnumber/barcode for you, you would be buying shelf-ready with the spine label and barcode already printed and attached. The bustage is much easier: have anything that would be interpreted as HTML in a title, author, or call number. Because it's being stuffed into a DataTable, it's actually quite easy to break the entire display. A callnumber that consists of "<script>" in a label batch is enough to trigger an alert() that "Something went wrong when loading the table" over a blank white page, with the only recourse being to delete the batch. One amusing source of bustage is that C4/Creators/Lib.pm thinks it is a template, and trims characters it doesn't like from the end of titles and author names, so your XSS attempt needs to not end with </script> since it will trim off the > and leave your script open until the next bit of HTML closes it. But the core of the problem is https://git.koha-community.org/Koha-community/Koha/src/commit/f44d2cccc75c614ec6f030c465d9f1d8800317d5/C4/Creators/Lib.pm#L346 - because that bit of C4 plays template and constructs the link there instead of in the template where it should be constructed, the template sticks everything in with the $raw filter, when the right thing to do is for the template to build the link, with the html filter for the title and biblionumber parts of it, and then be able to use the html filter on all the other parts since they aren't including HTML. Pending a fix for that 15 year old FIXME, the security bug fix is to HTML::Entities::encode the bits going into the _summary, and in the template only $raw output the summary, and use the html filter on the rest.
Created attachment 170675 [details] Sample label records Sample records for blowing up label batch editing
Created attachment 170676 [details] [review] Bug 37720: XSS (and bustage) in label creator Because labels/label-edit-batch.pl fills a DataTable with things that include a link created by C4/Creators/Lib.pm, it outputs them with the $raw filter, so HTML in author/title/callnumber is executed in the label batch editor. While we wait for a fix that moves the link creation into the template and out of C4, encoding HTML in Lib.pm for the bits going into the link, and switching from $raw to the html filter for the rest of the things, will at least get rid of the XSS. Test plan: 1. Without this patch, but with the patch from bug 37654 so you don't get alert()s in batch import, download attachment 170675 [details] 2. Cataloging - Stage records for import - browse to the downloaded file - Upload file - when the upload finishes Stage for import - when staging finishes View batch (get alert()s if you didn't apply bug 37654) - Import this batch into the catalog 3. Once the import finishes, Cataloging - Manage staged records 4. In the row for your import, in the # Items column, click "(Create label batch)" 5. In the "Label batch #n created" message, click the link to the batch # 6. Because the batch includes a call number with an open <script>, you'll get XSS alert()s and then one about something going wrong while loading the table, with only one of the two records showing in the batch editor 7. Apply patch, restart_all 8. Cataloging - Label creator - Manage Label batches 9. In the row for your batch, click Edit 10. You will see both labels, with their attempts at XSS visible as text rather than being interpreted as HTML Sponsored-by: Chetco Community Public Library
Created attachment 170691 [details] [review] Bug 37720: XSS (and bustage) in label creator Because labels/label-edit-batch.pl fills a DataTable with things that include a link created by C4/Creators/Lib.pm, it outputs them with the $raw filter, so HTML in author/title/callnumber is executed in the label batch editor. While we wait for a fix that moves the link creation into the template and out of C4, encoding HTML in Lib.pm for the bits going into the link, and switching from $raw to the html filter for the rest of the things, will at least get rid of the XSS. Test plan: 1. Without this patch, but with the patch from bug 37654 so you don't get alert()s in batch import, download attachment 170675 [details] 2. Cataloging - Stage records for import - browse to the downloaded file - Upload file - when the upload finishes Stage for import - when staging finishes View batch (get alert()s if you didn't apply bug 37654) - Import this batch into the catalog 3. Once the import finishes, Cataloging - Manage staged records 4. In the row for your import, in the # Items column, click "(Create label batch)" 5. In the "Label batch #n created" message, click the link to the batch # 6. Because the batch includes a call number with an open <script>, you'll get XSS alert()s and then one about something going wrong while loading the table, with only one of the two records showing in the batch editor 7. Apply patch, restart_all 8. Cataloging - Label creator - Manage Label batches 9. In the row for your batch, click Edit 10. You will see both labels, with their attempts at XSS visible as text rather than being interpreted as HTML Sponsored-by: Chetco Community Public Library Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 170801 [details] [review] [alternative-patch] Bug 37720: Prevent XSS in label creator
What about this patch? I think it's cleaner, and even make "N/A" translatable.
I like that, but how about instead of making "N/A" translatable, we make it go away? We have dozens of places where we display author if it exists, or "" if it doesn't, and this is the only one I can think of where we display a placeholder to say we tried but couldn't find one.
(In reply to Phil Ringnalda from comment #6) > I like that, but how about instead of making "N/A" translatable, we make it > go away? We have dozens of places where we display author if it exists, or > "" if it doesn't, and this is the only one I can think of where we display a > placeholder to say we tried but couldn't find one. I would quite like that, see bug 36401. But should not block the security bug.
I went to test Jonathan's patch and it looks like in "main" the "Create label batch" link no longer works. Guessing because of bug 35402.
Created attachment 170851 [details] [review] Bug 37720: [Alternate patch] Prevent XSS in label creator Signed-off-by: David Cook <dcook@prosentient.com.au>
Technically, I'd say Jonathan's patch would be the more appropriate choice. But they both work. Leaving it up to QA/RM for next steps.
Created attachment 170852 [details] [review] Bug 37720: Prevent XSS in label creator Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Not adding a new translatable string that we have every intention of then removing doesn't require blocking a security bug, it just requires removing a few characters from the patch. Done and tested. FWIW, I can't reproduce the Create label batch not working, so that must be later than the initial bug 35402 push, where I am right now.
Thanks! (In reply to Phil Ringnalda from comment #12) > Not adding a new translatable string that we have every intention of then > removing doesn't require blocking a security bug, it just requires removing > a few characters from the patch. Done and tested. Please add "Amended-by: You" and let a note about what you have done in the patch. For next time ;)
Module + $label_summary->{'_summary'} = { title => $record->{title}, author => $record->{author}, biblionumber => $record->{biblionumber} }; Template [% ELSIF ( text_field.field_name == '_summary_tbl' ) %] Looks like a typo? How was it tested?
It was tested with the test plan from my original patch, in comment 2. It *works*. The template doesn't get the return from C4::Creators::Lib::get_label_summary(), it gets the return from C4::Creators::Lib::html_table(). Along with having far more interest in the details of what the template will do than something in C4 should, what html_table() does is rename _foo to _foo_tbl.
Same, it works for me
Created attachment 171565 [details] [review] Bug 37720: Prevent XSS in label creator Because labels/label-edit-batch.pl fills a DataTable with things that include a link created by C4/Creators/Lib.pm, it outputs them with the $raw filter, so HTML in author/title/callnumber is executed in the label batch editor. While we wait for a fix that moves the link creation into the template and out of C4, encoding HTML in Lib.pm for the bits going into the link, and switching from $raw to the html filter for the rest of the things, will at least get rid of the XSS. Test plan: 1. Without this patch, but with the patch from bug 37654 so you don't get alert()s in batch import, download attachment 170675 [details] 2. Cataloging - Stage records for import - browse to the downloaded file - Upload file - when the upload finishes Stage for import - when staging finishes View batch (get alert()s if you didn't apply bug 37654) - Import this batch into the catalog 3. Once the import finishes, Cataloging - Manage staged records 4. In the row for your import, in the # Items column, click "(Create label batch)" 5. In the "Label batch #n created" message, click the link to the batch # 6. Because the batch includes a call number with an open <script>, you'll get XSS alert()s and then one about something going wrong while loading the table, with only one of the two records showing in the batch editor 7. Apply patch, restart_all 8. Cataloging - Label creator - Manage Label batches 9. In the row for your batch, click Edit 10. You will see both labels, with their attempts at XSS visible as text rather than being interpreted as HTML Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Applied to 23.05.x-security for 23.05.15
Tests are failing with this patch applied. You can now see this in the 23.05 and 24.05 security builds. Failing on main too.
Created attachment 172239 [details] [review] Bug 37720: (follow-up) Adjust tests
Setting back to PQA but can someone make sure those tests look OK now?
(In reply to Lucas Gass from comment #20) > Created attachment 172239 [details] [review] [review] > Bug 37720: (follow-up) Adjust tests Applied this to 23.05.x-security also for 23.05.15
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