Summary: | Circulation returns vulnerable to reflected XSS | ||
---|---|---|---|
Product: | Koha | Reporter: | David Cook <dcook> |
Component: | Circulation | Assignee: | 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
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 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> It works! 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. (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? (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.. (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. 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> Note: QA tool passes Moving this to BLOCKED in view of progress on bug bug 38467. (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. Back to SO. Hope to get here soon. Anyone else is fine too.. Why don't we fix all the Dopop calls here? (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 ;) 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> (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. (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... (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. (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. Applied to 24.05.x-security Applied to 23.11.x-security Applied to 24.11.x-security Please rebase for 22.11.x Applied to 22.11.x-security Pushed for 25.05! Well done everyone, thank you! |