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.
Created attachment 111026 [details] [review] Bug 26592: Prevent XSS vulnerabilities when circ/ysearch.pl is used
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.
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
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>
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.
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 :) )
Created attachment 111384 [details] [review] Bug 26592: [20.05] Prevent XSS vulnerabilities when circ/ysearch.pl is used
Created attachment 111385 [details] [review] Bug 26592: [19.11] Prevent XSS vulnerabilities when circ/ysearch.pl is used
Created attachment 111386 [details] [review] Bug 26592: [19.05] Prevent XSS vulnerabilities when circ/ysearch.pl is used
I have attached the patches for stable branches, please test them!
Thanks Jonathan for the patches, it works :D
Backported to 19.11.x for 19.11.11
Clar(In reply to Hayley Mapley from comment #12) > Backported to 19.11.x for 19.11.11 Clarifying, 19.11.1x security branch
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
This change to js_includes.inc is incorrect: + + ( item.zipcode ? item.city.escapeHtml() : "" ) + " " It should be: + + ( item.zipcode ? item.zipcode.escapeHtml() : "" ) + " "
Created attachment 111737 [details] [review] Bug 26592: (follow-up) fix typo
Thanks Owen! RMaints: The issue was only in the patch for master!
Backported to 19.05.x security branch for 19.05.16
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
(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()
Created attachment 112595 [details] [review] Bug 26592: [19.11] (rmaint follow-up) Fix JS error when using patron autocomplete searchbar
follow-up applied and tested on 19.05.x, thanks! :)
backported to 20.05.x for 20.05.05
And finally pushed to master for 20.11!
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.
Thanks Didier, Backported: Pushed to 19.11.x branch for 19.11.16