Bug 5131

Summary: XSS vulnerability in the OPAC search results interface
Product: Koha Reporter: Robin Sheat <robin>
Component: OPACAssignee: Owen Leonard <oleonard>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: major    
Priority: PATCH-Sent (DO NOT USE) CC: chris, paul.poulain, semarie
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:
Attachments: proposed patch
Bug 5131 :restrict use of sort_by value to allowed values
proposed patch (passed QA)

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) {
    $x=value;
  }

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