Bug 5131 - XSS vulnerability in the OPAC search results interface
Summary: XSS vulnerability in the OPAC search results interface
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: PATCH-Sent (DO NOT USE) major (vote)
Assignee: Owen Leonard
QA Contact: Bugs List
Depends on:
Reported: 2010-08-12 23:55 UTC by Robin Sheat
Modified: 2013-12-05 20:04 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:

proposed patch (1.44 KB, patch)
2011-07-27 11:19 UTC, Sébastien Marie
Details | Diff | Splinter Review
Bug 5131 :restrict use of sort_by value to allowed values (1.49 KB, patch)
2011-07-27 21:16 UTC, Chris Cormack
Details | Diff | Splinter Review
proposed patch (passed QA) (1.55 KB, patch)
2011-08-03 13:32 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Sheat 2010-08-12 23:55:48 UTC
In opac-search.pl:

foreach my $sort (@sort_by) {
    $template->param($sort => 1);   # FIXME: security hole.  can set any TMPL_VAR here

This makes it pretty easy for someone to do things like inject arbitrary HTML and Javascript, and so steal the credentials of another user.

This operation needs a whitelist of valid values that it checks against.
Comment 1 Robin Sheat 2010-08-13 00:04:39 UTC
Oh, it's not all that bad after all - you can't inject arbitrary code, but you can still overwrite any TMPL_VAR.
Comment 2 Sébastien Marie 2011-07-26 09:06:35 UTC
As this kind of code permit user to control some System Preferences setted normally by admin, it should be corrected.

One example: activation of Amazon images (on 3.4.2, using  "sort_by=OPACAmazonEnabled&sort_by=OPACAmazonCoverImages")

If it don't permit injection of code (it only add a template parameter of the chosen name and with value = "1"), it could permit to control of workflow for inclusion of unwanted part of code. So it is a possible vector for potentials issues.
Comment 3 Sébastien Marie 2011-07-27 11:19:39 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2011-07-27 21:16:41 UTC Comment hidden (obsolete)
Comment 5 Paul Poulain 2011-08-03 13:32:06 UTC
Created attachment 4821 [details] [review]
proposed patch (passed QA)

QA comment

Will mark as "passed QA" but I have a few comments though:
* it's more perlish to write 
  $x= value if $cond;
rather than 
  if ($cond) {

* The list is hardcoded, so if one day a new sorting is added, we will have to add the sorting here as well (but there's no other option, I agree)
Comment 6 Chris Cormack 2011-08-04 01:19:25 UTC
Pushed, please test
Comment 7 Jared Camins-Esakov 2012-12-31 00:03:49 UTC
There have been no further reports of problems so I am marking this bug resolved.