Bug 26592 - XSS vulnerability when ysearch is used
Summary: XSS vulnerability when ysearch is used
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 26572
  Show dependency treegraph
 
Reported: 2020-10-01 09:22 UTC by Jonathan Druart
Modified: 2021-12-13 21:11 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.05, 19.11.11, 19.05.16


Attachments
Bug 26592: Prevent XSS vulnerabilities when circ/ysearch.pl is used (9.99 KB, patch)
2020-10-01 09:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26592: Prevent XSS vulnerabilities when circ/ysearch.pl is used (10.05 KB, patch)
2020-10-02 09:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26592: [20.05] Prevent XSS vulnerabilities when circ/ysearch.pl is used (9.65 KB, patch)
2020-10-09 07:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26592: [19.11] Prevent XSS vulnerabilities when circ/ysearch.pl is used (9.65 KB, patch)
2020-10-09 07:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26592: [19.05] Prevent XSS vulnerabilities when circ/ysearch.pl is used (8.38 KB, patch)
2020-10-09 07:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26592: Prevent XSS vulnerabilities when circ/ysearch.pl is used (10.12 KB, patch)
2020-10-15 12:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26592: (follow-up) fix typo (1.23 KB, patch)
2020-10-15 13:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26592: [19.11] (rmaint follow-up) Fix JS error when using patron autocomplete searchbar (1.98 KB, patch)
2020-10-27 20:08 UTC, Aleisha Amohia
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-10-01 09:22:09 UTC
circ/ysearch.pl is returning patron's information (firstname, surname, address, etc.) to the template to build a list of patron matching the query.
These information are displayed without being escaped.
Comment 1 Jonathan Druart 2020-10-01 09:23:37 UTC
Created attachment 111026 [details] [review]
Bug 26592: Prevent XSS vulnerabilities when circ/ysearch.pl is used
Comment 2 Jonathan Druart 2020-10-01 09:25:17 UTC
I don't want to let more information, on purpose.
This sounds quite bad to me as the patron's info can be edited OPAC side.

You need to test the different places a patron search is done and results returned by ysearch (git grep circ/ysearch.pl will give you a list).

Ping me on IRC if you need more info.
Comment 3 Jonathan Druart 2020-10-01 09:26:24 UTC
A simple occurrence to test:
Use "edna" and fill address with <script>alert("booh!");</script>
Homepage, checkout tab, search for "edn", and wait for the AJAX returns
Comment 4 Martin Renvoize 2020-10-02 09:31:10 UTC
Created attachment 111075 [details] [review]
Bug 26592: Prevent XSS vulnerabilities when circ/ysearch.pl is used

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2020-10-02 09:32:08 UTC
Issue confirmed and resolution confirmed.  I also tested in value_builder, patrons and tag review area's and confirmed the problem is not present there (inside JS)

Signing off.
Comment 6 Victor Grousset/tuxayo 2020-10-08 23:23:45 UTC
Note for backporting: there are differences from master and 20.05 that will cause a conflict that is error prone to solve. IIUC To solve one must manually apply the escaping to each element of the concatenation. And it's unclear (for me) in which case a ternary operator is needed.

(I mistakenly tried to backport now but at least that allowed to foresee that :) )
Comment 7 Jonathan Druart 2020-10-09 07:33:54 UTC
Created attachment 111384 [details] [review]
Bug 26592: [20.05] Prevent XSS vulnerabilities when circ/ysearch.pl is used
Comment 8 Jonathan Druart 2020-10-09 07:34:36 UTC
Created attachment 111385 [details] [review]
Bug 26592: [19.11] Prevent XSS vulnerabilities when circ/ysearch.pl is used
Comment 9 Jonathan Druart 2020-10-09 07:35:21 UTC
Created attachment 111386 [details] [review]
Bug 26592: [19.05] Prevent XSS vulnerabilities when circ/ysearch.pl is used
Comment 10 Jonathan Druart 2020-10-09 07:35:57 UTC
I have attached the patches for stable branches, please test them!
Comment 11 Victor Grousset/tuxayo 2020-10-10 10:36:59 UTC
Thanks Jonathan for the patches, it works :D
Comment 12 Hayley Pelham 2020-10-13 22:15:55 UTC
Backported to 19.11.x for 19.11.11
Comment 13 Hayley Pelham 2020-10-13 22:17:33 UTC
Clar(In reply to Hayley Mapley from comment #12)
> Backported to 19.11.x for 19.11.11

Clarifying, 19.11.1x security branch
Comment 14 Kyle M Hall 2020-10-15 12:31:59 UTC
Created attachment 111722 [details] [review]
Bug 26592: Prevent XSS vulnerabilities when circ/ysearch.pl is used

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>


 Current status: Signed Off
Comment 15 Owen Leonard 2020-10-15 13:30:43 UTC
This change to js_includes.inc is incorrect:

+   + ( item.zipcode ? item.city.escapeHtml()    : "" ) + " "

It should be:

+   + ( item.zipcode ? item.zipcode.escapeHtml()    : "" ) + " "
Comment 16 Jonathan Druart 2020-10-15 13:42:33 UTC
Created attachment 111737 [details] [review]
Bug 26592: (follow-up) fix typo
Comment 17 Jonathan Druart 2020-10-15 13:43:40 UTC
Thanks Owen!

RMaints: The issue was only in the patch for master!
Comment 18 Victor Grousset/tuxayo 2020-10-18 14:39:45 UTC
Backported to 19.05.x security branch for 19.05.16
Comment 19 Lucas Gass 2020-10-27 19:33:41 UTC
There is a problem when applying this to the 20.05.x branch with what displays in the autocomplete form:

https://snipboard.io/xwJHsa.jpg
Comment 20 Aleisha Amohia 2020-10-27 20:04:41 UTC
(In reply to Lucas Gass from comment #19)
> There is a problem when applying this to the 20.05.x branch with what
> displays in the autocomplete form:
> 
> https://snipboard.io/xwJHsa.jpg

I see this bug, and 19.05.x will have the same problem.

In js_includes.inc, item.firstname.escapeHtml should be item.firstname.escapeHtml()
Comment 21 Aleisha Amohia 2020-10-27 20:08:37 UTC
Created attachment 112595 [details] [review]
Bug 26592: [19.11] (rmaint follow-up) Fix JS error when using patron autocomplete searchbar
Comment 22 Victor Grousset/tuxayo 2020-10-27 22:20:11 UTC
follow-up applied and tested on 19.05.x, thanks! :)
Comment 23 Lucas Gass 2020-10-27 22:46:02 UTC
backported to 20.05.x for 20.05.05
Comment 24 Jonathan Druart 2020-11-02 14:56:27 UTC
And finally pushed to master for 20.11!
Comment 25 Didier Gautheron 2021-03-18 14:05:29 UTC
There's a fix in 20.05 git not in these patches which could be backport to 19.11:

commit f67bd64dee422f380824a5520e7a383e3888da97

--- a/koha-tmpl/intranet-tmpl/prog/en/includes/js_includes.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/js_includes.inc
@@ -103,11 +103,11 @@
                         var cardnumber = "";
                         if( item.cardnumber != "" ){
                             // Display card number in parentheses if it exists
-                            cardnumber = " (" + ( item.cardnumber ? item.surname.escapeHtml() : "" ) + ") ";
+                            cardnumber = " (" + ( item.cardnumber ? item.cardnumber.escapeHtml() : "" ) + ") ";
                         }

ie use cardnumber not surname if there's a cardnumber.
Comment 26 Victor Grousset/tuxayo 2021-03-18 21:04:45 UTC
Thanks Didier,
Backported: Pushed to 19.11.x branch for 19.11.16