Bug 22478 - Cross-site scripting vulnerability in paginations
Summary: Cross-site scripting vulnerability in paginations
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
: 23613 (view as bug list)
Depends on:
Blocks: 14568 22836
  Show dependency treegraph
 
Reported: 2019-03-07 20:40 UTC by cking
Modified: 2019-10-14 09:50 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.05, 18.05.12, 17.11.18


Attachments
Bug 22478: Prevent XSS vulnerabilities when pagination appears (1.70 KB, patch)
2019-03-14 22:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22478: Add tests (5.42 KB, patch)
2019-03-15 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22478: Fix POD (809 bytes, patch)
2019-04-10 07:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22478: Prevent XSS vulnerabilities when pagination appears (1.76 KB, patch)
2019-04-10 07:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22478: Add tests (5.48 KB, patch)
2019-04-10 07:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22478: Fix POD (866 bytes, patch)
2019-04-10 07:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22478: Fix POD (866 bytes, patch)
2019-04-10 07:14 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22478: Prevent XSS vulnerabilities when pagination appears (1.86 KB, patch)
2019-04-12 07:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22478: Add tests (5.58 KB, patch)
2019-04-12 07:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22478: Fix POD (953 bytes, patch)
2019-04-12 07:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22478: (QA follow-up) Handle category in opac-shelves like a boolean (1.68 KB, patch)
2019-04-12 07:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22478: (RMaint follow-up) source t::lib::Mocks (934 bytes, patch)
2019-04-26 16:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22478: (RMaint follow-up) Fix selenium test (2.10 KB, patch)
2019-04-26 18:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22478: Prevent XSS vulnerabilities when pagination appears (1.82 KB, patch)
2019-04-29 09:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22478: Add tests (5.52 KB, patch)
2019-04-29 09:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22478: Fix POD (913 bytes, patch)
2019-04-29 09:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22478: (QA follow-up) Handle category in opac-shelves like a boolean (1.65 KB, patch)
2019-04-29 09:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22478: (QA follow-up) Make test consistent with variable name (2.04 KB, patch)
2019-04-29 09:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22478: (QA follow-up) Update tests to check for any script tags (1.78 KB, patch)
2019-04-29 12:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22478: (follow-up) [18.05.X ONLY] Fix rebase and fix selenium opac_auth (2.33 KB, patch)
2019-05-07 18:03 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description cking 2019-03-07 20:40:17 UTC
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.
Comment 1 Jonathan Druart 2019-03-08 22:07:33 UTC
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&quot;&gt;&lt;script&gt;alert(150)&lt;/script&gt;;
Comment 2 systems_analysts 2019-03-08 23:06:52 UTC
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.
Comment 3 Jonathan Druart 2019-03-08 23:41:27 UTC
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?
Comment 4 systems_analysts 2019-03-08 23:50:39 UTC
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&quot;&gt;&lt;script&gt;alert(150)&lt;/script&gt;;
    }
    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();
    }
}
Comment 5 systems_analysts 2019-03-08 23:51:43 UTC
needless to say, Chrome was blocking the XSS, but Firefox showed the Alert.
Comment 6 Jonathan Druart 2019-03-09 12:39:28 UTC
Do you see other occurrences of "script"?
Comment 7 systems_analysts 2019-03-14 21:19:10 UTC
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">
&nbsp;<span class="inactive">&lt;&lt;</span>
&nbsp;<span class="inactive">&lt;</span>
&nbsp;<span class="currentPage">1</span>
&nbsp;<a href="?page=2&amp;op=list&amp;category=2"><script>alert(150)</script>"&gt;2</a>
&nbsp;<a href="?page=3&amp;op=list&amp;category=2"><script>alert(150)</script>"&gt;3</a>
&nbsp;<a href="?page=2&amp;op=list&amp;category=2"><script>alert(150)</script>" rel="next"&gt;&gt;</a>
&nbsp;<a href="?page=3&amp;op=list&amp;category=2"><script>alert(150)</script>" rel="last"&gt;&gt;&gt;</a></div>
Comment 8 Jonathan Druart 2019-03-14 22:33:13 UTC
Confirmed, with pagination.
Comment 9 Jonathan Druart 2019-03-14 22:47:22 UTC
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.
Comment 10 Jonathan Druart 2019-03-15 16:42:48 UTC
Created attachment 86683 [details] [review]
Bug 22478: Add tests
Comment 11 Katrin Fischer 2019-04-10 07:04:06 UTC
Created attachment 87697 [details] [review]
Bug 22478: Fix POD
Comment 12 Katrin Fischer 2019-04-10 07:13:42 UTC
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>
Comment 13 Katrin Fischer 2019-04-10 07:13:46 UTC
Created attachment 87699 [details] [review]
Bug 22478: Add tests

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2019-04-10 07:13:50 UTC
Created attachment 87700 [details] [review]
Bug 22478: Fix POD

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2019-04-10 07:14:20 UTC
Created attachment 87701 [details] [review]
Bug 22478: Fix POD

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Katrin Fischer 2019-04-10 07:14:51 UTC
I couldn't run the Selenium tests, but I have tested the pagination in various places.
Comment 17 Marcel de Rooy 2019-04-12 06:12:57 UTC
QA: Looking here now
Comment 18 Marcel de Rooy 2019-04-12 06:27:54 UTC
(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.
Comment 19 Marcel de Rooy 2019-04-12 06:58:26 UTC
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..
Comment 20 Marcel de Rooy 2019-04-12 07:07:24 UTC
(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>
Comment 21 Marcel de Rooy 2019-04-12 07:59:06 UTC
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>
Comment 22 Marcel de Rooy 2019-04-12 07:59:11 UTC
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>
Comment 23 Marcel de Rooy 2019-04-12 07:59:15 UTC
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>
Comment 24 Marcel de Rooy 2019-04-12 07:59:20 UTC
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>
Comment 25 Jonathan Druart 2019-04-24 22:58:51 UTC
(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?
Comment 26 Martin Renvoize 2019-04-26 16:08:56 UTC
Pushed to 18.11.x for 18.11.05
Comment 27 Martin Renvoize 2019-04-26 16:37:07 UTC
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>
Comment 28 Martin Renvoize 2019-04-26 18:58:16 UTC
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>
Comment 29 Martin Renvoize 2019-04-26 19:21:06 UTC
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.
Comment 30 Jonathan Druart 2019-04-26 21:54:05 UTC
(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  :)
Comment 31 Martin Renvoize 2019-04-27 06:03:59 UTC
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.
Comment 32 Martin Renvoize 2019-04-27 13:47:55 UTC
# 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
Comment 33 Jonathan Druart 2019-04-27 18:31:02 UTC
Martin, could you confirm you did not apply the QA follow-up?
Comment 34 Martin Renvoize 2019-04-27 18:45:45 UTC
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.
Comment 35 Jonathan Druart 2019-04-27 19:05:59 UTC
What do you see on the screenshot? The test is failing on master as well?
Comment 36 Martin Renvoize 2019-04-29 09:03:34 UTC
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.
Comment 37 Martin Renvoize 2019-04-29 09:33:42 UTC
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>
Comment 38 Martin Renvoize 2019-04-29 09:33:45 UTC
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>
Comment 39 Martin Renvoize 2019-04-29 09:33:48 UTC
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>
Comment 40 Martin Renvoize 2019-04-29 09:33:52 UTC
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>
Comment 41 Martin Renvoize 2019-04-29 09:33:55 UTC
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>
Comment 42 Nick Clemens 2019-04-29 11:18:01 UTC
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?
Comment 43 Martin Renvoize 2019-04-29 12:33:10 UTC
Created attachment 89009 [details] [review]
Bug 22478: (QA follow-up) Update tests to check for any script tags
Comment 44 Jonathan Druart 2019-04-29 13:49:05 UTC
(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??
Comment 45 Jonathan Druart 2019-04-29 13:51:07 UTC
(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)
Comment 46 Fridolin SOMERS 2019-04-30 06:16:27 UTC
Pushed to 18.05.x for 18.05.12
Comment 47 Fridolin SOMERS 2019-04-30 07:31:17 UTC
Pushed to 17.11.x for 17.11.18
Comment 48 Fridolin SOMERS 2019-04-30 07:31:57 UTC
(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
Comment 49 Nick Clemens 2019-05-02 13:50:50 UTC
Awesome work all!

Pushed to master for 19.05
Comment 50 Nick Clemens 2019-05-02 13:51:59 UTC
(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.
Comment 51 Jonathan Druart 2019-05-03 01:04:33 UTC
Done, see bug 22836.
Comment 52 Marcel de Rooy 2019-05-03 05:40:27 UTC
(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?
Comment 53 Jonathan Druart 2019-05-03 13:01:58 UTC
(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.
Comment 54 Nick Clemens 2019-05-07 18:03:54 UTC
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
Comment 55 Jonathan Druart 2019-10-14 09:50:42 UTC
*** Bug 23613 has been marked as a duplicate of this bug. ***