Bug 18010

Summary: Remove potential exposure from gettemplate
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: TemplatesAssignee: 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

    
Comment 1 Marcel de Rooy 2017-01-29 15:23:15 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>
Comment 2 Marcel de Rooy 2017-01-29 15:23:19 UTC
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>
Comment 3 Marcel de Rooy 2017-01-29 15:31:05 UTC
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).
Comment 4 Marcel de Rooy 2017-02-17 12:46:09 UTC
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>
Comment 5 Marcel de Rooy 2017-02-17 12:46:14 UTC
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>
Comment 6 Marcel de Rooy 2017-02-17 12:46:46 UTC
Rebased ;)
Comment 7 Chris Cormack 2017-03-20 19:48:04 UTC
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>
Comment 8 Chris Cormack 2017-03-20 19:48:24 UTC
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>
Comment 9 Jonathan Druart 2017-03-20 20:13:50 UTC
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>
Comment 10 Jonathan Druart 2017-03-20 20:13:55 UTC
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>
Comment 11 Jonathan Druart 2017-03-20 20:14:38 UTC
Maybe should we reject a path if first char is '/'?
Comment 12 Marcel de Rooy 2017-03-20 20:51:08 UTC
(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.
Comment 13 Marcel de Rooy 2017-03-20 20:52:06 UTC
(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!
Comment 14 Brendan Gallagher 2017-03-23 15:38:58 UTC
Pushed to Master - Should be in the 17.05 release.  Thanks!
Comment 15 Katrin Fischer 2017-03-26 21:06:27 UTC
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
.
Comment 16 Julian Maurice 2017-03-31 13:20:16 UTC
Pushed to 3.22.x, will be in 3.22.19
Comment 17 Mason James 2017-05-03 03:46:21 UTC
Pushed to 16.05.x, for 16.05.12 release