Bug 37720 - XSS (and bustage) in label creator
Summary: XSS (and bustage) in label creator
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Label/patron card printing (show other bugs)
Version: unspecified
Hardware: All All
: P3 major
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-08-23 19:30 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.19
Circulation function:


Attachments
Sample label records (1.20 KB, application/marc)
2024-08-23 19:38 UTC, Phil Ringnalda
Details
Bug 37720: XSS (and bustage) in label creator (4.89 KB, patch)
2024-08-23 20:09 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37720: XSS (and bustage) in label creator (4.95 KB, patch)
2024-08-26 02:14 UTC, David Cook
Details | Diff | Splinter Review
[alternative-patch] Bug 37720: Prevent XSS in label creator (3.26 KB, patch)
2024-08-28 10:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37720: [Alternate patch] Prevent XSS in label creator (3.31 KB, patch)
2024-08-29 00:14 UTC, David Cook
Details | Diff | Splinter Review
Bug 37720: Prevent XSS in label creator (3.35 KB, patch)
2024-08-29 01:56 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37720: Prevent XSS in label creator (4.96 KB, patch)
2024-09-16 16:22 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37720: (follow-up) Adjust tests (5.48 KB, patch)
2024-09-30 20:46 UTC, Lucas Gass (lukeg)
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-23 19:30:19 UTC
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.
Comment 1 Phil Ringnalda 2024-08-23 19:38:25 UTC
Created attachment 170675 [details]
Sample label records

Sample records for blowing up label batch editing
Comment 2 Phil Ringnalda 2024-08-23 20:09:35 UTC
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
Comment 3 David Cook 2024-08-26 02:14:07 UTC
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>
Comment 4 Jonathan Druart 2024-08-28 10:19:03 UTC
Created attachment 170801 [details] [review]
[alternative-patch] Bug 37720: Prevent XSS in label creator
Comment 5 Jonathan Druart 2024-08-28 10:19:51 UTC
What about this patch? I think it's cleaner, and even make "N/A" translatable.
Comment 6 Phil Ringnalda 2024-08-28 15:01:50 UTC
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.
Comment 7 Katrin Fischer 2024-08-28 16:19:04 UTC
(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.
Comment 8 David Cook 2024-08-29 00:04:27 UTC
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.
Comment 9 David Cook 2024-08-29 00:14:02 UTC
Created attachment 170851 [details] [review]
Bug 37720: [Alternate patch] Prevent XSS in label creator

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 10 David Cook 2024-08-29 00:15:39 UTC
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.
Comment 11 Phil Ringnalda 2024-08-29 01:56:14 UTC
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>
Comment 12 Phil Ringnalda 2024-08-29 02:01:06 UTC
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.
Comment 13 Jonathan Druart 2024-08-29 09:41:37 UTC
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 ;)
Comment 14 Marcel de Rooy 2024-09-06 06:39:26 UTC
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?
Comment 15 Phil Ringnalda 2024-09-06 15:24:44 UTC
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.
Comment 16 Jonathan Druart 2024-09-09 09:15:58 UTC
Same, it works for me
Comment 17 Nick Clemens (kidclamp) 2024-09-16 16:22:58 UTC
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>
Comment 18 Wainui Witika-Park 2024-09-23 05:38:00 UTC
Applied to 23.05.x-security for 23.05.15
Comment 19 Lucas Gass (lukeg) 2024-09-30 19:02:50 UTC
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.
Comment 20 Lucas Gass (lukeg) 2024-09-30 20:46:09 UTC
Created attachment 172239 [details] [review]
Bug 37720: (follow-up) Adjust tests
Comment 21 Lucas Gass (lukeg) 2024-09-30 20:48:10 UTC
Setting back to PQA but can someone make sure those tests look OK now?
Comment 22 Wainui Witika-Park 2024-09-30 21:57:21 UTC
(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
Comment 23 Fridolin Somers 2024-10-01 13:19:22 UTC
Backported to 23.11.x-security
Comment 24 Lucas Gass (lukeg) 2024-10-09 14:31:53 UTC
Backported to 24.05.x for 24.05.04
Comment 25 Katrin Fischer 2024-10-18 10:17:34 UTC
Pushed for 24.11!

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