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.
Oh, it's not all that bad after all - you can't inject arbitrary code, but you can still overwrite any TMPL_VAR.
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.
Created attachment 4753 [details] [review] proposed patch I take the list of possible values for $sort_by parameter in 'koha-tmpl/opac-tmpl/prog/en/includes/resort_form.inc' (the select/options used for display "sort by" select), build a list with, and check the $sport_by value against. So only values from this list are allowed to be setted.
Created attachment 4759 [details] [review] Bug 5131 :restrict use of sort_by value to allowed values The user input for sort_by value was used without care, resulting the possibility for user to set any Template Variable to 1. This patch restrict the values to sort field. The list of allowd_sortby was taken from 'includes/resort_form.inc'. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
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)
Pushed, please test
There have been no further reports of problems so I am marking this bug resolved.