Bug 3652 - XSS vulnerabilities
Summary: XSS vulnerabilities
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P1 - high critical (vote)
Target Milestone: Oldversions
Assignee: Chris Hall
QA Contact: Mason James
URL:
Whiteboard:
Keywords:
: 8930 (view as bug list)
Depends on:
Blocks: 2600 2690 2847
  Show dependency treegraph
 
Reported: 2009-09-21 01:11 UTC by Chris Cormack
Modified: 2018-12-16 19:33 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch for opac-search-history.tmpl (1.67 KB, patch)
2010-02-16 00:08 UTC, Chris Cormack
Details | Diff | Splinter Review
OPAC fixes (87.24 KB, patch)
2010-07-14 14:27 UTC, Owen Leonard
Details | Diff | Splinter Review
Revised patch for the OPAC (86.61 KB, patch)
2010-11-24 20:00 UTC, Owen Leonard
Details | Diff | Splinter Review
Proposed fix for the staff client (88.36 KB, patch)
2010-11-24 20:01 UTC, Owen Leonard
Details | Diff | Splinter Review
Revised fix (88.11 KB, patch)
2010-12-13 14:22 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 3652 : [SECURITY] XSS vulnerability (1.24 KB, patch)
2012-10-08 08:31 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 3652 : [SECURITY] XSS vulnerability (1.40 KB, patch)
2012-10-08 08:52 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 3652 : [SIGNED-OFF] [SECURITY] XSS vulnerability (1.46 KB, patch)
2012-10-08 08:52 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 3652: XSS vulnerability in page numbering (2.50 KB, patch)
2012-10-09 01:13 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 3652 XSS vulnerability in facets (3.36 KB, patch)
2012-10-09 01:20 UTC, Chris Cormack
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 3652: XSS vulnerability in page numbering (2.67 KB, patch)
2012-10-09 06:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 3652 XSS vulnerability in facets (3.55 KB, patch)
2012-10-09 06:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 3652: XSS fixes - follow up (2.04 KB, patch)
2012-10-15 07:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 3652: XSS fixes - follow up (2.04 KB, patch)
2012-10-15 07:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 3652: [SIGNED-OFF] XSS fixes - follow up (2.27 KB, patch)
2012-10-15 08:15 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 3652: close XSS vulnerabilities on biblionumber and authid (3.35 KB, patch)
2012-10-15 15:54 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 3652: close XSS vulnerabilities in opac-export (2.57 KB, patch)
2012-10-15 16:04 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 3652: close XSS vulnerabilities on biblionumber and authid (3.40 KB, patch)
2012-10-15 19:52 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 3652: close XSS vulnerabilities in opac-export (2.62 KB, patch)
2012-10-15 19:54 UTC, Chris Cormack
Details | Diff | Splinter Review
Fixing XSS vulnerability in opac-search (2.93 KB, patch)
2012-10-17 07:02 UTC, Chris Hall
Details | Diff | Splinter Review
bug 3652 fixing XSS vulnerabilities in opac-search (3.04 KB, patch)
2012-10-19 04:00 UTC, Mason James
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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