Bug 16476 - CGI->param('foo') in list context allows XSS (e.g. Javascript injection) in Koha
Summary: CGI->param('foo') in list context allows XSS (e.g. Javascript injection) in Koha
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Galen Charlton
QA Contact: Testopia
URL:
Keywords:
Depends on: 15809
Blocks: 14568
  Show dependency treegraph
 
Reported: 2016-05-09 05:12 UTC by David Cook
Modified: 2019-06-27 09:24 UTC (History)
6 users (show)

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


Attachments
Bug 16476: Do not call CGI->param in list context, some more (2.77 KB, patch)
2016-05-09 14:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16476: Do not call CGI->param in list context, some more (2.82 KB, patch)
2016-05-09 20:56 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 16476: Do not call CGI->param in list context, some more (2.89 KB, patch)
2016-05-09 21:15 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2016-05-09 05:12:07 UTC
There's a list of bug reports dealing with CGI->param in list context and CGI->multi_param:

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15747
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15809

w/ patches
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16154
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16259

While those last two bugs fix a lot of the vulnerabilities, they only fix them for master. We should really work on backporting them to 3.22, 3.20, and older if necessary... as you can inject Javascript into the URL, and do all sorts of other nasty things.

Here's a quick test to see if you're vulnerable in 3.22, 3.20, etc:

OPAC/cgi-bin/koha/opac-search.pl?expanded_options=1&expanded_options=OPACUserJS&expanded_options=alert(%27Hello%27)

This shouldn't work in master thanks to Jonathan's 16154, but older versions aren't so lucky.

You might think that anyone can inject Javascript via their own browser, but you could craft a nasty piece of Javascript - put it in the URL - and then email that URL to someone. They open it and then you're executing arbitrary Javascript in their browser. I'm no security whiz, but you could probably hijack someone's session cookie that way as well.

Due to the location of the vulnerability in opac-search.pl, you actually can rewrite any parameter in $template->{VARS} via $template->param(). 

--

There actually are some unpatched vulnerabilities in master as well.

STAFFCLIENT/cgi-bin/koha/plugins/run.pl?class=Koha::Plugin::EDS&method=configure&method=IntranetUserJS&method=alert("hello");

Obviously the above is a bit more rare... you'd have to have Koha with plugins enabled and a plugin installed (the flaw is in Koha::Base, so any plugin will exhibit this vulnerability)

--

There are also the following:
svc/members/search:    template_name   => $input->param('template_path'),
svc/virtualshelves/search:    template_name   => $input->param('template_path'),

At the moment, these can't be exploited as only known keys are used, but that's only because of their location in the code. If this line was located at the end of the get_template_and_user() call, you'd be able to rewrite authnotrequired to 1, and then have unfettered access to the staff client.

So can't really exploit these ones at the moment, but they're dangerous.

--

Owen and Jonathan have done a good job overall... those are pretty much the only vulnerabilities I can find using Galen's technique of "git grep '=>.*->param'" to check for obvious vulnerabilities.

Oh, the admin/edi_accounts.pl and admin/edi_ean_accounts.pl scripts still look vulnerable to key => value injection although the danger there doesn't look too bad. 

--

There is one I'm investigating right now and it's the following:

C4/ILSDI/Services.pm:    my $borrower = GetMember($cgi->param('id_type') => $cgi->param('id'));

I think it'll allow SQL injection...
Comment 1 David Cook 2016-05-09 05:23:30 UTC
Actually it looks like /cgi-bin/koha/ilsdi.pl blocks multiple values, so that should be OK...

In that case, I think that's all I've found.

I'd suggest patching Koha::Plugins::Base for sure and backporting the existing patches back to 3.22 and 3.20...
Comment 2 David Cook 2016-05-09 05:25:30 UTC
Actually it looks like 3.20.3 requires a lower-cased opacuserjs:

OPAC3.20.3/cgi-bin/koha/opac-search.pl?expanded_options=1&expanded_options=opacuserjs&expanded_options=alert(%27Hello%27)
Comment 3 Jonathan Druart 2016-05-09 14:04:20 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-05-09 14:05:12 UTC
David, I have submitted a follow-up for the master branch.
For stable branches, Rmaints should backport existing fixes.
Comment 5 Chris Cormack 2016-05-09 20:56:40 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2016-05-09 21:15:00 UTC
Created attachment 51349 [details] [review]
[PASSED QA] Bug 16476: Do not call CGI->param in list context, some more

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 David Cook 2016-05-10 03:05:41 UTC
(In reply to Jonathan Druart from comment #4)
> David, I have submitted a follow-up for the master branch.
> For stable branches, Rmaints should backport existing fixes.

Thanks, Jonathan. And cheers for all the work that you and Owen have done on this one in all the other reports as well!
Comment 8 Kyle M Hall 2016-05-16 17:13:20 UTC
Pushed to master for Koha 16.05, thanks Jonathan!
Comment 9 Julian Maurice 2016-05-16 17:16:43 UTC
Patch pushed to 3.22.x, will be in 3.22.7
Comment 10 Chris Cormack 2016-08-03 20:21:20 UTC
Pushed to 3.20.x will be in 3.20.14