Bug 13425

Summary: There is an xss vulnerability in 3.16, 3.18 and master in the opac and intranet facets
Product: Koha Reporter: Chris Cormack <chris>
Component: SearchingAssignee: Galen Charlton <gmcharlt>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: arunet, bob-ewart, brendan, fridolin.somers, jonathan.druart, katrin.fischer, mtj, tomascohen
Version: 3.18   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13618
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13789
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 13428    
Attachments: Bug 13425 - XSS in opac facets - Patch for master and 3.18
Bug 13425 - XSS in opac facets - Patch for 3.16
Bug 13425 - XSS in opac facets - Patch for 3.16
Bug 13425 - XSS in opac facets - Patch for 3.16
Bug 13425 - XSS in opac facets - Patch for 3.16
Bug 13425 - XSS in opac facets - Patch for master and 3.18
Bug 13425 - XSS in opac facets - Patch for 3.16
Bug 13425 - XSS in opac facets - Patch for master and 3.18
[PUSHED] Bug 13425 - XSS in intranet facets - Patch for 3.16
[PUSHED] Bug 13425 - XSS in opac facets - Patch for 3.16
Bug 13425: Fix regression on deleting facets
[PUSHED] Bug 13425 - XSS in opac facets - Patch for master and 3.18
Bug 13425 - XSS in intranet facets - Patch for 3.18 and master
Bug 13425 - XSS in intranet facets - Patch for 3.18 and master
Bug 13425 - XSS in intranet facets - Patch for 3.18 and master
Bug 13425 - XSS in intranet facets - Patch for 3.18 and master
Bug 13425 - XSS in intranet facets - Patch for 3.18 and master
Bug 13425 - XSS in intranet facets - Patch for 3.18 and master
Bug 13425 - XSS in intranet facets - Patch for 3.18 and master
Bug 13425 - XSS in intranet facets - Patch for 3.18 and master

Description Chris Cormack 2014-12-09 23:47:14 UTC
A url like 
/cgi-bin/koha/opac-search.pl?q=123&sort_by='"><script>prompt('Happy_Holidays')</script>&limit=123 


Will cause the js to be executed. THere is a missing escaped url.
Comment 1 Chris Cormack 2014-12-09 23:49:01 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2014-12-09 23:51:11 UTC Comment hidden (obsolete)
Comment 6 Brendan Gallagher 2014-12-10 00:05:30 UTC Comment hidden (obsolete)
Comment 7 Brendan Gallagher 2014-12-10 00:05:35 UTC Comment hidden (obsolete)
Comment 8 Brendan Gallagher 2014-12-10 00:06:48 UTC
tested and works.  Sorry I kept hitting the wrong button - hence why I've got the same patch signed off 10 times.  :P
Comment 9 Mirko Tietgen 2014-12-10 00:07:47 UTC Comment hidden (obsolete)
Comment 10 Chris Cormack 2014-12-10 01:30:29 UTC
Pushed to 3.18.x still needs to go to master and 3.16.x
Comment 11 Chris Cormack 2014-12-10 01:31:01 UTC
*** Bug 13426 has been marked as a duplicate of this bug. ***
Comment 12 Jared Camins-Esakov 2014-12-10 04:42:49 UTC
Created attachment 34264 [details] [review]
[PUSHED] Bug 13425 - XSS in intranet facets - Patch for 3.16

To Test
1/ Craft a url like /cgi-bin/koha/catalogue/search.pl?q=smith&sort_by='"><script>prompt('Happy_Holidays')</script>

It is important it must return results and facets

2/ Notice the js is executed
3/ Apply the patch test again

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 13 Jared Camins-Esakov 2014-12-10 04:43:01 UTC
Created attachment 34265 [details] [review]
[PUSHED] Bug 13425 - XSS in opac facets - Patch for 3.16

To Test
1/ Craft a url like /cgi-bin/koha/opac-search.pl?q=123&sort_by='"><script>prompt('Happy_Holidays')</script>&limit=123

It is important it must return results and facets

2/ Notice the js is executed
3/ Apply the patch test again

Test this one both in prog and bootstrap please

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 14 Jared Camins-Esakov 2014-12-10 04:43:37 UTC
Brendan, I removed your sign-off line because it looks like you may have uploaded a different signed off patch than the one you were testing. Please feel free to readd it.
Comment 15 Jonathan Druart 2014-12-10 08:22:43 UTC
The patch fixes the issues, but introduce a regression:
Search for "123", click on a facet with a diacritic (eg. "Bazin, Hervé").
Remove this facet using the [x] link.
You cannot remove the facet.
This worked before the patch.
Comment 16 Chris Cormack 2014-12-10 08:29:10 UTC
All the patch does is escape the html in the url, if this breaks the functionality of the remove facet, then that code needs to be reworked, we simply can't have a feature that relies on us displaying unfiltered user input.
Comment 17 Chris Cormack 2014-12-10 08:31:29 UTC
Especially when the code to exploit it has been published :(
Comment 18 Jonathan Druart 2014-12-10 08:39:32 UTC Comment hidden (obsolete)
Comment 19 Chris Cormack 2014-12-10 08:42:28 UTC
I think that new patch should go on its own bug, outside of the security group so it can get signed off and qaed as normal.

And can go out in the 3.18.2 release (and 3.16.6), and tomas can push to master when he is back.

Do you want me to shift it?
Comment 20 Jonathan Druart 2014-12-10 09:00:44 UTC
(In reply to Jonathan Druart from comment #18)
> Created attachment 34267 [details] [review] [review]

moved to bug 13428.
Comment 22 Jonathan Druart 2014-12-10 09:17:45 UTC
Created attachment 34273 [details] [review]
[PUSHED] Bug 13425 - XSS in opac facets - Patch for master and 3.18

To Test
1/ Craft a url like /cgi-bin/koha/opac-search.pl?q=123&sort_by='"><script>prompt('Happy_Holidays')</script>&limit=123

It is important it must return results and facets

2/ Notice the js is executed
3/ Apply the patch test again

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Popup is gone after applying the patch. Facet link still shows it but does not execute. It's gone after clicking the link.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
This patch needs bug 13428.
Comment 23 Chris Cormack 2014-12-10 09:20:18 UTC
Hmm there are 2 patches for 3.16.x one for intranet one for opac, mason must have missed the opac one. I dont think he has rolled the release or done an announcement yet, so he should be able to fix that.
Comment 24 Katrin Fischer 2014-12-11 10:14:07 UTC
I can confirm that OPAC in 3.16 is NOT fixed. I just tested it. The confusing bit is that the commit message claims to fix the OPAC, but indeed it fixes the intranet :(

http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=30ca6c1d0eaf15ffc4339cb20595241182b9cdae
Comment 25 Katrin Fischer 2014-12-11 10:18:52 UTC
On another note: I'd like to receive emails about changes on the security bugs - is there something I can do? I thought there was a security mailing list too, but can't remember receiving an email.
Comment 26 Katrin Fischer 2014-12-11 11:21:30 UTC
Comment on attachment 34265 [details] [review]
[PUSHED] Bug 13425 - XSS in opac facets - Patch for 3.16

Review of attachment 34265 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-facets.inc
@@ +42,4 @@
>                                    [% IF facet.active %]
>                                      [% SET url = url _ "&amp;nolimit=" _ facet.type_link_value _ ":" _ facet.facet_link_value %]
>                                      <span class="facet-label">[% facet.facet_label_value %]</span>
> +                                    [<a href="[% url |url%]" title="Remove facet [% facet.facet_link_value | html %]">x</a>]

This change seems to only appear in this patch - not in the one for bootstrap or intranet - is it intentional?
It seems to break the functionality, but we are still running tests.
Comment 27 Jonathan Druart 2014-12-11 11:32:10 UTC
I completely forgot to test the prog theme...
Comment 28 Mason James 2014-12-11 12:54:42 UTC
(In reply to Chris Cormack from comment #23)
> Hmm there are 2 patches for 3.16.x one for intranet one for opac, mason must
> have missed the opac one. I dont think he has rolled the release or done an
> announcement yet, so he should be able to fix that.

the 2nd missing patch has now been pushed to 3.16 branch
Comment 29 Mason James 2014-12-11 23:27:33 UTC
(In reply to Jonathan Druart from comment #27)
> I completely forgot to test the prog theme...


I just tested the PROG theme too

the bug is fixed :)
Comment 30 Chris Cormack 2014-12-14 23:09:32 UTC Comment hidden (obsolete)
Comment 31 Chris Cormack 2014-12-14 23:10:03 UTC
Just the last patch for the intranet needs to be tested
Comment 32 Chris Cormack 2014-12-17 20:08:37 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2014-12-18 08:18:30 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2014-12-18 08:19:56 UTC
The last patch is pushed to 3.18 and should be pushed to master.
Marked as Passed QA.
Comment 35 Chris Cormack 2014-12-18 08:23:49 UTC
It isn't actually pushed to 3.18 yet, I was wanting a sign off on it. But if you have tested it, ill push it to 3.18 as soon as it's in master.
Comment 36 Jonathan Druart 2014-12-18 09:34:56 UTC
(In reply to Chris Cormack from comment #35)
> It isn't actually pushed to 3.18 yet, I was wanting a sign off on it. But if
> you have tested it, ill push it to 3.18 as soon as it's in master.

Arg yes, sorry to add confusion.

So the last patch needs to be pushed to 3.18.x and master.
Comment 37 Chris Cormack 2014-12-19 23:13:14 UTC Comment hidden (obsolete)
Comment 38 Chris Cormack 2014-12-19 23:13:58 UTC
Reworked the last patch, it was missing the fix for the availability facet, it needs new testing please
Comment 39 Katrin Fischer 2014-12-21 20:43:57 UTC Comment hidden (obsolete)
Comment 40 Chris Cormack 2014-12-21 20:50:40 UTC Comment hidden (obsolete)
Comment 41 Katrin Fischer 2014-12-21 20:56:24 UTC Comment hidden (obsolete)
Comment 42 Chris Cormack 2014-12-21 21:00:52 UTC
Pushed to 3.18.x, Tomas just the last patch needs to go to master, at your convenience  :)
Comment 43 Mason James 2014-12-22 17:02:11 UTC
Created attachment 34636 [details] [review]
Bug 13425 - XSS in intranet facets - Patch for 3.18 and master

To Test
1/ Craft a url like /cgi-bin/koha/catalogue/search.pl?q=smith&sort_by='"><script>prompt('Happy_Holidays')</script>

It is important it must return results and facets

2/ Notice the js is executed
3/ Apply the patch test again

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
No prompts, no functional regressions found.
Checked selecting and undoing facets, show more links and paging.
Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 44 Mason James 2014-12-22 17:05:30 UTC
(In reply to Chris Cormack from comment #42)
> Pushed to 3.18.x, Tomas just the last patch needs to go to master, at your
> convenience  :)

This last patch looks to fix a CGI quoting bug in 3.16.x

I plan to include this patch in 3.16.6
Comment 45 Chris Cormack 2014-12-26 19:41:11 UTC
Tomas the last intranet patch needs to be pushed to master, and then we can move this out of security into the open, and I will request some cve numbers.
Comment 46 Chris Cormack 2015-01-20 20:33:14 UTC
We got this assigned a CVE number CVE-2014-9446


http://www.cvedetails.com/cve/CVE-2014-9446/
Comment 47 Fridolin Somers 2015-02-23 07:51:29 UTC
Not needed for 3.14.x, I set has resolved.