Created attachment 58965 [details] [review] Bug 17900: Fix possible SQL injection in patron cards template editing To recreate: /cgi-bin/koha/patroncards/edit-template.pl?op=edit&element_id=23%20and%201%3d2+union+all+select+1,user(),@@version+--%20 Look at the Profile dropdown list. To fix this problem and to make sure it does not appears anywhere else in the label and patroncards modules, I have refactored the way the queries are built in C4::Creators::Lib Now all of the subroutine takes a hashref in parameters with a 'fields' and 'filters' parameters. From these 2 parameters the new internal subroutine _build_query will build the query and use placeholders. Test plan: 1/ Make sure you do not recreate the vulnerability with this patch applied. 2/ With decent data in the labels and patroncards modules, compare all the different view (undef the New and Manage button groups) with and without this patch applied. => You should not see any differences. This vulnerability has been reported by MDSec.
Created attachment 59064 [details] [review] Bug 17900: Fix possible SQL injection in patron cards template editing To recreate: /cgi-bin/koha/patroncards/edit-template.pl?op=edit&element_id=23%20and%201%3d2+union+all+select+1,user(),@@version+--%20 Look at the Profile dropdown list. To fix this problem and to make sure it does not appears anywhere else in the label and patroncards modules, I have refactored the way the queries are built in C4::Creators::Lib Now all of the subroutine takes a hashref in parameters with a 'fields' and 'filters' parameters. From these 2 parameters the new internal subroutine _build_query will build the query and use placeholders. Test plan: 1/ Make sure you do not recreate the vulnerability with this patch applied. 2/ With decent data in the labels and patroncards modules, compare all the different view (undef the New and Manage button groups) with and without this patch applied. => You should not see any differences. This vulnerability has been reported by MDSec. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Running t/db_dependent/Creators/Lib.t ok 86 - creator is good Can't use string ("filter") as a HASH ref while "strict refs" in use at /usr/share/koha/masterclone/C4/Creators/Lib.pm line 147. # Looks like your test exited with 255 just after 86.
Created attachment 59603 [details] [review] Bug 17900: Update the tests to the new API
Created attachment 59612 [details] [review] Bug 17900: Fix possible SQL injection in patron cards template editing To recreate: /cgi-bin/koha/patroncards/edit-template.pl?op=edit&element_id=23%20and%201%3d2+union+all+select+1,user(),@@version+--%20 Look at the Profile dropdown list. To fix this problem and to make sure it does not appears anywhere else in the label and patroncards modules, I have refactored the way the queries are built in C4::Creators::Lib Now all of the subroutine takes a hashref in parameters with a 'fields' and 'filters' parameters. From these 2 parameters the new internal subroutine _build_query will build the query and use placeholders. Test plan: 1/ Make sure you do not recreate the vulnerability with this patch applied. 2/ With decent data in the labels and patroncards modules, compare all the different view (undef the New and Manage button groups) with and without this patch applied. => You should not see any differences. This vulnerability has been reported by MDSec. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59613 [details] [review] Bug 17900: Update the tests to the new API Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Good job, Jonathan. Code looks good to me. Mostly dry QA (code review). Test passes. Passed QA
Pushed to master for 17.05, thanks Jonathan!
This patch has been pushed to 16.11.x and will be in 16.11.03.
Pushed to 3.22.x for 3.22.16
Pushed to 16.05.x, for 16.05.09 release