Bug #71448 for Class-Load: Security [PATCH]: Do not allow absolute paths or step paths upwards

Am Di 04. Okt 2011, 12:16:18, rurban@x-ray.at schrieb: Show quoted text
Sorry, wrong patch. Documentation and testcases missing. See the new patch. We should not allow to shoot oneself into your foot, even it's entirely the users fault not to check unsafe userinput. With Class::Load I would go with the Module::Runtime sanity checks.

diff -bu Class-Load-0.10/Makefile.PL~ Class-Load-0.10/Makefile.PL --- Class-Load-0.10/Makefile.PL~ 2011-09-06 10:14:02.000000000 -0500 +++ Class-Load-0.10/Makefile.PL 2011-10-04 11:08:13.000000000 -0500 @@ -26,7 +26,8 @@ "Data::OptList" => 0, "Package::Stash" => "0.32", "Scalar::Util" => 0, - "Try::Tiny" => 0 + "Try::Tiny" => 0, + "Module::Runtime" => "0.008" }, "VERSION" => "0.10", "test" => { diff -bu Class-Load-0.10/lib/Class/Load.pm~ Class-Load-0.10/lib/Class/Load.pm --- Class-Load-0.10/lib/Class/Load.pm~ 2011-09-06 10:14:02.000000000 -0500 +++ Class-Load-0.10/lib/Class/Load.pm 2011-10-04 12:10:10.000000000 -0500 @@ -8,6 +8,7 @@ use Data::OptList 'mkopt'; use Package::Stash; use Try::Tiny; +use Module::Runtime (); our $IMPLEMENTATION; @@ -159,13 +160,8 @@ _croak($ERROR); } -# XXX Module::Runtime? sub _is_module_name { - my $name = shift; - - return if !defined($name); - return if ref($name); - return $name =~ /\A[0-9A-Z_a-z]+(?:::[0-9A-Z_a-z]+)*\z/; + return Module::Runtime::is_module_name(shift); } sub _mod2pm { @@ -329,6 +325,8 @@ If, when attempting to load a class, it fails to load because of a syntax error, then an error will be thrown immediately. +An error will also be thrown, when a module name does not pass the +L<Module::Runtime> C<is_valid_module_name> check. =head2 load_optional_class Class::Name, \%options -> 0|1 @@ -375,6 +373,10 @@ This module was designed to be used anywhere you have C<if (eval "require $module"; 1)>, which occurs in many large projects. +=item L<http://blogs.perl.org/users/michael_g_schwern/2011/10/how-not-to-load-a-module-or-bad-interfaces-make-good-people-do-bad-things.html> + +Warn about dangerous user input in module names, which could lead to loading unexpected files. + =back =head1 AUTHOR diff -bu Class-Load-0.10/t/009-invalid-module-name.t~ Class-Load-0.10/t/009-invalid-module-name.t --- Class-Load-0.10/t/009-invalid-module-name.t~ 2011-09-06 10:14:02.000000000 -0500 +++ Class-Load-0.10/t/009-invalid-module-name.t 2011-10-04 12:15:44.000000000 -0500 @@ -6,10 +6,12 @@ use lib 't/lib'; use Test::Class::Load 'load_class'; -like( - exception { load_class('Foo:Bar') }, - qr/^Foo:Bar is not a module name/, +for my $badname ('Foo:Bar', 'Foo::..::..::tmp::bad.pl', '::..::tmp::bad') { + like( + exception { load_class($badname) }, + qr/^$badname is not a module name/, "invalid module name" -); + ); +} done_testing;

Am Di 04. Okt 2011, 13:28:14, autarch@urth.org schrieb: Show quoted text
Right. It's not a security fix. But people should be aware and this module instead of the problematic eval "require $string". Attached is an improved version where the test actually fails on the old version. Only checking a number as first char was missing in 0.10

diff -bu Class-Load-0.10/Makefile.PL~ Class-Load-0.10/Makefile.PL --- Class-Load-0.10/Makefile.PL~ 2011-09-06 10:14:02.000000000 -0500 +++ Class-Load-0.10/Makefile.PL 2011-10-04 11:08:13.000000000 -0500 @@ -26,7 +26,8 @@ "Data::OptList" => 0, "Package::Stash" => "0.32", "Scalar::Util" => 0, - "Try::Tiny" => 0 + "Try::Tiny" => 0, + "Module::Runtime" => "0.008" }, "VERSION" => "0.10", "test" => { diff -bu Class-Load-0.10/lib/Class/Load.pm~ Class-Load-0.10/lib/Class/Load.pm --- Class-Load-0.10/lib/Class/Load.pm~ 2011-09-06 10:14:02.000000000 -0500 +++ Class-Load-0.10/lib/Class/Load.pm 2011-10-04 12:10:10.000000000 -0500 @@ -8,6 +8,7 @@ use Data::OptList 'mkopt'; use Package::Stash; use Try::Tiny; +use Module::Runtime (); our $IMPLEMENTATION; @@ -159,13 +160,8 @@ _croak($ERROR); } -# XXX Module::Runtime? sub _is_module_name { - my $name = shift; - - return if !defined($name); - return if ref($name); - return $name =~ /\A[0-9A-Z_a-z]+(?:::[0-9A-Z_a-z]+)*\z/; + return Module::Runtime::is_module_name(shift); } sub _mod2pm { @@ -329,6 +325,8 @@ If, when attempting to load a class, it fails to load because of a syntax error, then an error will be thrown immediately. +An error will also be thrown, when a module name does not pass the +L<Module::Runtime> C<is_valid_module_name> check. =head2 load_optional_class Class::Name, \%options -> 0|1 @@ -375,6 +373,10 @@ This module was designed to be used anywhere you have C<if (eval "require $module"; 1)>, which occurs in many large projects. +=item L<http://blogs.perl.org/users/michael_g_schwern/2011/10/how-not-to-load-a-module-or-bad-interfaces-make-good-people-do-bad-things.html> + +Warn about dangerous user input in module names, which could lead to loading unexpected files. + =back =head1 AUTHOR diff -bu Class-Load-0.10/t/009-invalid-module-name.t~ Class-Load-0.10/t/009-invalid-module-name.t --- Class-Load-0.10/t/009-invalid-module-name.t~ 2011-09-06 10:14:02.000000000 -0500 +++ Class-Load-0.10/t/009-invalid-module-name.t 2011-10-04 14:17:16.000000000 -0500 @@ -6,10 +6,19 @@ use lib 't/lib'; use Test::Class::Load 'load_class'; -like( - exception { load_class('Foo:Bar') }, - qr/^Foo:Bar is not a module name/, - "invalid module name" -); +for my $badname ('Foo:Bar', '0Foo::Bar', 'Foo::..::..::tmp::bad.pl', '::..::tmp::bad', './test.pl') { + like( + exception { load_class($badname) }, + qr/^$badname is not a module name/, + "$badname is an invalid module name" + ); +} +for my $goodname ('Foo::Bar', 'Foo::0Bar') { # not allowed to exist + like( + exception { load_class($goodname) }, + qr/^Can't locate /, + "$goodname is a valid but not existing module name" + ); +} done_testing;