Bug 36520 - SQL Injection in opac-sendbasket.pl (CVE-2024-36058)
Summary: SQL Injection in opac-sendbasket.pl (CVE-2024-36058)
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P1 - high critical (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 33237
Blocks:
  Show dependency treegraph
 
Reported: 2024-04-04 12:30 UTC by Amit Gupta
Modified: 2024-06-20 16:11 UTC (History)
19 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.01,23.11.06,23.05.12,22.11.18,22.05.22


Attachments
SQL Injection - bib_list parameter (80.69 KB, image/jpeg)
2024-05-12 11:04 UTC, Karolis Narvilas
Details
Bug 36520 : Blind sql injection (1.33 KB, patch)
2024-05-13 02:30 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 36520 : Blind sql injection (1.39 KB, patch)
2024-05-13 03:52 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 36520 : Blind sql injection (1.45 KB, patch)
2024-05-13 10:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
[ALTERNATIVE PATCH] Bug 36520: Prevent SQL injection in GetPreparedLetter (1.90 KB, patch)
2024-05-13 12:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
SQLi - Retrieving Password Reset Links (72.50 KB, image/png)
2024-05-13 13:00 UTC, Karolis Narvilas
Details
SQLi PoC - Excerpt (45.03 KB, image/png)
2024-05-13 13:19 UTC, Karolis Narvilas
Details
Bug 36520: Prevent SQL injection in GetPreparedLetter (2.72 KB, patch)
2024-05-13 14:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36520: Prevent SQL injection in GetPreparedLetter and sanitize input in opac-sendbasket.pl (3.79 KB, patch)
2024-05-13 15:40 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 36520: Add tests (2.33 KB, patch)
2024-05-15 09:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36520: Prevent SQL injection in GetPreparedLetter (3.22 KB, patch)
2024-05-15 09:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36520: Sanitize input in opac-sendbasket.pl (1.46 KB, patch)
2024-05-15 09:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36520: Add tests (2.38 KB, patch)
2024-05-15 23:15 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 36520: Prevent SQL injection in GetPreparedLetter (3.28 KB, patch)
2024-05-15 23:15 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 36520: Sanitize input in opac-sendbasket.pl (1.52 KB, patch)
2024-05-15 23:15 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 36520: Add tests (2.45 KB, patch)
2024-05-17 06:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36520: Prevent SQL injection in GetPreparedLetter (3.37 KB, patch)
2024-05-17 06:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36520: Sanitize input in opac-sendbasket.pl (1.62 KB, patch)
2024-05-17 06:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Amit Gupta 2024-04-04 12:30:07 UTC
SQL Injection present in /cgi-bin/koha/opac-sendbasket.pl 
bib_list is VULNERABLE_PARAMETER
Comment 1 Amit Gupta 2024-04-04 12:31:15 UTC
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
Comment 2 David Cook 2024-04-04 22:18:19 UTC
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.
Comment 3 Amit Gupta 2024-04-05 03:05:11 UTC
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.
Comment 4 David Cook 2024-04-05 03:14:47 UTC
(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?
Comment 5 Amit Gupta 2024-05-08 12:10:38 UTC
Yes, checked and seems working fine.
Comment 6 Karolis Narvilas 2024-05-12 11:04:42 UTC
Created attachment 166614 [details]
SQL Injection - bib_list parameter
Comment 7 Karolis Narvilas 2024-05-12 11:13:05 UTC
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.
Comment 8 Chris Cormack 2024-05-13 02:19:18 UTC
I can confirm I can recreate the blind injection with a carefully crafted curl.
Will work on a fix now
Comment 9 Chris Cormack 2024-05-13 02:30:45 UTC
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
Comment 10 Amit Gupta 2024-05-13 03:52:17 UTC
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.
Comment 11 Martin Renvoize 2024-05-13 10:10:57 UTC
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>
Comment 12 Martin Renvoize 2024-05-13 10:11:23 UTC
Thanks for the quick fix Chris.

Passing QA
Comment 13 Jonathan Druart 2024-05-13 12:39:55 UTC
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
Comment 14 Jonathan Druart 2024-05-13 12:50:06 UTC
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.
Comment 15 Jonathan Druart 2024-05-13 12:50:51 UTC
This is not as bad as it sounds, the query is executed in a DB transaction anyway.
Comment 16 Jonathan Druart 2024-05-13 12:51:05 UTC
Back to NQA/in discussion.
Comment 17 Jonathan Druart 2024-05-13 12:51:46 UTC
IMO this is not critical.
Comment 18 Jonathan Druart 2024-05-13 12:53:07 UTC
(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?
Comment 24 Jonathan Druart 2024-05-13 14:15:38 UTC
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?
Comment 25 Jonathan Druart 2024-05-13 14:35:26 UTC
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.
Comment 26 Chris Cormack 2024-05-13 15:07:55 UTC
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.
Comment 27 Karolis Narvilas 2024-05-13 15:14:12 UTC
I agree - ensuring that bib_list is a valid integer in the first place should restrict any SQLi attempts.
Comment 28 Chris Cormack 2024-05-13 15:40:51 UTC
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>
Comment 29 Chris Cormack 2024-05-13 15:43:20 UTC
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
Comment 30 Jonathan Druart 2024-05-13 16:22:15 UTC
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 31 Victor Grousset/tuxayo 2024-05-13 18:11:49 UTC
Comment on attachment 166646 [details] [review]
Bug 36520: Prevent SQL injection in GetPreparedLetter

(obsoleted previous patch version)
Comment 32 Victor Grousset/tuxayo 2024-05-13 21:33:17 UTC
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.
Comment 33 Jonathan Druart 2024-05-14 08:46:04 UTC
(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)--+-'
Comment 34 Victor Grousset/tuxayo 2024-05-14 23:04:40 UTC
OMG yes indeed 🤦
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.
Comment 35 Jonathan Druart 2024-05-15 09:37:47 UTC
Created attachment 166760 [details] [review]
Bug 36520: Add tests
Comment 36 Jonathan Druart 2024-05-15 09:37:50 UTC
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>
Comment 37 Jonathan Druart 2024-05-15 09:37:54 UTC
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>
Comment 38 Jonathan Druart 2024-05-15 09:39:21 UTC
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.
Comment 39 Victor Grousset/tuxayo 2024-05-15 23:15:29 UTC
Created attachment 166795 [details] [review]
Bug 36520: Add tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 40 Victor Grousset/tuxayo 2024-05-15 23:15:32 UTC
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>
Comment 41 Victor Grousset/tuxayo 2024-05-15 23:15:36 UTC
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>
Comment 42 Victor Grousset/tuxayo 2024-05-15 23:57:08 UTC
> 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.
Comment 45 Marcel de Rooy 2024-05-16 07:44:41 UTC
Looking here
Comment 46 Marcel de Rooy 2024-05-16 14:06:49 UTC
(In reply to Marcel de Rooy from comment #45)
> Looking here

Resuming tomorrow here
Comment 48 Marcel de Rooy 2024-05-17 06:20:44 UTC
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 !
Comment 49 Marcel de Rooy 2024-05-17 06:25:09 UTC
(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
Comment 50 Marcel de Rooy 2024-05-17 06:38:07 UTC
(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
Comment 51 Marcel de Rooy 2024-05-17 06:41:49 UTC
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.
Comment 52 Marcel de Rooy 2024-05-17 06:44:15 UTC
# 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.
Comment 53 Marcel de Rooy 2024-05-17 06:44:51 UTC
(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 :)
Comment 54 Marcel de Rooy 2024-05-17 06:48:20 UTC
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>
Comment 55 Marcel de Rooy 2024-05-17 06:48:23 UTC
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>
Comment 56 Marcel de Rooy 2024-05-17 06:48:27 UTC
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>
Comment 57 Karolis Narvilas 2024-05-20 09:29:00 UTC
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 :)
Comment 61 David Cook 2024-05-29 02:57:47 UTC
(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.
Comment 63 Karolis Narvilas 2024-06-11 14:53:15 UTC
Thanks - is there an approximate timeline for when these fixes will be published ?
Comment 64 Katrin Fischer 2024-06-20 14:48:14 UTC
(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.
Comment 65 Katrin Fischer 2024-06-20 16:11:39 UTC
Pushed for 24.11!
Well done everyone, thank you!