Our InfoSec group has flagged opac-shelves as being vulnerable to cross-site scripting attack. The test URL they provided: https://catalog.uttyler.edu/cgi-bin/koha/opac-shelves.pl?op=list&category=2%22%3E<script>alert(150)</script> We are running Koha version 18.11.02.000.
Please ask them more detail, if you are using 18.11.02 you should have a safe version. The category parameter is correctly escaped, in function AdjustRemark(): category = 2"><script>alert(150)</script>;
We are not seeing that code on our server (and yes we did confirm we are using 18.11.02). When we are looking here on gitHub, we don't see that that function is actually escaping the script tag. Could you give us more information about where we should be seeing that being escaped? https://github.com/Koha-Community/Koha/blob/18.11.x/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt Also (at the risk of being repetitive), that test URL did work successfully, so it does certainly appear that the script at that particular place is not being escaped.
The variable 'category' is displayed using the 'html' filter on line 972: category = [% category | html %]; I had a very quick look so maybe I missed something. Hit the page, view page source, then search 'script'. What do you see?
Hi, are you wanting what the script looks like when we go to that URL? This is what it looks like: function AdjustRemark() { var category; if( $("#category").length > 0 ) { category = $("#category").val(); } else { category = 2"><script>alert(150)</script>; } var perms = $("#allow_changes_from").val(); if( perms < 2 ) { $("#anyone_remark").hide(); } else if( category==1 ) { // If we move to Private (without shares), show Anyone remark // Note: the number of shares is not tested real-time $("#anyone_remark").show(); } else { // category==2 $("#anyone_remark").hide(); } }
needless to say, Chrome was blocking the XSS, but Firefox showed the Alert.
Do you see other occurrences of "script"?
The source of the rendered page after clicking OK on the alert() popups contains these instances of <script> if that's what you mean: <div class="pages"> <span class="inactive"><<</span> <span class="inactive"><</span> <span class="currentPage">1</span> <a href="?page=2&op=list&category=2"><script>alert(150)</script>">2</a> <a href="?page=3&op=list&category=2"><script>alert(150)</script>">3</a> <a href="?page=2&op=list&category=2"><script>alert(150)</script>" rel="next">></a> <a href="?page=3&op=list&category=2"><script>alert(150)</script>" rel="last">>></a></div>
Confirmed, with pagination.
Created attachment 86636 [details] [review] Bug 22478: Prevent XSS vulnerabilities when pagination appears This is a bad one as we thought we were XSS safe since bug 13618. The html code generated in C4::Output::pagination_bar must escape the variables and values correctly. This patch needs to be widely tested, everywhere the pagination appears, to make sure we will not introduce regressions.
Created attachment 86683 [details] [review] Bug 22478: Add tests
Created attachment 87697 [details] [review] Bug 22478: Fix POD
Created attachment 87698 [details] [review] Bug 22478: Prevent XSS vulnerabilities when pagination appears This is a bad one as we thought we were XSS safe since bug 13618. The html code generated in C4::Output::pagination_bar must escape the variables and values correctly. This patch needs to be widely tested, everywhere the pagination appears, to make sure we will not introduce regressions. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 87699 [details] [review] Bug 22478: Add tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 87700 [details] [review] Bug 22478: Fix POD Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 87701 [details] [review] Bug 22478: Fix POD Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
I couldn't run the Selenium tests, but I have tested the pagination in various places.
QA: Looking here now
(In reply to systems_analysts from comment #5) > needless to say, Chrome was blocking the XSS, but Firefox showed the Alert. Do not see the alert in FF but yes, this needs correction.
Jonathan: Your patch is not the complete answer for this. Although it probably is an improvement. I will add a small specific follow-up for the category parameter in opac-shelves too..
(In reply to Katrin Fischer from comment #11) > Created attachment 87697 [details] [review] [review] > Bug 22478: Fix POD Katrin: Koha instance kohadev-koha <kohadev-koha@kohadevbox>
Created attachment 87841 [details] [review] Bug 22478: Prevent XSS vulnerabilities when pagination appears This is a bad one as we thought we were XSS safe since bug 13618. The html code generated in C4::Output::pagination_bar must escape the variables and values correctly. This patch needs to be widely tested, everywhere the pagination appears, to make sure we will not introduce regressions. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 87842 [details] [review] Bug 22478: Add tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 87843 [details] [review] Bug 22478: Fix POD Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 87844 [details] [review] Bug 22478: (QA follow-up) Handle category in opac-shelves like a boolean The category parameter should be restricted to 1 or 2 (private/public). In order to keep same behavior, no parameter means 1 (private). Note: Adding the same line in intranet counterpart. Test plan [1] Check for category empty, '1a', '11' etc. And with script ;) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #19) > Jonathan: Your patch is not the complete answer for this. Could you explain what is missing?
Pushed to 18.11.x for 18.11.05
Created attachment 88937 [details] [review] Bug 22478: (RMaint follow-up) source t::lib::Mocks In 18.11.x the t::lib::Mocks module is not yet sourced for the relevant test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 88954 [details] [review] Bug 22478: (RMaint follow-up) Fix selenium test Seems no-one actually ran the tests during QA of this one Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Well.. I've managed to get the test past the failure to find element issue but I'm very much stuck on having it pass.. I'm never seeing the escaped script tags on the URL in line 229 like( $second_page->get_attribute('href'), qr{category=1%22%3E%3Cscript%3Ealert%28%27booh%21%27%29%3C%2Fscript%3E}, 'The second patch should displayed the variables and attributes correctly URI escaped' ); The test reports a failure because the query params are already stripped. Did you manage to run the Selenium tests Marcelr? Jonathan, any help would be really appreciated.. I've hit the very end of my knowledge now.
(In reply to Martin Renvoize from comment #29) > Jonathan, any help would be really appreciated.. I've hit the very end of my > knowledge now. Well, if you apply only my patches, the tests will pass... However I indeed mixed up the variable name public vs private, or the code 1 vs 2 :)
Hmm, the tests don't pass for me or nick on koha-testing-docker with just your patches I'm afraid. Obviously master doesn't needy first follow-up but I found it does need the second one to get as far as the last test which then fails for me at the moment as it seems that the params are stripped away rather than escaped. My hunch was that the selenium runner in koha-testing-docker has something like xss-auditor for Chrome running and so the script stuff is stripped before it hits Koha, but I couldn't vaeify that yet.
# Subtest: XSS vulnerabilities in pagination 1..3 ok 1 - No alert box displayed ok 2 - No alert box displayed, even if evil intent not ok 3 - The second patch should displayed the variables and attributes correctly URI escaped # Failed test 'The second patch should displayed the variables and attributes correctly URI escaped' # at t/db_dependent/selenium/regressions.t line 229. # 'http://koha:8080/cgi-bin/koha/opac-shelves.pl?page=2&category=2&op=list' # doesn't match '(?^u:category=2%22%3E%3Cscript%3Ealert%28%27booh%21%27%29%3C%2Fscript%3E)' # Looks like you failed 1 test of 3. not ok 5 - XSS vulnerabilities in pagination # Failed test 'XSS vulnerabilities in pagination' # at t/db_dependent/selenium/regressions.t line 232. # Looks like you failed 1 test of 5. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/5 subtests Test Summary Report ------------------- t/db_dependent/selenium/regressions.t (Wstat: 256 Tests: 5 Failed: 1) Failed test: 5 Non-zero exit status: 1 Files=1, Tests=5, 214 wallclock secs ( 0.04 usr 0.02 sys + 6.24 cusr 2.05 csys = 8.35 CPU) Result: FAIL
Martin, could you confirm you did not apply the QA follow-up?
Yup, I tried without any followups the with marcels and then I added the last one as my attempt to get things working. The middle one is 18.11 only, just sourcing t::lib::Mocks.
What do you see on the screenshot? The test is failing on master as well?
Right.. now I'm even more confused. I think there's a race condition in the test somewhere as I'm getting erratic results.. It now passes sometimes and fails other times.. without me changing anything at all! I think I'm going to go ahead and push it as is and see how Jenkins reacts over the next cycle.. if it's consistently passing on Jenkins I won't worry about it.. if not we'll need to revisit the test.
Created attachment 89002 [details] [review] Bug 22478: Prevent XSS vulnerabilities when pagination appears This is a bad one as we thought we were XSS safe since bug 13618. The html code generated in C4::Output::pagination_bar must escape the variables and values correctly. This patch needs to be widely tested, everywhere the pagination appears, to make sure we will not introduce regressions. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 89003 [details] [review] Bug 22478: Add tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 89004 [details] [review] Bug 22478: Fix POD Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 89005 [details] [review] Bug 22478: (QA follow-up) Handle category in opac-shelves like a boolean The category parameter should be restricted to 1 or 2 (private/public). In order to keep same behavior, no parameter means 1 (private). Note: Adding the same line in intranet counterpart. Test plan [1] Check for category empty, '1a', '11' etc. And with script ;) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 89006 [details] [review] Bug 22478: (QA follow-up) Make test consistent with variable name We were adding data to category 1 (private lists) and testing against that, but we named the variable 'publist_lists'. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I am testing on master: If I drop Marcel's patch then I get the expected results (though Jonathan's test has 1 where it means 2) so things are getting escaped correctly with just that patch If I add back Marcel's patch the escape is sort of moot, no other params get passed through and both page and category are cleaned to be simply numbers so.. Do we accept/need Marcel's patch? I think yes Do we need to update Jonathan's test after Marcel's patch to look for just a clean numebr in the url? I think yes too Jonathan's patch is still presumably needed for a future case somewhere if other variables get passed through, but Marcel's makes the response here even safer. Does everyone here agree with that?
Created attachment 89009 [details] [review] Bug 22478: (QA follow-up) Update tests to check for any script tags
(In reply to Nick Clemens from comment #42) > Does everyone here agree with that? No, we do not need Marcel's patch. Moreover the pagination is used at several other places. The original report is about opac-shelves, but other scripts are impacted. Do you need ceinture and bretelles??
(In reply to Martin Renvoize from comment #43) > Created attachment 89009 [details] [review] [review] > Bug 22478: (QA follow-up) Update tests to check for any script tags I do not think these tests are correct. We want to test that the variable are correctly escaped, not that a specific script (opac-shelves) will replace an invalid value with a correct one (everything != 1 or 2 will be 2)
Pushed to 18.05.x for 18.05.12
Pushed to 17.11.x for 17.11.18
(In reply to Fridolin SOMERS from comment #47) > Pushed to 17.11.x for 17.11.18 But without Selenium test. I've tested manually shelves lists at opac and staff
Awesome work all! Pushed to master for 19.05
(In reply to Jonathan Druart from comment #45) > (In reply to Martin Renvoize from comment #43) > > Created attachment 89009 [details] [review] [review] [review] > > Bug 22478: (QA follow-up) Update tests to check for any script tags > > I do not think these tests are correct. We want to test that the variable > are correctly escaped, not that a specific script (opac-shelves) will > replace an invalid value with a correct one (everything != 1 or 2 will be 2) Please file a follow-up if you think we need to fix this Jonathan, patches were pushed to stable and so master followed.
Done, see bug 22836.
(In reply to Jonathan Druart from comment #44) > (In reply to Nick Clemens from comment #42) > > Does everyone here agree with that? > > No, we do not need Marcel's patch. Moreover the pagination is used at > several other places. The original report is about opac-shelves, but other > scripts are impacted. > Do you need ceinture and bretelles?? I would just like to add the question: Do you need left or right hand?
(In reply to Marcel de Rooy from comment #52) > (In reply to Jonathan Druart from comment #44) > > (In reply to Nick Clemens from comment #42) > > > Does everyone here agree with that? > > > > No, we do not need Marcel's patch. Moreover the pagination is used at > > several other places. The original report is about opac-shelves, but other > > scripts are impacted. > > Do you need ceinture and bretelles?? > > I would just like to add the question: Do you need left or right hand? Yes, typing is faster.
Created attachment 89453 [details] [review] Bug 22478: (follow-up) [18.05.X ONLY] Fix rebase and fix selenium opac_auth We missed a case or two in the backport, also picked changes from https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76414&action=diff in order to fix opac auth in selenium
*** Bug 23613 has been marked as a duplicate of this bug. ***