Bug 3652

Summary: XSS vulnerabilities
Product: Koha Reporter: Chris Cormack <chris>
Component: TemplatesAssignee: Chris Hall <chrish>
Status: CLOSED FIXED QA Contact: Mason James <mtj>
Severity: critical    
Priority: P1 - high CC: cnighswonger, jcamins, jwagner, katrin.fischer, magnus, mjr, mtj, nengard, paul.poulain
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 2600, 2690, 2847    
Attachments: Patch for opac-search-history.tmpl
OPAC fixes
Revised patch for the OPAC
Proposed fix for the staff client
Revised fix
Bug 3652 : [SECURITY] XSS vulnerability
Bug 3652 : [SECURITY] XSS vulnerability
Bug 3652 : [SIGNED-OFF] [SECURITY] XSS vulnerability
Bug 3652: XSS vulnerability in page numbering
Bug 3652 XSS vulnerability in facets
[SIGNED-OFF] Bug 3652: XSS vulnerability in page numbering
[SIGNED-OFF] Bug 3652 XSS vulnerability in facets
Bug 3652: XSS fixes - follow up
Bug 3652: XSS fixes - follow up
Bug 3652: [SIGNED-OFF] XSS fixes - follow up
Bug 3652: close XSS vulnerabilities on biblionumber and authid
Bug 3652: close XSS vulnerabilities in opac-export
Bug 3652: close XSS vulnerabilities on biblionumber and authid
Bug 3652: close XSS vulnerabilities in opac-export
Fixing XSS vulnerability in opac-search
bug 3652 fixing XSS vulnerabilities in opac-search

Description Chris Cormack 2010-05-21 01:13:19 UTC


---- Reported by mjr@ttllp.co.uk 2009-09-21 13:11:35 ----

Koha has several XSS attack opportunities.  There are two approaches to fixing this:

1. identify and ESCAPE="HTML" (or URL or JS) all places in the templates which are vulnerable;

2. set default_escape => "HTML" in C4::Output and identify the places in the templates which require ESCAPE="0" (unescaped) output.

1 (default permit) seems less secure than 2.  2 will break some things in the short term.

This seems like a generalisation of http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=2690.



---- Additional Comments From mjr@ttllp.co.uk 2009-11-04 22:53:28 ----

*** http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3759 has been marked as a duplicate of this bug. ***



---- Additional Comments From gcollum@gmail.com 2010-02-16 00:08:34 ----

Created an attachment
Patch for opac-search-history.tmpl





--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:13 UTC  ---

This bug was previously known as _bug_ 3652 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3652
Imported an attachment (id=1482)

Actual time not defined. Setting to 0.0
The original reporter of this bug does not have
   an account here. Reassigning to the person who moved
   it here: chris@bigballofwax.co.nz.
   Previous reporter was mjr@ttllp.co.uk.
The original assignee of this bug does not have
   an account here. Reassigning to the default assignee
   for the component, gmcharlt@gmail.com.
   Previous assignee was mjr@ttllp.co.uk.
CC member sirusdv@pwnzord.com does not have an account here
The original submitter of attachment 1482 [details] [review] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.

Comment 1 MJ Ray (software.coop) 2010-07-07 19:27:05 UTC
Restoring where "Previous assignee was mjr@ttllp.co.uk"
Comment 2 Owen Leonard 2010-07-14 14:27:24 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2010-11-24 19:19:55 UTC
This is in new/bug3652

I have merged master in and pushed it up, please test this branch and if it works ok, let me know and I can merge it into master
Comment 4 Chris Cormack 2010-11-24 19:22:27 UTC
Chris N

http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=f1d4d4d15c5038f2c668a232b3a169c20df31966

Is the commit you will want to cherry-pick if this tests ok
Comment 5 Owen Leonard 2010-11-24 20:00:27 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2010-11-24 20:01:11 UTC Comment hidden (obsolete)
Comment 7 MJ Ray (software.coop) 2010-12-13 12:16:20 UTC
This bug is mentioned in:
Further fixes for Bug 3652 - XSS	vulnerabilities http://lists.koha-community.org/pipermail/koha-patches/2010-November/013097.html
Comment 8 Owen Leonard 2010-12-13 14:22:49 UTC Comment hidden (obsolete)
Comment 9 MJ Ray (software.coop) 2011-01-06 10:18:19 UTC
This bug is mentioned in:
Further fixes for Bug 3652 -	XSS	vulnerabilities http://lists.koha-community.org/pipermail/koha-patches/2010-December/013162.html
Comment 10 Paul Poulain 2011-04-28 20:37:26 UTC
patch still not made his way into Koha, XSS vulnerabilities still here, but the patch won't apply anymore as we've now using T::T and this patch is based on H::T::P
Comment 11 Chris Cormack 2011-07-07 21:22:12 UTC
This will need a total rewrite for master Template::Toolkit
Comment 12 Chris Cormack 2011-08-01 03:34:33 UTC
I think we could do it with this

https://metacpan.org/module/Template::AutoFilter
Comment 13 Paul Poulain 2011-08-01 08:00:05 UTC
mmm... reading comments I see:

WARNING: This module is highly experimental. I have not done a lot of testing and things might blow up in unexpected ways. The API and behavior might change with any release (until 1.0). If you'd like to see any changes implemented, let me know via RT, email, IRC or by opening a pull request on github.

Use at your own risk.
Comment 14 Katrin Fischer 2012-03-19 06:06:22 UTC
What is the current status on this? Do we still have XSS vulnerabilities?
Comment 15 Chris Cormack 2012-10-08 08:30:44 UTC
XSS vulnerability when viewing shelves.

To test
1/ Visit /cgi-bin/koha/opac-shelves.pl?viewshelf=<blink>fish</blink>&sortfield=1

After patch this should no longer be blinking
Comment 16 Chris Cormack 2012-10-08 08:31:41 UTC Comment hidden (obsolete)
Comment 17 Chris Cormack 2012-10-08 08:39:09 UTC
Sorry forgot to mention this only shows up when you dont have permission to view the list
Comment 18 Magnus Enger 2012-10-08 08:52:12 UTC Comment hidden (obsolete)
Comment 19 Magnus Enger 2012-10-08 08:52:53 UTC Comment hidden (obsolete)
Comment 20 Chris Cormack 2012-10-09 01:13:23 UTC
If you have a search that returns more than one page of results it is possible to craft an xss exploit. 
With page numbers turned on try
/cgi-bin/koha/opac-search.pl?q=1&do=Search&limit-yr=1&limit=1&idx=kw&sort_by=relevance"></a><b>This%20shouldn't%20happen</b>

Then try it again with the patch applied.

Another patch to follow to fix facets
Comment 21 Chris Cormack 2012-10-09 01:13:39 UTC Comment hidden (obsolete)
Comment 22 Chris Cormack 2012-10-09 01:20:06 UTC Comment hidden (obsolete)
Comment 23 Katrin Fischer 2012-10-09 06:43:45 UTC Comment hidden (obsolete)
Comment 24 Katrin Fischer 2012-10-09 06:43:54 UTC Comment hidden (obsolete)
Comment 25 Mason James 2012-10-11 07:34:48 UTC
(In reply to comment #24)
> Created attachment 12741 [details] [review]
> [SIGNED-OFF] Bug 3652 XSS vulnerability in facets
> 
> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
> Confirmed bug with test plan on the bug and checked that
> problem is fixed after applying the patch and that facets
> still work.

3 patches look good, passing QA - well spotted!

$ koha-qa.pl -c 3 

testing 3 commit(s) (applied to commit 0c75d66)
 * 55c3117 Bug 3652 XSS vulnerability in facets
      koha-tmpl/opac-tmpl/prog/en/includes/opac-facets.inc

 * 210a889 Bug 3652: XSS vulnerability in page numbering
      koha-tmpl/opac-tmpl/prog/en/includes/page-numbers.inc

 * f2db538 Bug 3652 : [SIGNED-OFF] [SECURITY] XSS vulnerability
      koha-tmpl/opac-tmpl/prog/en/modules/opac-shelves.tt

* koha-tmpl/opac-tmpl/prog/en/includes/opac-facets.inc                     OK
* koha-tmpl/opac-tmpl/prog/en/includes/page-numbers.inc                    OK
* koha-tmpl/opac-tmpl/prog/en/modules/opac-shelves.tt                      OK
Comment 26 Paul Poulain 2012-10-12 10:18:13 UTC
Patch pushed to master
Comment 27 Chris Cormack 2012-10-12 17:57:24 UTC
Pushed to 3.8.x, will be in 3.8.6, probably needed for 3.6.x too
Comment 28 Katrin Fischer 2012-10-15 07:11:41 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2012-10-15 07:12:53 UTC Comment hidden (obsolete)
Comment 30 Magnus Enger 2012-10-15 08:15:30 UTC Comment hidden (obsolete)
Comment 31 Jared Camins-Esakov 2012-10-15 15:54:05 UTC Comment hidden (obsolete)
Comment 32 Jared Camins-Esakov 2012-10-15 16:04:58 UTC Comment hidden (obsolete)
Comment 33 Chris Cormack 2012-10-15 19:52:33 UTC Comment hidden (obsolete)
Comment 34 Chris Cormack 2012-10-15 19:54:10 UTC Comment hidden (obsolete)
Comment 35 Jared Camins-Esakov 2012-10-16 17:16:43 UTC
*** Bug 8930 has been marked as a duplicate of this bug. ***
Comment 36 Chris Hall 2012-10-17 07:02:52 UTC Comment hidden (obsolete)
Comment 37 Paul Poulain 2012-10-17 15:38:51 UTC
Comment on attachment 12809 [details] [review]
Bug 3652: [SIGNED-OFF] XSS fixes - follow up

QA comment, trivial patch, passes QA & pushed
Comment 38 Paul Poulain 2012-10-17 15:41:42 UTC
Comment on attachment 12835 [details] [review]
Bug 3652: close XSS vulnerabilities on biblionumber and authid

About this patch, Jared, why do you add
 || $query->param('bib');
to opac-ISBD|MARCdetail.pl ?
I see it's in opac-detail, but it's an oldies and not goodies (in early versions of Koha, biblionumber was sometimes written bib, bn, ... It has been fixed, and I favour removing
 ||  $query->param('bib');
from opac-detail.pl, because we must not have param('bib')
Comment 39 Jared Camins-Esakov 2012-10-17 15:50:15 UTC
(In reply to comment #38)
> Comment on attachment 12835 [details] [review]
> Bug 3652: close XSS vulnerabilities on biblionumber and authid
> 
> About this patch, Jared, why do you add
>  || $query->param('bib');
> to opac-ISBD|MARCdetail.pl ?
> I see it's in opac-detail, but it's an oldies and not goodies (in early
> versions of Koha, biblionumber was sometimes written bib, bn, ... It has
> been fixed, and I favour removing
>  ||  $query->param('bib');
> from opac-detail.pl, because we must not have param('bib')

I wanted to make sure the behavior was identical, and I figured there must surely be a good reason for the $query->param('bib'). If you wanted to remove the || $query->param('bib') from all three files, I would not object at all.
Comment 40 Mason James 2012-10-19 03:51:09 UTC
passing QA on 2 patches...

$ koha-qa.pl -c 2

testing 2 commit(s) (applied to commit d91a4f8)
 * 09305f2 bug 3652 fixing XSS vulnerabilities in opac-search
      koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt
      koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt
      koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt

 * 005a1ea Bug 3652: close XSS vulnerabilities in opac-export
      opac/opac-export.pl

* opac/opac-export.pl                                                      OK
* koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt                       OK
* koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt              OK
* koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt                      OK
Comment 41 Mason James 2012-10-19 04:00:44 UTC Comment hidden (obsolete)
Comment 42 Mason James 2012-10-19 04:02:24 UTC
(In reply to comment #41)
> Created attachment 12926 [details] [review]
> bug 3652 fixing XSS vulnerabilities in opac-search
> 
> Signed-off-by: Mason James <mtj@kohaaloha.com>

tested Chris' patch, works well
Comment 43 Mason James 2012-10-19 04:06:39 UTC
(In reply to comment #40)
> passing QA on 2 patches...

oops, QA'ed the wrong patch

$ koha-qa.pl -c 2

testing 2 commit(s) (applied to commit 32fab74)
 * 66a972f Bug 3652: close XSS vulnerabilities in opac-export
      opac/opac-export.pl

 * 2387688 Bug 3652: close XSS vulnerabilities on biblionumber and authid
      opac/opac-ISBDdetail.pl
      opac/opac-MARCdetail.pl
      opac/opac-authoritiesdetail.pl
      opac/opac-detail.pl
      opac/opac-showmarc.pl

* opac/opac-export.pl                                                      OK
* opac/opac-ISBDdetail.pl                                                  OK
* opac/opac-MARCdetail.pl                                                  OK
* opac/opac-authoritiesdetail.pl                                           OK
* opac/opac-detail.pl                                                      OK
* opac/opac-showmarc.pl                                                    OK
Comment 44 Mason James 2012-10-19 04:37:59 UTC
i actually tested these against master,

ill flick the version to reflect that (hope thats ok)
Comment 45 Chris Cormack 2012-10-22 04:04:46 UTC
Pushed to 3.8.x will be in 3.8.6
Comment 46 Paul Poulain 2012-10-24 13:48:16 UTC
The 3 patches
 Bug 3652: close XSS vulnerabilities on biblionumber and authid (3.40 KB, patch) 
 Bug 3652: close XSS vulnerabilities in opac-export (2.62 KB, patch) 
 bug 3652 fixing XSS vulnerabilities in opac-search (3.04 KB, patch) 

have been pushed

QA comment for  Bug 3652: close XSS vulnerabilities on biblionumber and authid (3.40 KB, patch) = I made a follow-up to remove the || $query->param('bib'); (see comment 38)
I think opac-detail.pl could also be fixed, but in case there's an old reference to this, I won't do that without a specific patch.

Comment for opac-search = the XSS did not work for me if I entered 
> Search in the opac for ';</script><script>alert(10);</alert>' 
If was exploitable only with
> /cgi-bin/koha/opac-search.
> pl?q=%3B%3C%2Fscript%3E%3Cscript%3Ealert%2810%29%3B%3C%2Fscript%3E

but it's worth pushing it anyway

status back to ASSIGNED if another XSS vulnerability is found & fixed
Comment 47 Chris Cormack 2013-03-24 05:26:51 UTC
No outstanding vulnerabilities at this time, a new bug can be opened if some are found