The "mDataProp_5" field in the "Lists" page below is vulnerable to time-based SQL injection. An attacker can exploit this to read any information in the MariaDB database. This could therefore be used to extract sensitive information such as usernames, password hashes and library users' personally identifiable information. https://bcp-staff.koha-ptfs.co.uk/cgi-bin/koha/virtualshelves/shelves.pl The following proof of concept demonstrates that it is possible to inject arbitrary SQL queries in the "mDataProp_5" field. The highlighted SQL command instructs the server to pause the SQL query execution for 40 seconds. POST https://bcp-staff.koha-ptfs.co.uk/cgi-bin/koha/svc/virtualshelves/search [...] mDataProp_4=dt_owner&bSortable_4=true&mDataProp_5=dt_sortby,(select*f rom(select(sleep(40)))a)&bSortable_5=true&mDataProp_6=dt_created_on&b Sortable_6=true&mDataProp_7=dt_modification_time&bSortable_7=true&mDa taProp_8=dt_action&bSortable_8=false&iSortCol_0=5&sSortDir_0=asc&iSor tingCols=1&type=1&shelfname=&owner=&sortby=&template_path=virtualshel ves%2Ftables%2Fshelves_results.tt As a result, the request above was executed in 41.269 milliseconds which indicates a successful execution of the injected SQL query. This vulnerability can be leveraged to extract information from the database. The following SQL statement selects the username of the user running the database and the extracts the first character from the username. Furthermore, the "IF()" function is used to compare if the extracted character matches the test character provided. As an example, if the first character of the username is equal the letter "k", then the database would execute "SLEEP(5)" statement, which would delay the response by five seconds. Alternatively, if the first username's character was not "k", then the response would only be delayed by one second. This example can be translated into the following SQL query: (select*from(SELECT+IF((SELECT+MID(user(),+1,+1)+AS+ExtractString)='k ',+SLEEP(5),+SLEEP(1)))b) Confidential Project No: 500-1169Q1 Page 13 of 61 Date: 2021-02-11 Using this logic, it was possible to determine the rest of the characters and extract the full username "koha_bcp@localhost". By automating this process, an authenticated attacker can extract a wealth of information from the KOHA database. This includes personally identifiable information of library users. Impact: High An attacker can execute arbitrary SQL queries and gain access to sensitive personally identifiable information such as library user addresses, date of birth and contact details. This information can be used for targeted social engineering attacks or identity theft. Likelihood: Medium SQL injection is a well-known vulnerability and some of the pages that are vulnerable are publicly accessible and straightforward to exploit. A large number of automated tools exist to exploit SQL injection. However, to exploit this issue an attacker would need to have an administrative access to the "Lists" page. Recommendation Review all of the code used by the website and use prepared statements where possible for all SQL calls. If this is not possible, ensure that all user input is satanized from dangerous characters or use data type casting for all other data types. Please see the following OWASP article for more information: https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.ht ml
Created attachment 116992 [details] [review] Bug 27715: Remove unused build_dt_query
Created attachment 116993 [details] [review] Bug 27715: Remove unused dt_build_having
Created attachment 116994 [details] [review] Bug 27715: Add tests
Created attachment 116995 [details] [review] Bug 27715: Sanitize order by DT params We are not on the safe side when we build the ORDER BY clause from the DataTables parameters. I've started to limit the columns by using Koha::Objects->columns, but for instance for the patron search we need (at least) the columns from the branches, categories and members tables. It seems easier, and still safe, to use a regex.
We will have to backport these patches into all the stable branches so please test carefully the list and patrons searches (filters, sort, etc.)
Created attachment 116998 [details] [review] Bug 27715: Add a deprecation notice
Created attachment 117027 [details] [review] Bug 27715: Remove unused build_dt_query Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 117028 [details] [review] Bug 27715: Remove unused dt_build_having Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 117029 [details] [review] Bug 27715: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 117030 [details] [review] Bug 27715: Sanitize order by DT params We are not on the safe side when we build the ORDER BY clause from the DataTables parameters. I've started to limit the columns by using Koha::Objects->columns, but for instance for the patron search we need (at least) the columns from the branches, categories and members tables. It seems easier, and still safe, to use a regex. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 117031 [details] [review] Bug 27715: Add a deprecation notice Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Thanks for jumping on this so quickly Jonathan :) Great to see a bit of cleaning up going on too.. I hadn't dug far enough to spot that half these routines are no longer actively used! Patches work as expected and close the loop nicely. Signing off.
Created attachment 117032 [details] [review] Bug 27715: Remove unused build_dt_query Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 117033 [details] [review] Bug 27715: Remove unused dt_build_having Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 117034 [details] [review] Bug 27715: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 117035 [details] [review] Bug 27715: Sanitize order by DT params We are not on the safe side when we build the ORDER BY clause from the DataTables parameters. I've started to limit the columns by using Koha::Objects->columns, but for instance for the patron search we need (at least) the columns from the branches, categories and members tables. It seems easier, and still safe, to use a regex. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 117036 [details] [review] Bug 27715: Add a deprecation notice Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
(In reply to Martin Renvoize from comment #16) > Created attachment 117035 [details] [review] [review] > Bug 27715: Sanitize order by DT params > > We are not on the safe side when we build the ORDER BY clause from the > DataTables parameters. > > I've started to limit the columns by using Koha::Objects->columns, but > for instance for the patron search we need (at least) the columns from > the branches, categories and members tables. > It seems easier, and still safe, to use a regex. > > Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> I am not 100% convinced this protects us from all injections, there is so many unicode characters in \w. In C4/Utils/DataTables/Members.pm for this we use $dbh->quote_identifier($searchfield) – I think we might be able to use it here as well but have to do a bit more complex change. Please see https://metacpan.org/pod/DBI#quote_identifier for documentation.
I forgot about quote_identifier, and only tried quote (it didn't work).
I started now working on adding quote_identifier() sanitization to this. Will let you know the results.
Created attachment 117068 [details] [review] Bug 27715: Remove unused build_dt_query Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 117069 [details] [review] Bug 27715: Remove unused dt_build_having Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 117070 [details] [review] Bug 27715: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 117071 [details] [review] Bug 27715: Sanitize order by DT params We are not on the safe side when we build the ORDER BY clause from the DataTables parameters. I've started to limit the columns by using Koha::Objects->columns, but for instance for the patron search we need (at least) the columns from the branches, categories and members tables. It seems easier, and still safe, to use a regex. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 117072 [details] [review] Bug 27715: Add a deprecation notice Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 117073 [details] [review] Bug 27715: Use $dbh->quote_identifier to quote untrusted input The sanitization using regex and \w class of characters might be enough but given the vast number of unicode characters in \w and possibility of in the future the database engines interpreting some of those characters with special meaning it is better to wrap the column identifier to quotes using $dbh->quote_identifier so it is only interpreted as identifier and nothing else.
Created attachment 117074 [details] [review] Bug 27715: (follow-up) Remove accidentally introduced debugging statement
I added the quotation part on top of the earlier fix, doesn't hurt to have two levels of defense? Albeit it is now a bit more complex...
Created attachment 117076 [details] [review] Bug 27715: Remove unused build_dt_query Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 117077 [details] [review] Bug 27715: Remove unused dt_build_having Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 117078 [details] [review] Bug 27715: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 117079 [details] [review] Bug 27715: Sanitize order by DT params We are not on the safe side when we build the ORDER BY clause from the DataTables parameters. I've started to limit the columns by using Koha::Objects->columns, but for instance for the patron search we need (at least) the columns from the branches, categories and members tables. It seems easier, and still safe, to use a regex. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 117080 [details] [review] Bug 27715: Add a deprecation notice Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 117081 [details] [review] Bug 27715: Use $dbh->quote_identifier to quote untrusted input The sanitization using regex and \w class of characters might be enough but given the vast number of unicode characters in \w and possibility of in the future the database engines interpreting some of those characters with special meaning it is better to wrap the column identifier to quotes using $dbh->quote_identifier so it is only interpreted as identifier and nothing else. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 117082 [details] [review] Bug 27715: (follow-up) Remove accidentally introduced debugging statement Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Nice teamwork there guys.. I've tested the followups and am really happy with them. Signed off those two too
Created attachment 117112 [details] [review] Bug 27715: Remove unused build_dt_query Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 117113 [details] [review] Bug 27715: Remove unused dt_build_having Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Amended: removed warn. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 117114 [details] [review] Bug 27715: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Amended: chmod 755. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 117115 [details] [review] Bug 27715: Sanitize order by DT params We are not on the safe side when we build the ORDER BY clause from the DataTables parameters. I've started to limit the columns by using Koha::Objects->columns, but for instance for the patron search we need (at least) the columns from the branches, categories and members tables. It seems easier, and still safe, to use a regex. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 117116 [details] [review] Bug 27715: Add a deprecation notice Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 117117 [details] [review] Bug 27715: Use $dbh->quote_identifier to quote untrusted input The sanitization using regex and \w class of characters might be enough but given the vast number of unicode characters in \w and possibility of in the future the database engines interpreting some of those characters with special meaning it is better to wrap the column identifier to quotes using $dbh->quote_identifier so it is only interpreted as identifier and nothing else. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 117118 [details] [review] Bug 27715: (QA follow-up) Remove conditional declaration Resolves: FAIL critic # Variables::ProhibitConditionalDeclarations: Got 1 violation(s). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt: 'sAjaxSource': "/cgi-bin/koha/svc/virtualshelves/search", This svc needs catalogue permission. The opac-shelves html does not run ajax svc script to get the info. So the impact is less high. Passed QA
Will be in this month release of stable branches
Backported: Pushed to 19.11.x branch for 19.11.15
Backported to 20.05.x for 20.05.09
Pushed to 20.11.x for 20.11.03