Bug 17900 - Possible SQL injection in patroncards template editing
Summary: Possible SQL injection in patroncards template editing
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Label/patron card printing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 18535 18044
  Show dependency treegraph
 
Reported: 2017-01-13 15:33 UTC by Jonathan Druart
Modified: 2017-08-08 04:28 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17900: Fix possible SQL injection in patron cards template editing (16.35 KB, patch)
2017-01-13 16:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17900: Fix possible SQL injection in patron cards template editing (16.41 KB, patch)
2017-01-17 07:35 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 17900: Update the tests to the new API (3.72 KB, patch)
2017-01-27 09:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17900: Fix possible SQL injection in patron cards template editing (16.50 KB, patch)
2017-01-27 09:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17900: Update the tests to the new API (3.81 KB, patch)
2017-01-27 09:53 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 Jonathan Druart 2017-01-13 15:33:59 UTC

    
Comment 1 Jonathan Druart 2017-01-13 16:53:01 UTC
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.
Comment 2 Chris Cormack 2017-01-17 07:35:47 UTC
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>
Comment 3 Marcel de Rooy 2017-01-27 07:46:03 UTC
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.
Comment 4 Jonathan Druart 2017-01-27 09:02:15 UTC
Created attachment 59603 [details] [review]
Bug 17900: Update the tests to the new API
Comment 5 Marcel de Rooy 2017-01-27 09:53:33 UTC
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>
Comment 6 Marcel de Rooy 2017-01-27 09:53:37 UTC
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>
Comment 7 Marcel de Rooy 2017-01-27 09:54:19 UTC
Good job, Jonathan.
Code looks good to me. Mostly dry QA (code review). Test passes.
Passed QA
Comment 8 Kyle M Hall 2017-01-30 14:18:34 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 9 Katrin Fischer 2017-01-30 14:39:35 UTC
This patch has been pushed to 16.11.x and will be in 16.11.03.
Comment 10 Julian Maurice 2017-01-30 15:28:18 UTC
Pushed to 3.22.x for 3.22.16
Comment 11 Mason James 2017-01-31 09:23:26 UTC
Pushed to 16.05.x, for 16.05.09 release