Bug 38469

Summary: Circulation returns vulnerable to reflected XSS
Product: Koha Reporter: David Cook <dcook>
Component: CirculationAssignee: David Cook <dcook>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: baptiste.wojtkowski, dcook, didier.gautheron, fridolin.somers, gmcharlt, jesse, jonathan.druart, katrin.fischer, kyle.m.hall, laurent.ducos, lisette, lucas, m.de.rooy, martin.renvoize, nick, paul.derscheid, philippe.blouin, tomascohen, victor, wainuiwitikapark
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38467
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00,24.11.02,24.05.07,23.11.12,22.11.24
Circulation function:
Attachments: Bug 38469: Replace single quotes with double quotes to prevent XSS
Bug 38469: Replace single quotes with double quotes to prevent XSS
Bug 38469: Replace single quotes with double quotes to prevent XSS
Bug 38469: Replace single quotes with double quotes to prevent XSS

Description David Cook 2024-11-18 04:39:40 UTC
I noticed that the circulation return script is vulnerable to reflected XSS
Comment 2 David Cook 2024-11-18 04:48:06 UTC
Created attachment 174657 [details] [review]
Bug 38469: Replace single quotes with double quotes to prevent XSS

This change replaces single quotes with double quotes to prevent XSS
for particular operations on the circ returns page.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/circ/returns.pl?print_slip=1&reserve_id=1
2. Note that a pring slip is generated
(you may need to allow popups)

3. To test the XSS is patched, try the proof-of-concept from the
bug report
Comment 3 Victor Grousset/tuxayo 2024-11-30 03:57:23 UTC
Created attachment 175035 [details] [review]
Bug 38469: Replace single quotes with double quotes to prevent XSS

This change replaces single quotes with double quotes to prevent XSS
for particular operations on the circ returns page.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/circ/returns.pl?print_slip=1&reserve_id=1
2. Note that a pring slip is generated
(you may need to allow popups)

3. To test the XSS is patched, try the proof-of-concept from the
bug report

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 4 Victor Grousset/tuxayo 2024-11-30 03:57:45 UTC
It works!
Comment 5 Marcel de Rooy 2024-12-06 10:44:44 UTC
Hi David,
Not sure if this patch would always work.
Doesnt it depend on the exact TT version being used? Or which characters are deemed safe or unsafe?

Note that the TT docs say:
As of version 2.28 of the Template Toolkit, the uri and url filters use the unsafe character set defined by RFC3986. This means that certain characters ("(", ")", "*", "!", "'", and '"') are now deemed unsafe and will be escaped as hex character sequences.
The ability to use the RFC3986 character set was added in 2.26 but not enabled by default; double quote was incorrectly deemed safe in 2.26 but correctly escaped in 2.27.

Depending on the TT version, we could set use_rfc3986 to escape both single and double quotes.. From 2.28 (not on D12 yet) it would not even be needed anymore.
Comment 6 David Cook 2024-12-09 00:15:56 UTC
(In reply to Marcel de Rooy from comment #5)
> Hi David,
> Not sure if this patch would always work.
> Doesnt it depend on the exact TT version being used? Or which characters are
> deemed safe or unsafe?

That's a good point. I'm assuming that people are using the Debian packages which have a minimum version of 2.27, but technically in the cpanfile it says >= 2.22:

requires 'Template', '>= 2.22, != 3.008';

But I'd say that means we need to update the cpanfile to 2.27 at least.

It looks like Phil has made that same suggestion on bug 38467.

> Note that the TT docs say:
> As of version 2.28 of the Template Toolkit, the uri and url filters use the
> unsafe character set defined by RFC3986. This means that certain characters
> ("(", ")", "*", "!", "'", and '"') are now deemed unsafe and will be escaped
> as hex character sequences.
> The ability to use the RFC3986 character set was added in 2.26 but not
> enabled by default; double quote was incorrectly deemed safe in 2.26 but
> correctly escaped in 2.27.
> 
> Depending on the TT version, we could set use_rfc3986 to escape both single
> and double quotes.. From 2.28 (not on D12 yet) it would not even be needed
> anymore.

So I've added a patch for that on bug 38467, but I think that widespread of a change might scare people?
Comment 7 Marcel de Rooy 2024-12-09 13:06:29 UTC
(In reply to David Cook from comment #6)
> > Depending on the TT version, we could set use_rfc3986 to escape both single
> > and double quotes.. From 2.28 (not on D12 yet) it would not even be needed
> > anymore.
> 
> So I've added a patch for that on bug 38467, but I think that widespread of
> a change might scare people?

Yeah could be.
But Debian suddenly jumping the TT version at some point, might be even more scary?
This is under Koha control at least..
Comment 8 David Cook 2024-12-09 22:19:21 UTC
(In reply to Marcel de Rooy from comment #7)
> > So I've added a patch for that on bug 38467, but I think that widespread of
> > a change might scare people?
> 
> Yeah could be.
> But Debian suddenly jumping the TT version at some point, might be even more
> scary?
> This is under Koha control at least..

Agreed.
Comment 9 Baptiste Wojtkowski (bwoj) 2025-01-02 15:12:23 UTC
Created attachment 176058 [details] [review]
Bug 38469: Replace single quotes with double quotes to prevent XSS

This change replaces single quotes with double quotes to prevent XSS
for particular operations on the circ returns page.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/circ/returns.pl?print_slip=1&reserve_id=1
2. Note that a pring slip is generated
(you may need to allow popups)

3. To test the XSS is patched, try the proof-of-concept from the
bug report

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Comment 10 Baptiste Wojtkowski (bwoj) 2025-01-02 15:26:34 UTC
Note: QA tool passes
Comment 11 Marcel de Rooy 2025-01-10 08:29:35 UTC
Moving this to BLOCKED in view of progress on bug bug 38467.
Comment 12 David Cook 2025-01-12 23:23:03 UTC
(In reply to Marcel de Rooy from comment #11)
> Moving this to BLOCKED in view of progress on bug bug 38467.

I figure it's still a good idea to be consistent in our quote usage. With our Javascript, everywhere else uses double quotes.
Comment 13 Marcel de Rooy 2025-01-20 10:59:49 UTC
Back to SO. Hope to get here soon. Anyone else is fine too..
Comment 14 Jonathan Druart 2025-01-23 10:44:11 UTC
Why don't we fix all the Dopop calls here?
Comment 15 David Cook 2025-01-28 02:19:42 UTC
(In reply to Jonathan Druart from comment #14)
> Why don't we fix all the Dopop calls here?

Yeah, that's a good question. I think this particular one was just noticed by accident, so I wrote a fix for it while it was still in my working memory.

Looks like Dopop is actually a few different functions and gets implemented and called in different ways as well, which is fun...

Happy for you to FQA this one and do your own version ;)
Comment 16 Marcel de Rooy 2025-02-14 07:39:30 UTC
Created attachment 178075 [details] [review]
Bug 38469: Replace single quotes with double quotes to prevent XSS

This change replaces single quotes with double quotes to prevent XSS
for particular operations on the circ returns page.

Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/circ/returns.pl?print_slip=1&reserve_id=1
2. Note that a pring slip is generated
(you may need to allow popups)

3. To test the XSS is patched, try the proof-of-concept from the
bug report

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2025-02-14 07:41:34 UTC
(In reply to Jonathan Druart from comment #14)
> Why don't we fix all the Dopop calls here?

Passing QA here now. Lets get this simple change further now and fix other stuff somewhere else.
Comment 18 Jonathan Druart 2025-02-15 05:34:18 UTC
(In reply to Marcel de Rooy from comment #17)
> (In reply to Jonathan Druart from comment #14)
> > Why don't we fix all the Dopop calls here?
> 
> Passing QA here now. Lets get this simple change further now and fix other
> stuff somewhere else.

If you only fix a part of a security problem, you are only highlight that there are other security issues...
Comment 19 David Cook 2025-02-16 22:15:20 UTC
(In reply to Jonathan Druart from comment #18)
> If you only fix a part of a security problem, you are only highlight that
> there are other security issues...

Agreed. Happy for you to FQA this one.
Comment 20 David Cook 2025-02-16 23:11:30 UTC
(In reply to Jonathan Druart from comment #18)
> (In reply to Marcel de Rooy from comment #17)
> > (In reply to Jonathan Druart from comment #14)
> > > Why don't we fix all the Dopop calls here?
> > 
> > Passing QA here now. Lets get this simple change further now and fix other
> > stuff somewhere else.
> 
> If you only fix a part of a security problem, you are only highlight that
> there are other security issues...

Then again... anyone looking at the other security patches from the last few years should be able to realise fairly easily that there are other security issues.

We get more reports than we have time to deal with.
Comment 21 Wainui Witika-Park 2025-02-17 22:52:31 UTC
Applied to 24.05.x-security
Comment 22 Fridolin Somers 2025-02-21 14:16:51 UTC
Applied to 23.11.x-security
Comment 23 Paul Derscheid 2025-02-22 18:15:18 UTC
Applied to 24.11.x-security
Comment 24 Jesse Maseto 2025-02-24 16:28:15 UTC
Please rebase for 22.11.x
Comment 25 Jesse Maseto 2025-02-24 17:07:27 UTC
Applied to 22.11.x-security
Comment 26 Katrin Fischer 2025-03-06 12:56:26 UTC
Pushed for 25.05!

Well done everyone, thank you!