|
Description
Hammat wele
2024-06-27 12:54:11 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 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. 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> This patch fixes it but we should put this whole script on the list for a full refactor at some point. Created attachment 168372 [details] [review] Bug 37210: Properly escape SQL query parameters by using bind values 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 (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 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.) Created attachment 168487 [details] [review] Bug 37210: Properly escape SQL query parameters by using bind values (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. bind params is definitely the way to go, I'll try to test it 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> 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> (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.. This has been backported to 24.05.x-security branch for 24.05.02 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 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> 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> Pushed for 24.11! Well done everyone, thank you! |