| Summary: | XSS vulnerability in the OPAC search results interface | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Robin Sheat <robin> |
| Component: | OPAC | Assignee: | 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: | ||
| Circulation function: | |||
| Attachments: |
proposed patch
Bug 5131 :restrict use of sort_by value to allowed values proposed patch (passed QA) |
||
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. |
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.