Bug 38969 - Reflected XSS vulnerability in tags
Summary: Reflected XSS vulnerability in tags
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Jonathan Druart
QA Contact: David Cook
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-01-26 00:31 UTC by eandudley30
Modified: 2025-04-28 16:54 UTC (History)
19 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00,24.11.04,24.05.09,23.11.14,22.11.26
Circulation function:


Attachments
Screenshot of XSS (74.51 KB, image/png)
2025-01-26 00:31 UTC, eandudley30
Details
Bug 38969: Prevent XSS in tags/review.pl (2.02 KB, patch)
2025-01-27 08:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38969: Prevent JS error when adding tags with quote at the OPAC (1.49 KB, patch)
2025-01-27 08:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38969: Prevent XSS in tags/review.pl (2.34 KB, patch)
2025-03-20 15:55 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 38969: Prevent JS error when adding tags with quote at the OPAC (1.62 KB, patch)
2025-03-20 15:55 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 38969: [alternate] Prevent self-XSS when testing tags in staff interface (2.62 KB, patch)
2025-03-25 00:56 UTC, David Cook
Details | Diff | Splinter Review
Bug 38969: Prevent XSS in tags/review.pl (2.40 KB, patch)
2025-03-25 22:53 UTC, David Cook
Details | Diff | Splinter Review
Bug 38969: Prevent JS error when adding tags with quote at the OPAC (1.68 KB, patch)
2025-03-25 22:53 UTC, David Cook
Details | Diff | Splinter Review
Bug 38969: (QA follow-up) fix tidy (727 bytes, patch)
2025-03-25 22:53 UTC, David Cook
Details | Diff | Splinter Review
[24.05] Bug 38969 (3.10 KB, patch)
2025-04-23 00:40 UTC, Wainui Witika-Park
Details | Diff | Splinter Review
Bug 38969: Prevent XSS in tags/review.pl and Prevent JS error when adding tags with quote at the OPAC (3.18 KB, patch)
2025-04-23 00:42 UTC, Wainui Witika-Park
Details | Diff | Splinter Review
[24.05] Bug 38969: Prevent XSS in tags/review.pl and Prevent JS error when adding tags with quote at the OPAC (3.19 KB, patch)
2025-04-23 00:43 UTC, Wainui Witika-Park
Details | Diff | Splinter Review
[24.05] Bug 38969: Prevent XSS in tags/review.pl and Prevent JS error when adding tags with quote at the OPAC (3.37 KB, patch)
2025-04-23 00:48 UTC, Wainui Witika-Park
Details | Diff | Splinter Review
[22.11] Bug 38969: Prevent XSS in tags/review.pl and Prevent JS error when adding tags with quote at the OPAC (3.38 KB, patch)
2025-04-24 15:10 UTC, Paul Derscheid
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description eandudley30 2025-01-26 00:31:51 UTC
Created attachment 177148 [details]
Screenshot of XSS

CWE-79: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting')

CVSS: 5.8 (Medium)

Open Koha and navigate to /cgi-bin/koha/tags/review.pl
Insert Payload: <script> alert("XSS FOUND!") </script>
XSS Triggered when tag is submitted.
Comment 1 Jonathan Druart 2025-01-27 08:57:57 UTC
Created attachment 177159 [details] [review]
Bug 38969: Prevent XSS in tags/review.pl
Comment 2 Jonathan Druart 2025-01-27 08:58:00 UTC
Created attachment 177160 [details] [review]
Bug 38969: Prevent JS error when adding tags with quote at the OPAC

Uncaught SyntaxError: missing ) after argument list

There are way more to fix in this controller...
Comment 3 Magnus Enger 2025-01-30 07:47:44 UTC
(In reply to Jonathan Druart from comment #2)
> Created attachment 177160 [details] [review] [review]
> Bug 38969: Prevent JS error when adding tags with quote at the OPAC
> 
> Uncaught SyntaxError: missing ) after argument list
> 
> There are way more to fix in this controller...

Would it make sense to tackle that in a separate bug? If it only fixes errors it might be better to focus on the XSS in this bug?
Comment 4 Magnus Enger 2025-01-30 07:50:33 UTC
I tested thus:

- Go to Admin > Tools > Tags
- Paste "<script> alert("XSS FOUND!") </script>" into the input field under the "Check lists" heading
- Click on "Test"
- Get a popup saying "XSS FOUND!"
- Apply the patch
- restart all
- Paste and click again
- Get the same popup
Comment 5 Jonathan Druart 2025-01-30 08:24:03 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2025-01-30 08:24:30 UTC
(In reply to Magnus Enger from comment #4)
> I tested thus:
> 
> - Go to Admin > Tools > Tags
> - Paste "<script> alert("XSS FOUND!") </script>" into the input field under
> the "Check lists" heading
> - Click on "Test"
> - Get a popup saying "XSS FOUND!"
> - Apply the patch
> - restart all
> - Paste and click again
> - Get the same popup

Works for me: https://snipboard.io/rTwMs3.jpg
Comment 7 Magnus Enger 2025-02-24 10:20:12 UTC
(In reply to Jonathan Druart from comment #6)
> Works for me: https://snipboard.io/rTwMs3.jpg

Sounds like I missed something then. Setting back to NSO.
Comment 8 Baptiste Wojtkowski (bwoj) 2025-03-20 15:55:25 UTC
Created attachment 179553 [details] [review]
Bug 38969: Prevent XSS in tags/review.pl

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Comment 9 Baptiste Wojtkowski (bwoj) 2025-03-20 15:55:29 UTC
Created attachment 179554 [details] [review]
Bug 38969: Prevent JS error when adding tags with quote at the OPAC

Uncaught SyntaxError: missing ) after argument list

There are way more to fix in this controller...

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Comment 10 Baptiste Wojtkowski (bwoj) 2025-03-20 15:56:36 UTC
Note: had to use auto-rebase before signing off, idk if it matters
Comment 11 David Cook 2025-03-20 23:33:50 UTC
Planning to come back to this in the coming weeks, but happy for someone else to QA if they can.
Comment 12 David Cook 2025-03-24 01:20:27 UTC
I keep looking at these patches and thinking I don't love the change, but then tell myself I'll come back to it later...
Comment 13 Jonathan Druart 2025-03-24 09:41:00 UTC
(In reply to David Cook from comment #12)
> I keep looking at these patches and thinking I don't love the change, but
> then tell myself I'll come back to it later...

Please explain why you "don't love the change", otherwise it will get stuck.
Comment 14 David Cook 2025-03-24 23:39:34 UTC
(In reply to Jonathan Druart from comment #13)
> (In reply to David Cook from comment #12)
> > I keep looking at these patches and thinking I don't love the change, but
> > then tell myself I'll come back to it later...
> 
> Please explain why you "don't love the change", otherwise it will get stuck.

I suppose what I mean is that it doesn't look clean. I haven't tested it myself yet, but I imagine it works.

In any case, a cleaner solution would require refactoring the code, which would take more time and effort. After all, we're patching very old code here. 

I was just about to take a look at this, but it looks like main might be broken at the moment...
Comment 15 David Cook 2025-03-25 00:09:32 UTC
Got my KTD working again...

Do we have a test plan for this?

At the moment, the only test plan I see is this:

Test plan:
- Go to Admin > Tools > Tags
- Paste "<script> alert("XSS FOUND!") </script>" into the input field under the "Check lists" heading
- Click on "Test"

That's not really XSS, since you're doing it to yourself.

It doesn't look like that field uses stored data (so not a stored XSS) and you can't get the data into that field at all nor activate it via a GET, so it's not a reflected XSS either. Still... obviously something we want to avoid.
Comment 16 David Cook 2025-03-25 00:55:11 UTC
Not sure we need the OPAC fix here, and without a test plan, I'm not sure how to test it for sure.
Comment 17 David Cook 2025-03-25 00:56:21 UTC
Created attachment 179676 [details] [review]
Bug 38969: [alternate] Prevent self-XSS when testing tags in staff interface

This change refactors the tag test in the staff interface to
use more standard Javascript/JSON for passing messages
and writing HTML.

Test plan:
- Apply the patch and koha-plack --restart kohadev
- Go to Admin > Tools > Tags
- Paste "<script> alert("不好! XSS FOUND!") </script>" into the input field under the "Check lists" heading
- Click on "Test"
- Note that the following test appears below the Test button:
<script> alert("不好! XSS FOUND!") </script> is neither permitted nor prohibited!
Comment 18 David Cook 2025-03-25 01:00:22 UTC
I like my alternate patch better since it gets rid of the unnecessary URI encoding and instead treats the tag as a JSON string, which it is, and uses the OWASP recommended safe sink rather than our custom escaping function. 

But I'm happy to sign off on "Prevent XSS in tags/review.pl" since it does work.

Just holding off the SO/QA because of "Bug 38969: Prevent JS error when adding tags with quote at the OPAC". If you can give me a test plan and steps to reproduce the problem, then I'll happily test and QA.
Comment 20 Jonathan Druart 2025-03-25 13:06:11 UTC
(In reply to David Cook from comment #18)
> I like my alternate patch better since it gets rid of the unnecessary URI
> encoding and instead treats the tag as a JSON string, which it is, and uses
> the OWASP recommended safe sink rather than our custom escaping function. 
> 
> But I'm happy to sign off on "Prevent XSS in tags/review.pl" since it does
> work.
> 
> Just holding off the SO/QA because of "Bug 38969: Prevent JS error when
> adding tags with quote at the OPAC". If you can give me a test plan and
> steps to reproduce the problem, then I'll happily test and QA.

Enter this (with the surrounded quotes): "<script> alert("XSS FOUND!") </script>"
Comment 22 David Cook 2025-03-25 22:47:27 UTC
(In reply to Jonathan Druart from comment #20)
> Enter this (with the surrounded quotes): "<script> alert("XSS FOUND!")
> </script>"

Ah thanks for that. I can see the console error on the OPAC now with that payload. 

The error message looks a bit weird, but like you say... there's a lot of things that need fixing.
Comment 24 David Cook 2025-03-25 22:53:08 UTC
Created attachment 179708 [details] [review]
Bug 38969: Prevent XSS in tags/review.pl

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 25 David Cook 2025-03-25 22:53:11 UTC
Created attachment 179709 [details] [review]
Bug 38969: Prevent JS error when adding tags with quote at the OPAC

Uncaught SyntaxError: missing ) after argument list

There are way more to fix in this controller...

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 26 David Cook 2025-03-25 22:53:15 UTC
Created attachment 179710 [details] [review]
Bug 38969: (QA follow-up) fix tidy
Comment 29 Wainui Witika-Park 2025-04-23 00:40:12 UTC
Created attachment 181329 [details] [review]
[24.05] Bug 38969
Comment 30 Wainui Witika-Park 2025-04-23 00:42:26 UTC
Created attachment 181330 [details] [review]
Bug 38969: Prevent XSS in tags/review.pl and Prevent JS error when adding tags with quote at the OPAC
Comment 31 Wainui Witika-Park 2025-04-23 00:43:26 UTC
Created attachment 181331 [details] [review]
[24.05] Bug 38969: Prevent XSS in tags/review.pl and Prevent JS error when adding tags with quote at the OPAC
Comment 32 Wainui Witika-Park 2025-04-23 00:48:40 UTC
Created attachment 181332 [details] [review]
[24.05] Bug 38969: Prevent XSS in tags/review.pl and Prevent JS error when adding tags with quote at the OPAC

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: wainuiwitikapark <wainuiwitikapark@catalyst.net.nz>
Comment 33 Wainui Witika-Park 2025-04-23 00:56:57 UTC
Applied to 24.05.x-security for 24.05.09

========================================

I applied these patches to 24.05.x-security and they had a few minor conflict issues which were easy for me to fix (syntax issues and alignment etc)

I squashed my commits and attached them to this bug report in case anyone wants to use them...

P.S. sorry about all the attachments and obsoletes
Comment 34 Fridolin Somers 2025-04-24 12:17:37 UTC
Applied to 23.11.x-security for 23.11.14

Code is a bit different because param is "test" instead of "tag"
Comment 35 Paul Derscheid 2025-04-24 15:10:20 UTC
Created attachment 181464 [details] [review]
[22.11] Bug 38969: Prevent XSS in tags/review.pl and Prevent JS error when adding tags with quote at the OPAC

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: wainuiwitikapark <wainuiwitikapark@catalyst.net.nz>
Comment 36 Paul Derscheid 2025-04-24 19:38:53 UTC
Applied to 24.11.x-security for 24.11.04
Comment 37 Jesse Maseto 2025-04-24 19:47:11 UTC
Applied to 22.11.x-security for 22.11.26
Comment 38 Katrin Fischer 2025-04-28 16:54:48 UTC
Pushed for 25.05!

Well done everyone, thank you!