Description
Martin Renvoize
2021-02-17 11:22:35 UTC
Created attachment 116992 [details] [review] Bug 27715: Remove unused build_dt_query Created attachment 116993 [details] [review] Bug 27715: Remove unused dt_build_having Created attachment 116994 [details] [review] Bug 27715: Add tests Created attachment 116995 [details] [review] Bug 27715: Sanitize order by DT params We are not on the safe side when we build the ORDER BY clause from the DataTables parameters. I've started to limit the columns by using Koha::Objects->columns, but for instance for the patron search we need (at least) the columns from the branches, categories and members tables. It seems easier, and still safe, to use a regex. We will have to backport these patches into all the stable branches so please test carefully the list and patrons searches (filters, sort, etc.) Created attachment 116998 [details] [review] Bug 27715: Add a deprecation notice Created attachment 117027 [details] [review] Bug 27715: Remove unused build_dt_query Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 117028 [details] [review] Bug 27715: Remove unused dt_build_having Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 117029 [details] [review] Bug 27715: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 117030 [details] [review] Bug 27715: Sanitize order by DT params We are not on the safe side when we build the ORDER BY clause from the DataTables parameters. I've started to limit the columns by using Koha::Objects->columns, but for instance for the patron search we need (at least) the columns from the branches, categories and members tables. It seems easier, and still safe, to use a regex. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 117031 [details] [review] Bug 27715: Add a deprecation notice Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Thanks for jumping on this so quickly Jonathan :) Great to see a bit of cleaning up going on too.. I hadn't dug far enough to spot that half these routines are no longer actively used! Patches work as expected and close the loop nicely. Signing off. Created attachment 117032 [details] [review] Bug 27715: Remove unused build_dt_query Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 117033 [details] [review] Bug 27715: Remove unused dt_build_having Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 117034 [details] [review] Bug 27715: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 117035 [details] [review] Bug 27715: Sanitize order by DT params We are not on the safe side when we build the ORDER BY clause from the DataTables parameters. I've started to limit the columns by using Koha::Objects->columns, but for instance for the patron search we need (at least) the columns from the branches, categories and members tables. It seems easier, and still safe, to use a regex. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 117036 [details] [review] Bug 27715: Add a deprecation notice Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> (In reply to Martin Renvoize from comment #16) > Created attachment 117035 [details] [review] [review] > Bug 27715: Sanitize order by DT params > > We are not on the safe side when we build the ORDER BY clause from the > DataTables parameters. > > I've started to limit the columns by using Koha::Objects->columns, but > for instance for the patron search we need (at least) the columns from > the branches, categories and members tables. > It seems easier, and still safe, to use a regex. > > Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> I am not 100% convinced this protects us from all injections, there is so many unicode characters in \w. In C4/Utils/DataTables/Members.pm for this we use $dbh->quote_identifier($searchfield) – I think we might be able to use it here as well but have to do a bit more complex change. Please see https://metacpan.org/pod/DBI#quote_identifier for documentation. I forgot about quote_identifier, and only tried quote (it didn't work). I started now working on adding quote_identifier() sanitization to this. Will let you know the results. Created attachment 117068 [details] [review] Bug 27715: Remove unused build_dt_query Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 117069 [details] [review] Bug 27715: Remove unused dt_build_having Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 117070 [details] [review] Bug 27715: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 117071 [details] [review] Bug 27715: Sanitize order by DT params We are not on the safe side when we build the ORDER BY clause from the DataTables parameters. I've started to limit the columns by using Koha::Objects->columns, but for instance for the patron search we need (at least) the columns from the branches, categories and members tables. It seems easier, and still safe, to use a regex. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 117072 [details] [review] Bug 27715: Add a deprecation notice Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 117073 [details] [review] Bug 27715: Use $dbh->quote_identifier to quote untrusted input The sanitization using regex and \w class of characters might be enough but given the vast number of unicode characters in \w and possibility of in the future the database engines interpreting some of those characters with special meaning it is better to wrap the column identifier to quotes using $dbh->quote_identifier so it is only interpreted as identifier and nothing else. Created attachment 117074 [details] [review] Bug 27715: (follow-up) Remove accidentally introduced debugging statement I added the quotation part on top of the earlier fix, doesn't hurt to have two levels of defense? Albeit it is now a bit more complex... Created attachment 117076 [details] [review] Bug 27715: Remove unused build_dt_query Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 117077 [details] [review] Bug 27715: Remove unused dt_build_having Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 117078 [details] [review] Bug 27715: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 117079 [details] [review] Bug 27715: Sanitize order by DT params We are not on the safe side when we build the ORDER BY clause from the DataTables parameters. I've started to limit the columns by using Koha::Objects->columns, but for instance for the patron search we need (at least) the columns from the branches, categories and members tables. It seems easier, and still safe, to use a regex. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 117080 [details] [review] Bug 27715: Add a deprecation notice Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 117081 [details] [review] Bug 27715: Use $dbh->quote_identifier to quote untrusted input The sanitization using regex and \w class of characters might be enough but given the vast number of unicode characters in \w and possibility of in the future the database engines interpreting some of those characters with special meaning it is better to wrap the column identifier to quotes using $dbh->quote_identifier so it is only interpreted as identifier and nothing else. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 117082 [details] [review] Bug 27715: (follow-up) Remove accidentally introduced debugging statement Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Nice teamwork there guys.. I've tested the followups and am really happy with them. Signed off those two too Created attachment 117112 [details] [review] Bug 27715: Remove unused build_dt_query Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 117113 [details] [review] Bug 27715: Remove unused dt_build_having Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Amended: removed warn. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 117114 [details] [review] Bug 27715: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Amended: chmod 755. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 117115 [details] [review] Bug 27715: Sanitize order by DT params We are not on the safe side when we build the ORDER BY clause from the DataTables parameters. I've started to limit the columns by using Koha::Objects->columns, but for instance for the patron search we need (at least) the columns from the branches, categories and members tables. It seems easier, and still safe, to use a regex. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 117116 [details] [review] Bug 27715: Add a deprecation notice Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 117117 [details] [review] Bug 27715: Use $dbh->quote_identifier to quote untrusted input The sanitization using regex and \w class of characters might be enough but given the vast number of unicode characters in \w and possibility of in the future the database engines interpreting some of those characters with special meaning it is better to wrap the column identifier to quotes using $dbh->quote_identifier so it is only interpreted as identifier and nothing else. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 117118 [details] [review] Bug 27715: (QA follow-up) Remove conditional declaration Resolves: FAIL critic # Variables::ProhibitConditionalDeclarations: Got 1 violation(s). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt: 'sAjaxSource': "/cgi-bin/koha/svc/virtualshelves/search", This svc needs catalogue permission. The opac-shelves html does not run ajax svc script to get the info. So the impact is less high. Passed QA Will be in this month release of stable branches Backported: Pushed to 19.11.x branch for 19.11.15 Backported to 20.05.x for 20.05.09 Pushed to 20.11.x for 20.11.03 |