Bug 13425

tested and works.  Sorry I kept hitting the wrong button - hence why I've got the same patch signed off 10 times.  :P

Pushed to 3.18.x still needs to go to master and 3.16.x

*** Bug 13426 has been marked as a duplicate of this bug. ***

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>

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>

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.

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.

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.

Especially when the code to exploit it has been published :(

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?

(In reply to Jonathan Druart from comment #18)
> Created attachment 34267 [details] [review] [review]

moved to bug 13428.

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.

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.

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

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 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 _ "&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.

I completely forgot to test the prog theme...

(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

(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 :)

Just the last patch for the intranet needs to be tested

The last patch is pushed to 3.18 and should be pushed to master.
Marked as Passed QA.

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.

(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.

Reworked the last patch, it was missing the fix for the availability facet, it needs new testing please

Pushed to 3.18.x, Tomas just the last patch needs to go to master, at your convenience  :)

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>

(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

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.

We got this assigned a CVE number CVE-2014-9446


http://www.cvedetails.com/cve/CVE-2014-9446/

Not needed for 3.14.x, I set has resolved.