SQL Injection present in /cgi-bin/koha/opac-sendbasket.pl bib_list is VULNERABLE_PARAMETER
Recently we have done the security vulnerability testing and found SQL Injection present in /cgi-bin/koha/opac-sendbasket.pl bib_list is VULNERABLE_PARAMETER
Do you have any additional information? At a glance, opac-sendbasket.pl in master doesn't look like it would be vulnerable to SQL injection via bib_list per se. The bib_list isn't validated, so it does look like you could pass whatever you want to the DBIC find method but only as a string.
Hi David, Thank you for your comment. I checked in 23.11 I will check on the master and see if SQL injection is present or not.
(In reply to Amit Gupta from comment #3) > Hi David, > > Thank you for your comment. > > I checked in 23.11 I will check on the master and see if SQL injection is > present or not. At a glance, master and 23.11 look the same in opac-sendbasket.pl. Do you have a proof of concept for exploiting it?
Yes, checked and seems working fine.
Created attachment 166614 [details] SQL Injection - bib_list parameter
Hi - I have recently reported bug 36818 and thought I could shed some light on this. bib_list parameter is vulnerable to Time-Based Blind SQL Injection and can be exploited by regular library users via the below payload: Request: POST /cgi-bin/koha/opac-sendbasket.pl ... …&bib_list=1)+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+-&csrf_token=... This should cause the DB to pause for approx 6 seconds before returning a response. I have written a PoC which leverages these time discrepancies to enumerate the borrower_password_recovery table to perform password resets for other's accounts. I have attached a screenshot for reference. Let me know if you need any further details.
I can confirm I can recreate the blind injection with a carefully crafted curl. Will work on a fix now
Created attachment 166617 [details] [review] Bug 36520 : Blind sql injection To test 1/ Add some items to your cart in the opac 2/ Choose send cart 3/ Open firefox developer tools and switch to the network tab 4/ Send cart 5/ In the network tab, find the post request and choose copy as curl 6/ Edit the curl command to add )+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+- to the bib_list parameter 7/ Run the curl notice it takes a long time to respond, if you want to check run the curl without the above part added 8/ Apply the patch and restart plack 9/ Run the modified curl and notice no longer the slow down 10/ Test in browser and make sure the basket is still sent
Created attachment 166618 [details] [review] Bug 36520 : Blind sql injection To test 1/ Add some items to your cart in the opac 2/ Choose send cart 3/ Open firefox developer tools and switch to the network tab 4/ Send cart 5/ In the network tab, find the post request and choose copy as curl 6/ Edit the curl command to add )+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+- to the bib_list parameter 7/ Run the curl notice it takes a long time to respond, if you want to check run the curl without the above part added 8/ Apply the patch and restart plack 9/ Run the modified curl and notice no longer the slow down 10/ Test in browser and make sure the basket is still sent Work as suggested.
Created attachment 166620 [details] [review] Bug 36520 : Blind sql injection To test 1/ Add some items to your cart in the opac 2/ Choose send cart 3/ Open firefox developer tools and switch to the network tab 4/ Send cart 5/ In the network tab, find the post request and choose copy as curl 6/ Edit the curl command to add )+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+- to the bib_list parameter 7/ Run the curl notice it takes a long time to respond, if you want to check run the curl without the above part added 8/ Apply the patch and restart plack 9/ Run the modified curl and notice no longer the slow down 10/ Test in browser and make sure the basket is still sent Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Thanks for the quick fix Chris. Passing QA
Is it enough? Where is the query executed? If this is coming from C4::Letters maybe we need to provide a global fix and escape a parameter somewhere in GetPreparedLetter
Created attachment 166632 [details] [review] [ALTERNATIVE PATCH] Bug 36520: Prevent SQL injection in GetPreparedLetter Actually in _get_tt_params The following query will delay the response SELECT `me`.`biblionumber`, `me`.`frameworkcode`, `me`.`author`, `me`.`title`, `me`.`medium`, `me`.`subtitle`, `me`.`part_number`, `me`.`part_name`, `me`.`unititle`, `me`.`notes`, `me`.`serial`, `me`.`seriestitle` , `me`.`copyrightdate`, `me`.`timestamp`, `me`.`datecreated`, `me`.`abstract` FROM `biblio` `me` WHERE `biblionumber` = '1) AND (SELECT 1 FROM (SELECT(SLEEP(6)))x)-- -' ORDER BY field( biblionumber, 1 ) AND ( SELECT 1 FROM SELECT SLEEP( 6 ) x ) -- - ) Note that those queries are run inside a transaction and thus it would not be possible to run a bad (stateful) query.
This is not as bad as it sounds, the query is executed in a DB transaction anyway.
Back to NQA/in discussion.
IMO this is not critical.
(In reply to Karolis Narvilas from comment #7) > This should cause the DB to pause for approx 6 seconds before returning a > response. I have written a PoC which leverages these time discrepancies to > enumerate the borrower_password_recovery table to perform password resets > for other's accounts. It should not be possible to modify the data in DB with this security hole. Can you show us your PoC please?
Thanks! I am lacking time at the moment to deep dive into this... Could you eventually confirm that my alternative version addresses all your concerns?
Created attachment 166646 [details] [review] Bug 36520: Prevent SQL injection in GetPreparedLetter Actually in _get_tt_params The following query will delay the response SELECT `me`.`biblionumber`, `me`.`frameworkcode`, `me`.`author`, `me`.`title`, `me`.`medium`, `me`.`subtitle`, `me`.`part_number`, `me`.`part_name`, `me`.`unititle`, `me`.`notes`, `me`.`serial`, `me`.`seriestitle` , `me`.`copyrightdate`, `me`.`timestamp`, `me`.`datecreated`, `me`.`abstract` FROM `biblio` `me` WHERE `biblionumber` = '1) AND (SELECT 1 FROM (SELECT(SLEEP(6)))x)-- -' ORDER BY field( biblionumber, 1 ) AND ( SELECT 1 FROM SELECT SLEEP( 6 ) x ) -- - ) Note that those queries are run inside a transaction and thus it would not be possible to run a bad (stateful) query.
I'm not sure why we don't still strip any non integer from the bib_list, which is what the original patch does. There is no point handing that information to the find call. We could also make the tt change, but I don't know why we wouldn't sanitize the input ftom the user too.
I agree - ensuring that bib_list is a valid integer in the first place should restrict any SQLi attempts.
Created attachment 166650 [details] [review] Bug 36520: Prevent SQL injection in GetPreparedLetter and sanitize input in opac-sendbasket.pl Actually in _get_tt_params The following query will delay the response SELECT `me`.`biblionumber`, `me`.`frameworkcode`, `me`.`author`, `me`.`title`, `me`.`medium`, `me`.`subtitle`, `me`.`part_number`, `me`.`part_name`, `me`.`unititle`, `me`.`notes`, `me`.`serial`, `me`.`seriestitle` , `me`.`copyrightdate`, `me`.`timestamp`, `me`.`datecreated`, `me`.`abstract` FROM `biblio` `me` WHERE `biblionumber` = '1) AND (SELECT 1 FROM (SELECT(SLEEP(6)))x)-- -' ORDER BY field( biblionumber, 1 ) AND ( SELECT 1 FROM SELECT SLEEP( 6 ) x ) -- - ) To test 1/ Add some items to your cart in the opac 2/ Choose send cart 3/ Open firefox developer tools and switch to the network tab 4/ Send cart 5/ In the network tab, find the post request and choose copy as curl 6/ Edit the curl command to add )+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+- to the bib_list parameter 7/ Run the curl notice it takes a long time to respond, if you want to check run the curl without the above part added 8/ Apply the patch and restart plack 9/ Run the modified curl and notice no longer the slow down 10/ Test in browser and make sure the basket is still sent Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
I have modified the patch to do both, quote chars in output and sanitize on input also. Before patch time curl 'http://127.0.0.1:8080/cgi-bin/koha/opac-sendbasket.pl' --compressed -X POST -H 'User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:125.0) Gecko/20100101 Firefox/125.0' -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8' -H 'Accept-Language: en-US,mi;q=0.7,en;q=0.3' -H 'Accept-Encoding: gzip, deflate, br' -H 'Content-Type: application/x-www-form-urlencoded' -H 'Origin: http://127.0.0.1:8080' -H 'DNT: 1' -H 'Connection: keep-alive' -H 'Referer: http://127.0.0.1:8080/cgi-bin/koha/opac-sendbasket.pl?bib_list=248/' -H 'Cookie: CGISESSID=45fa6ea8319cdf1c45911a443e209978; bib_list=248/' -H 'Upgrade-Insecure-Requests: 1' -H 'Sec-Fetch-Dest: document' -H 'Sec-Fetch-Mode: navigate' -H 'Sec-Fetch-Site: same-origin' -H 'Sec-Fetch-User: ?1' -H 'Sec-GPC: 1' --data-raw 'email_add=chris%40bigballofwax.co.nz&comment=&bib_list=248)+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+-%2F&csrf_token=745a8356f79ebac78cc4183471fb5f214ac57fac%2C4ac8f3e985fc422b648ef28c9e397ddfc17498f5%2C1715614102&op=cud-send' curl 'http://127.0.0.1:8080/cgi-bin/koha/opac-sendbasket.pl' --compressed -X 0.01s user 0.00s system 0% cpu 6.257 total After patch curl 'http://127.0.0.1:8080/cgi-bin/koha/opac-sendbasket.pl' --compressed -X 0.01s user 0.00s system 1% cpu 0.286 total
Yes, both is fine. My point was that there are certainly other places impacted, and quoting at low-level would protect us from other holes.
Comment on attachment 166646 [details] [review] Bug 36520: Prevent SQL injection in GetPreparedLetter (obsoleted previous patch version)
If I understand correctly, the injection takes place in Letters.pm. In $module->search(), the ORM can take an order_by key whose value is the result of a concatenation. And the ORM allows subqueries for order_by so we can do a SELECT anywhere else and here we can do the blind injection. Is that correct? --- Why does $dbh->quote work with the injected payload? $ re.pl my $dbh = DBI->connect('dbi:mysql:;host=db;port=3306', 'koha_kohadev', 'password'); $dbh->quote('+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+-') => '+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+-' I get the same string. --- IIUC the other fix is upstream, in the controller, enforcing types in that precise param to avoid any bad stuff to get any further in the process.
(In reply to Victor Grousset/tuxayo from comment #32) > $ re.pl > my $dbh = DBI->connect('dbi:mysql:;host=db;port=3306', 'koha_kohadev', > 'password'); > $dbh->quote('+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+-') > => '+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+-' > > I get the same string. Nope, a quoted string. use C4::Context; my $dbh = C4::Context->dbh; my $s = '+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+-'; say $s; say $dbh->quote($s); +AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+- '+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+-'
OMG yes indeed [U+1F926] The point is to in the end concatenate just a string instead of SQL code. No need for some escaping of the many special characters that make sense in SQL. I guess the DBMS treats the same 3 or '3' passed in an order by clause. Code looks good as far as I can tell then.
Created attachment 166760 [details] [review] Bug 36520: Add tests
Created attachment 166761 [details] [review] Bug 36520: Prevent SQL injection in GetPreparedLetter Actually in _get_tt_params The following query will delay the response SELECT `me`.`biblionumber`, `me`.`frameworkcode`, `me`.`author`, `me`.`title`, `me`.`medium`, `me`.`subtitle`, `me`.`part_number`, `me`.`part_name`, `me`.`unititle`, `me`.`notes`, `me`.`serial`, `me`.`seriestitle` , `me`.`copyrightdate`, `me`.`timestamp`, `me`.`datecreated`, `me`.`abstract` FROM `biblio` `me` WHERE `biblionumber` = '1) AND (SELECT 1 FROM (SELECT(SLEEP(6)))x)-- -' ORDER BY field( biblionumber, 1 ) AND ( SELECT 1 FROM SELECT SLEEP( 6 ) x ) -- - ) To test 1/ Add some items to your cart in the opac 2/ Choose send cart 3/ Open firefox developer tools and switch to the network tab 4/ Send cart 5/ In the network tab, find the post request and choose copy as curl 6/ Edit the curl command to add )+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+- to the bib_list parameter 7/ Run the curl notice it takes a long time to respond, if you want to check run the curl without the above part added 8/ Apply the patch and restart plack 9/ Run the modified curl and notice no longer the slow down 10/ Test in browser and make sure the basket is still sent Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 166762 [details] [review] Bug 36520: Sanitize input in opac-sendbasket.pl To test 1/ Add some items to your cart in the opac 2/ Choose send cart 3/ Open firefox developer tools and switch to the network tab 4/ Send cart 5/ In the network tab, find the post request and choose copy as curl 6/ Edit the curl command to add )+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+- to the bib_list parameter 7/ Run the curl notice it takes a long time to respond, if you want to check run the curl without the above part added 8/ Apply the patch and restart plack 9/ Run the modified curl and notice no longer the slow down 10/ Test in browser and make sure the basket is still sent Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Wrote tests, fixed a bug (wrong join/map order), split and restore the original patch (seems better to keep the user input sanitize and the low-level fix separated). Ready for QA.
Created attachment 166795 [details] [review] Bug 36520: Add tests Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 166796 [details] [review] Bug 36520: Prevent SQL injection in GetPreparedLetter Actually in _get_tt_params The following query will delay the response SELECT `me`.`biblionumber`, `me`.`frameworkcode`, `me`.`author`, `me`.`title`, `me`.`medium`, `me`.`subtitle`, `me`.`part_number`, `me`.`part_name`, `me`.`unititle`, `me`.`notes`, `me`.`serial`, `me`.`seriestitle` , `me`.`copyrightdate`, `me`.`timestamp`, `me`.`datecreated`, `me`.`abstract` FROM `biblio` `me` WHERE `biblionumber` = '1) AND (SELECT 1 FROM (SELECT(SLEEP(6)))x)-- -' ORDER BY field( biblionumber, 1 ) AND ( SELECT 1 FROM SELECT SLEEP( 6 ) x ) -- - ) To test 1/ Add some items to your cart in the opac 2/ Choose send cart 3/ Open firefox developer tools and switch to the network tab 4/ Send cart 5/ In the network tab, find the post request and choose copy as curl 6/ Edit the curl command to add )+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+- to the bib_list parameter 7/ Run the curl notice it takes a long time to respond, if you want to check run the curl without the above part added 8/ Apply the patch and restart plack 9/ Run the modified curl and notice no longer the slow down 10/ Test in browser and make sure the basket is still sent Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 166797 [details] [review] Bug 36520: Sanitize input in opac-sendbasket.pl To test 1/ Add some items to your cart in the opac 2/ Choose send cart 3/ Open firefox developer tools and switch to the network tab 4/ Send cart 5/ In the network tab, find the post request and choose copy as curl 6/ Edit the curl command to add )+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+- to the bib_list parameter 7/ Run the curl notice it takes a long time to respond, if you want to check run the curl without the above part added 8/ Apply the patch and restart plack 9/ Run the modified curl and notice no longer the slow down 10/ Test in browser and make sure the basket is still sent Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
> fixed a bug (wrong join/map order) Confirmed. t/db_dependent/Letters/TemplateToolkit.t fails with the old patch. And dumping the email in Koha/Email.pm::send_or_die() shows that the old patch wasn't respecting the order of the records. (you need to have in the cart records not already sorted by ascending biblionumber to see this) And that the content of the email is the same as with before the patch. Confirmed both approach can individually prevent the injection. So here is another signoff. New code still looks good.
Looking here
(In reply to Marcel de Rooy from comment #45) > Looking here Resuming tomorrow here
Letters.t currently fails for me with and without this patch. Looks like this is the cause: t::lib::Mocks::mock_config( 'intrahtdocs', '/kohadevbox/koha/t/mock_templates/intranet-tmpl' ); We should not add kohadevbox to our tests !
(In reply to Marcel de Rooy from comment #48) > Letters.t currently fails for me with and without this patch. > > Looks like this is the cause: > > t::lib::Mocks::mock_config( 'intrahtdocs', > '/kohadevbox/koha/t/mock_templates/intranet-tmpl' ); > > We should not add kohadevbox to our tests ! Bug 33237
(In reply to Marcel de Rooy from comment #49) > (In reply to Marcel de Rooy from comment #48) > > Letters.t currently fails for me with and without this patch. > > > > Looks like this is the cause: > > > > t::lib::Mocks::mock_config( 'intrahtdocs', > > '/kohadevbox/koha/t/mock_templates/intranet-tmpl' ); > > > > We should not add kohadevbox to our tests ! > > Bug 33237 Added a fix there
Dpendency on 33237 is a bit troublesome? If this secu report goes faster than the base report. But the fix is only two lines. So could be copied into.
# Subtest: Quote user params in GetPreparedLetter 1..1 not ok 1 - We should not exec the SLEEP # Failed test 'We should not exec the SLEEP' # at t/db_dependent/Letters.t line 1559. # Spent 10s to run GetPreparredLetter, SLEEP has certainly been executed which could lead to SQL injections # Looks like you failed 1 test of 1. not ok 101 - Quote user params in GetPreparedLetter Will have a look. I saw it sleeping.
(In reply to Marcel de Rooy from comment #52) > # Subtest: Quote user params in GetPreparedLetter > 1..1 > not ok 1 - We should not exec the SLEEP > # Failed test 'We should not exec the SLEEP' > # at t/db_dependent/Letters.t line 1559. > # Spent 10s to run GetPreparredLetter, SLEEP has certainly been executed > which could lead to SQL injections > # Looks like you failed 1 test of 1. > not ok 101 - Quote user params in GetPreparedLetter > > Will have a look. I saw it sleeping. Without the fix. So thats good :)
Created attachment 166861 [details] [review] Bug 36520: Add tests Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 166862 [details] [review] Bug 36520: Prevent SQL injection in GetPreparedLetter Actually in _get_tt_params The following query will delay the response SELECT `me`.`biblionumber`, `me`.`frameworkcode`, `me`.`author`, `me`.`title`, `me`.`medium`, `me`.`subtitle`, `me`.`part_number`, `me`.`part_name`, `me`.`unititle`, `me`.`notes`, `me`.`serial`, `me`.`seriestitle` , `me`.`copyrightdate`, `me`.`timestamp`, `me`.`datecreated`, `me`.`abstract` FROM `biblio` `me` WHERE `biblionumber` = '1) AND (SELECT 1 FROM (SELECT(SLEEP(6)))x)-- -' ORDER BY field( biblionumber, 1 ) AND ( SELECT 1 FROM SELECT SLEEP( 6 ) x ) -- - ) To test 1/ Add some items to your cart in the opac 2/ Choose send cart 3/ Open firefox developer tools and switch to the network tab 4/ Send cart 5/ In the network tab, find the post request and choose copy as curl 6/ Edit the curl command to add )+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+- to the bib_list parameter 7/ Run the curl notice it takes a long time to respond, if you want to check run the curl without the above part added 8/ Apply the patch and restart plack 9/ Run the modified curl and notice no longer the slow down 10/ Test in browser and make sure the basket is still sent Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 166863 [details] [review] Bug 36520: Sanitize input in opac-sendbasket.pl To test 1/ Add some items to your cart in the opac 2/ Choose send cart 3/ Open firefox developer tools and switch to the network tab 4/ Send cart 5/ In the network tab, find the post request and choose copy as curl 6/ Edit the curl command to add )+AND+(SELECT+1+FROM+(SELECT(SLEEP(6)))x)--+- to the bib_list parameter 7/ Run the curl notice it takes a long time to respond, if you want to check run the curl without the above part added 8/ Apply the patch and restart plack 9/ Run the modified curl and notice no longer the slow down 10/ Test in browser and make sure the basket is still sent Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Hi all - This issue has just been assigned CVE-2024-36058 by CVE MITRE - if possible, please reference this ID in the next release announcement (assuming this issue is fixed in that release). I have also been assigned CVE-2024-36057 for bug 36818 - I have left a comment there as well. Many thanks - let me know if you have any questions :)
(In reply to Chris Cormack from comment #26) > I'm not sure why we don't still strip any non integer from the bib_list, > which is what the original patch does. > There is no point handing that information to the find call. > > We could also make the tt change, but I don't know why we wouldn't sanitize > the input ftom the user too. +1 Note there's quite a few places in Koha where we accept user input without any validation, especially bib_list/biblionumber. We talked about this 4 years ago on bug 25339 but the discussion dropped off.
Thanks - is there an approximate timeline for when these fixes will be published ?
(In reply to Karolis Narvilas from comment #63) > Thanks - is there an approximate timeline for when these fixes will be > published ? The security releases have gone out now with these fixes.
Pushed for 24.11! Well done everyone, thank you!
Reviewed and nothing to document for the manual.