Description
Jonathan Druart
2020-10-01 09:22:09 UTC
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 |