Bug 26592

Summary: XSS vulnerability when ysearch is used
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: ToolsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: 1joynelson, aleisha, didier.gautheron, fridolin.somers, hayleypelham, jonathan.druart, kyle, lucas, martin.renvoize, nick, oleonard, victor
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26102
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
Circulation function:
Bug Depends on:    
Bug Blocks: 26572    
Attachments: Bug 26592: Prevent XSS vulnerabilities when circ/ysearch.pl is used
Bug 26592: Prevent XSS vulnerabilities when circ/ysearch.pl is used
Bug 26592: [20.05] Prevent XSS vulnerabilities when circ/ysearch.pl is used
Bug 26592: [19.11] Prevent XSS vulnerabilities when circ/ysearch.pl is used
Bug 26592: [19.05] Prevent XSS vulnerabilities when circ/ysearch.pl is used
Bug 26592: Prevent XSS vulnerabilities when circ/ysearch.pl is used
Bug 26592: (follow-up) fix typo
Bug 26592: [19.11] (rmaint follow-up) Fix JS error when using patron autocomplete searchbar

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 (ashimema) 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 (ashimema) 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 (khall) 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 (lukeg) 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 (lukeg) 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