Bug 37210 - SQL injection in overdue.pl
Summary: SQL injection in overdue.pl
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P3 normal
Assignee: Hammat wele
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-06-27 12:54 UTC by Hammat wele
Modified: 2024-08-01 15:43 UTC (History)
19 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
24.11.00,24.05.02,23.11.07,23.05.13,22.11.19
Version(s) released in:
Circulation function:


Attachments
Bug 37210: Escape single quote in search string in overdue.pl (1.82 KB, patch)
2024-06-27 14:11 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 37210: Escape single quote in search string in overdue.pl (1.87 KB, patch)
2024-06-29 23:09 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 37210: Properly escape SQL query parameters by using bind values (4.25 KB, patch)
2024-07-02 14:37 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 37210: Properly escape SQL query parameters by using bind values (4.33 KB, patch)
2024-07-04 07:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 37210: Escape single quote in search string in overdue.pl (1.97 KB, patch)
2024-07-19 08:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37210: Properly escape SQL query parameters by using bind values (4.43 KB, patch)
2024-07-19 08:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37210: Escape single quote in search string in overdue.pl (1.96 KB, patch)
2024-08-01 09:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37210: Properly escape SQL query parameters by using bind values (4.61 KB, patch)
2024-08-01 09:56 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Hammat wele 2024-06-27 12:54:11 UTC
Time-based SQL injection present in overdue.pl
To reproduce:
1. Go to /cgi-bin/koha/circ/overdue.pl
2. In the « Name or card number » field, type Tommy'and(select(0)from(select(sleep(10)))v)and'
3. Apply the filter
---> It takes 10 seconds, sleep(10) is executed
4. Inspect the page, in «Patron category:» field, put «Tommy'and(select(0)from(select(sleep(10)))v)and'» in one of his option's value
5. select the option from the filter and Apply the filter
---> It takes 10 seconds, sleep(10) is executed
we can inject SQL to the followin field : borname, itemtype, borcat, holdingbranch, homebranch and branch
Comment 1 Hammat wele 2024-06-27 14:11:26 UTC
Created attachment 168197 [details] [review]
Bug 37210: Escape single quote in search string in overdue.pl

To Test:
1. Go to /cgi-bin/koha/circ/overdue.pl
2. In the «Name or card number» field, type «Tommy'and(select(0)from(select(sleep(10)))v)and'»
3. Apply the filter
   ==> It takes 10 seconds, sleep(10) is executed
4. Inspect the page, in «Patron category:» field, put «Tommy'and(select(0)from(select(sleep(10)))v)and'» in one of his option's value
5. select the option from the filter and Apply the filter
   ==> It takes 10 seconds, sleep(10) is executed
we can inject SQL to the followin field : borname, itemtype, borcat, holdingbranch, homebranch and branch
6. Apply the patch
7. Repeat step 1,2,3
   ==> it doesn't take 10 seconds, the injected sql is not executed
8. Repeat step 5
==> it doesn't take 10 seconds, the injected sql is not executed
9. Repeat step 5 with the followin field : itemtype, holdingbranch, homebranch and branch
   ==> it doesn't take 10 seconds, the injected sql is not executed
Comment 2 David Cook 2024-06-28 00:01:50 UTC
Good catch.

This does seem like an improvement over the current situation, but I think a robust fix would need these converted into SQL parameters (like $dateduefrom and $datedueto).

We'd have a @bind array passed to $sth->execute(), and as SQL parameters are added into the SQL string, you can push the value data into @bind.
Comment 3 Chris Cormack 2024-06-29 23:09:07 UTC
Created attachment 168300 [details] [review]
Bug 37210: Escape single quote in search string in overdue.pl

To Test:
1. Go to /cgi-bin/koha/circ/overdue.pl
2. In the «Name or card number» field, type «Tommy'and(select(0)from(select(sleep(10)))v)and'»
3. Apply the filter
   ==> It takes 10 seconds, sleep(10) is executed
4. Inspect the page, in «Patron category:» field, put «Tommy'and(select(0)from(select(sleep(10)))v)and'» in one of his option's value
5. select the option from the filter and Apply the filter
   ==> It takes 10 seconds, sleep(10) is executed
we can inject SQL to the followin field : borname, itemtype, borcat, holdingbranch, homebranch and branch
6. Apply the patch
7. Repeat step 1,2,3
   ==> it doesn't take 10 seconds, the injected sql is not executed
8. Repeat step 5
==> it doesn't take 10 seconds, the injected sql is not executed
9. Repeat step 5 with the followin field : itemtype, holdingbranch, homebranch and branch
   ==> it doesn't take 10 seconds, the injected sql is not executed

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 4 Chris Cormack 2024-06-29 23:10:09 UTC
This patch fixes it but we should put this whole script on the list for a full refactor at some point.
Comment 5 Julian Maurice 2024-07-02 14:37:29 UTC
Created attachment 168372 [details] [review]
Bug 37210: Properly escape SQL query parameters by using bind values
Comment 6 Julian Maurice 2024-07-02 14:41:23 UTC
Escaping quotes with a backslash can be disabled using https://mariadb.com/kb/en/sql-mode/#no_backslash_escapes so in theory the first patch alone could have left some instances vulnerable
Comment 7 Julian Maurice 2024-07-03 11:38:43 UTC
(In reply to Julian Maurice from comment #6)
> Escaping quotes with a backslash can be disabled using
> https://mariadb.com/kb/en/sql-mode/#no_backslash_escapes so in theory the
> first patch alone could have left some instances vulnerable

Erratum: This is probably wrong as Koha always set the sql_mode when connecting to the database and it does not include no_backlash_escapes. But plugins can change the sql_mode, so... better be careful
Comment 8 David Cook 2024-07-04 00:20:55 UTC
Thanks for providing that patch, Julian.

I'm curious if the DateTime::Format::MySQL->format_datetime() part was left off intentionally or not.

I'll make a note to look at this one in any case. (Lots of time off work at the moment, so makes it a bit tricky to get Bugzilla stuff done too.)
Comment 9 Julian Maurice 2024-07-04 07:02:16 UTC
Created attachment 168487 [details] [review]
Bug 37210: Properly escape SQL query parameters by using bind values
Comment 10 Julian Maurice 2024-07-04 07:03:58 UTC
(In reply to David Cook from comment #8)
> I'm curious if the DateTime::Format::MySQL->format_datetime() part was left
> off intentionally or not.

No it was not... This is fixed in the latest patch.

Not sure if it was useful though. Date filters still worked and the resulting query parameters were the same.
Comment 11 Mark Hofstetter 2024-07-04 07:14:54 UTC
bind params is definitely the way to go, I'll try to test it
Comment 12 Marcel de Rooy 2024-07-19 08:49:27 UTC
Created attachment 169182 [details] [review]
Bug 37210: Escape single quote in search string in overdue.pl

To Test:
1. Go to /cgi-bin/koha/circ/overdue.pl
2. In the «Name or card number» field, type «Tommy'and(select(0)from(select(sleep(10)))v)and'»
3. Apply the filter
   ==> It takes 10 seconds, sleep(10) is executed
4. Inspect the page, in «Patron category:» field, put «Tommy'and(select(0)from(select(sleep(10)))v)and'» in one of his option's value
5. select the option from the filter and Apply the filter
   ==> It takes 10 seconds, sleep(10) is executed
we can inject SQL to the followin field : borname, itemtype, borcat, holdingbranch, homebranch and branch
6. Apply the patch
7. Repeat step 1,2,3
   ==> it doesn't take 10 seconds, the injected sql is not executed
8. Repeat step 5
==> it doesn't take 10 seconds, the injected sql is not executed
9. Repeat step 5 with the followin field : itemtype, holdingbranch, homebranch and branch
   ==> it doesn't take 10 seconds, the injected sql is not executed

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2024-07-19 08:49:30 UTC
Created attachment 169183 [details] [review]
Bug 37210: Properly escape SQL query parameters by using bind values

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2024-07-19 08:50:33 UTC
(In reply to Hammat wele from comment #0)
> ---> It takes 10 seconds, sleep(10) is executed

Think it takes 30 secs. Since it is inserted a few times here and there..
Comment 15 Lucas Gass 2024-07-25 15:22:17 UTC
This has been backported to 24.05.x-security branch for 24.05.02
Comment 16 Katrin Fischer 2024-07-31 16:06:37 UTC
I have some trouble with conflicts here. Please rebase on main!

I believe the tidy of the file here is the cause, but would prefer a fix from someone who has worked on this before:

https://git.koha-community.org/Koha-community/Koha/commit/67db70d43fe0566fb4c1901f810b4f37e85a6a34
Comment 17 Martin Renvoize 2024-08-01 09:56:49 UTC
Created attachment 169929 [details] [review]
Bug 37210: Escape single quote in search string in overdue.pl

To Test:
1. Go to /cgi-bin/koha/circ/overdue.pl
2. In the «Name or card number» field, type «Tommy'and(select(0)from(select(sleep(10)))v)and'»
3. Apply the filter
   ==> It takes 10 seconds, sleep(10) is executed
4. Inspect the page, in «Patron category:» field, put «Tommy'and(select(0)from(select(sleep(10)))v)and'» in one of his option's value
5. select the option from the filter and Apply the filter
   ==> It takes 10 seconds, sleep(10) is executed
we can inject SQL to the followin field : borname, itemtype, borcat, holdingbranch, homebranch and branch
6. Apply the patch
7. Repeat step 1,2,3
   ==> it doesn't take 10 seconds, the injected sql is not executed
8. Repeat step 5
==> it doesn't take 10 seconds, the injected sql is not executed
9. Repeat step 5 with the followin field : itemtype, holdingbranch, homebranch and branch
   ==> it doesn't take 10 seconds, the injected sql is not executed

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Martin Renvoize 2024-08-01 09:56:53 UTC
Created attachment 169930 [details] [review]
Bug 37210: Properly escape SQL query parameters by using bind values

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Rebased-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Katrin Fischer 2024-08-01 15:39:14 UTC
Pushed for 24.11!

Well done everyone, thank you!