Summary: | Remove potential exposure from gettemplate | ||
---|---|---|---|
Product: | Koha | Reporter: | Marcel de Rooy <m.de.rooy> |
Component: | Templates | Assignee: | Marcel de Rooy <m.de.rooy> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | normal | ||
Priority: | P5 - low | CC: | brendan, chris, jonathan.druart, julian.maurice, katrin.fischer, kyle, martin.renvoize, mtj, nick, tomascohen |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Trivial patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Bug Depends on: | |||
Bug Blocks: | 17989 | ||
Attachments: |
Bug 18010: Unit test for gettemplate
Bug 18010: Remove potential exposure from gettemplate Bug 18010: Unit test for gettemplate Bug 18010: Remove potential exposure from gettemplate Bug 18010: Unit test for gettemplate Bug 18010: Remove potential exposure from gettemplate Bug 18010: Unit test for gettemplate Bug 18010: Remove potential exposure from gettemplate |
Description
Marcel de Rooy
2017-01-29 14:30:07 UTC
Created attachment 59631 [details] [review] Bug 18010: Unit test for gettemplate A trivial test, similar to the ones in Auth.t. Without the check in gettemplate (added in the second patch), the passwd file will be exposed and the test fails. Test plan: Run t/db_dependent/Templates.t without second patch. The two tests in the last subtest should fail. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 59632 [details] [review] Bug 18010: Remove potential exposure from gettemplate A similar bad template check from C4::Auth::get_template_and_user should be applied in C4::Templates::gettemplate. Before this patch it would be possible to expose files like: my $template = C4::Templates::gettemplate( '/etc/passwd', 'intranet', CGI::new, 1 ); print $template->output; Note that the is_plugin flag in the above call is the culprit. This patch provides a quick security fix without touching get_template_and_user, and can be backported to stable branches. I will provide an enhanced and centralized check on report 17989, also removing the is_plugin flag. Note: We allow .pref here too for use in admin/preferences.pl. Test plan: [1] Run t/db_dependent/Auth.t (triggering get_template_and_user and gettemplate). [2] Run t/db_dependent/Templates.t again (see first test plan). The tests should no longer fail. [3] Open a page on opac or intranet. [4] Open a systempreferences tab. [5] Add a book to the cart and send it ([opac-]sendbasket uses gettemplate). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> This seems to be a theoretical exploit in view of the current calls of gettemplate. Due to its nature, it seemed to be better imo though to put this on a separate security report. Further enhancement on a regular report (17989). Created attachment 60418 [details] [review] Bug 18010: Unit test for gettemplate A trivial test, similar to the ones in Auth.t. Without the check in gettemplate (added in the second patch), the passwd file will be exposed and the test fails. Test plan: Run t/db_dependent/Templates.t without second patch. The two tests in the last subtest should fail. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 60419 [details] [review] Bug 18010: Remove potential exposure from gettemplate A similar bad template check from C4::Auth::get_template_and_user should be applied in C4::Templates::gettemplate. Before this patch it would be possible to expose files like: my $template = C4::Templates::gettemplate( '/etc/passwd', 'intranet', CGI::new, 1 ); print $template->output; Note that the is_plugin flag in the above call is the culprit. This patch provides a quick security fix without touching get_template_and_user, and can be backported to stable branches. I will provide an enhanced and centralized check on report 17989, also removing the is_plugin flag. Note: We allow .pref here too for use in admin/preferences.pl. Test plan: [1] Run t/db_dependent/Auth.t (triggering get_template_and_user and gettemplate). [2] Run t/db_dependent/Templates.t again (see first test plan). The tests should no longer fail. [3] Open a page on opac or intranet. [4] Open a systempreferences tab. [5] Add a book to the cart and send it ([opac-]sendbasket uses gettemplate). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased ;) Created attachment 61340 [details] [review] Bug 18010: Unit test for gettemplate A trivial test, similar to the ones in Auth.t. Without the check in gettemplate (added in the second patch), the passwd file will be exposed and the test fails. Test plan: Run t/db_dependent/Templates.t without second patch. The two tests in the last subtest should fail. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 61341 [details] [review] Bug 18010: Remove potential exposure from gettemplate A similar bad template check from C4::Auth::get_template_and_user should be applied in C4::Templates::gettemplate. Before this patch it would be possible to expose files like: my $template = C4::Templates::gettemplate( '/etc/passwd', 'intranet', CGI::new, 1 ); print $template->output; Note that the is_plugin flag in the above call is the culprit. This patch provides a quick security fix without touching get_template_and_user, and can be backported to stable branches. I will provide an enhanced and centralized check on report 17989, also removing the is_plugin flag. Note: We allow .pref here too for use in admin/preferences.pl. Test plan: [1] Run t/db_dependent/Auth.t (triggering get_template_and_user and gettemplate). [2] Run t/db_dependent/Templates.t again (see first test plan). The tests should no longer fail. [3] Open a page on opac or intranet. [4] Open a systempreferences tab. [5] Add a book to the cart and send it ([opac-]sendbasket uses gettemplate). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 61344 [details] [review] Bug 18010: Unit test for gettemplate A trivial test, similar to the ones in Auth.t. Without the check in gettemplate (added in the second patch), the passwd file will be exposed and the test fails. Test plan: Run t/db_dependent/Templates.t without second patch. The two tests in the last subtest should fail. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 61345 [details] [review] Bug 18010: Remove potential exposure from gettemplate A similar bad template check from C4::Auth::get_template_and_user should be applied in C4::Templates::gettemplate. Before this patch it would be possible to expose files like: my $template = C4::Templates::gettemplate( '/etc/passwd', 'intranet', CGI::new, 1 ); print $template->output; Note that the is_plugin flag in the above call is the culprit. This patch provides a quick security fix without touching get_template_and_user, and can be backported to stable branches. I will provide an enhanced and centralized check on report 17989, also removing the is_plugin flag. Note: We allow .pref here too for use in admin/preferences.pl. Test plan: [1] Run t/db_dependent/Auth.t (triggering get_template_and_user and gettemplate). [2] Run t/db_dependent/Templates.t again (see first test plan). The tests should no longer fail. [3] Open a page on opac or intranet. [4] Open a systempreferences tab. [5] Add a book to the cart and send it ([opac-]sendbasket uses gettemplate). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Maybe should we reject a path if first char is '/'? (In reply to Jonathan Druart from comment #11) > Maybe should we reject a path if first char is '/'? No, we need it for plugins now. But in a follow-up patch I plan to add additional control. (In reply to Jonathan Druart from comment #10) > Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> > Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Thanks! Pushed to Master - Should be in the 17.05 release. Thanks! Had to fix a conflict in the unit tests - all passing now. These patches have been pushed to 16.11.x and will be in 16.11.06 . Pushed to 3.22.x, will be in 3.22.19 Pushed to 16.05.x, for 16.05.12 release |