Summary: | XSS (and bustage) in label creator | ||
---|---|---|---|
Product: | Koha | Reporter: | Phil Ringnalda <phil> |
Component: | Label/patron card printing | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | RESOLVED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | major | ||
Priority: | P3 | CC: | dcook, didier.gautheron, fridolin.somers, jesse, jonathan.druart, katrin.fischer, laurent.ducos, lisette, lucas, m.de.rooy, martin.renvoize, nick, philippe.blouin, tomascohen, wainuiwitikapark |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | 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.19,22.11.25
|
Circulation function: | |
Attachments: |
Sample label records
Bug 37720: XSS (and bustage) in label creator Bug 37720: XSS (and bustage) in label creator [alternative-patch] Bug 37720: Prevent XSS in label creator Bug 37720: [Alternate patch] Prevent XSS in label creator Bug 37720: Prevent XSS in label creator Bug 37720: Prevent XSS in label creator Bug 37720: (follow-up) Adjust tests |
Description
Phil Ringnalda
2024-08-23 19:30:19 UTC
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 |