Bug #18892 for Module-Load-Conditional: Security Hole Renders Module::Load::Conditional unuseable under taint checking

Sun Apr 23 16:01:00 2006 ovid [...] cpan.org - Ticket created

Subject:

Security Hole Renders Module::Load::Conditional unuseable under taint checking

I've tried to use Module::Load::Conditional with Class::CGI, but I've run into a serious problem in writing some tests. The following program illustrates the problem: #!/usr/bin/perl -T use Module::Load::Conditional qw(check_install); check_install( module => 'Data::Dumper' ); That fails with: Insecure dependency in eval while running with -T switch at /usr/local/lib/perl5/site_perl/5.8.7/Module/Load/Conditional.pm line 215, <GEN0> line 12. This happens in check_install where you're iterating over directories and trying to find the version number for a module. While you read the lines of the module, you eventually get to this (the last line is the line which makes things fall down and go boom): if ( /([\$*])(([\w\:\']*)\bVERSION)\b.*\=/ ) { ### this will eval the version in to $VERSION if it ### was declared as $VERSION in the module. ### else the result will be in $res. ### this is a fix on skud's Module::InstalledVersion local $VERSION; my $res = eval $_; This will not run under taint mode and is a showstopper for me. Now one could argue that you could just skip the VERSION check if the programmer did not pass in a "version" parameter to check_install and that would solve the problem for me. However, that doesn't fix the security hole for others who might need to check the version. I'm trying to work out a solution to this problem now. Obviously, it wont' be particularly easy and I'm not sure when I'll have anything. Cheers, Ovid

Mon Apr 24 04:37:04 2006 kane [...] xs4all.nl - Correspondence added

Subject:	Re: [rt.cpan.org #18892] Security Hole Renders Module::Load::Conditional unuseable under taint checking
Date:	Mon, 24 Apr 2006 10:36:22 +0200 (CEST)
To:	bug-Module-Load-Conditional [...] rt.cpan.org
From:	kane [...] xs4all.nl

Show quoted text

> Insecure dependency in eval while running with -T switch at > /usr/local/lib/perl5/site_perl/5.8.7/Module/Load/Conditional.pm line > 215, <GEN0> line 12. > > This happens in check_install where you're iterating over directories > and trying to find the version number for a module. While you read the > lines of the module, you eventually get to this (the last line is the > line which makes things fall down and go boom): > > if ( /([\$*])(([\w\:\']*)\bVERSION)\b.*\=/ ) { > > ### this will eval the version in to $VERSION if it > ### was declared as $VERSION in the module. > ### else the result will be in $res. > ### this is a fix on skud's Module::InstalledVersion > > local $VERSION; > my $res = eval $_;

The problem is with 'eval $_' -- i've made a minor change to M::L::C that captures the whole string from the regex and evals that, making it taint safe (not necessarily secure of course): ==== //member/kane/module-load-conditional/lib/Module/Load/Conditional.pm#21 - /Users/kane/sources/p4/other/module-load-conditional/lib/Module/Load/Conditional.pm ==== 207c207,211 < if ( /([\$*])(([\w\:\']*)\bVERSION)\b.*\=/ ) { --- Show quoted text

> ### Following #18892, which tells us the original > ### regex breaks under -T, we must modifiy it so > ### it captures the entire expression, and eval /that/ > ### rather than $_, which is insecure. > if ( /([\$*][\w\:\']*\bVERSION\b.*\=)/ ) {

215c219 < my $res = eval $_; --- Show quoted text

> my $res = eval $1;

However, as you see from the original comments, this regex was originally taken from the EU::MM documentation, which means it's probably present in quite a few more modules, but at least in EU::MM -- whatever fix we can come up with that's 'good enough' should at least be reported back there. Do you think this patch is workable? Take care, -- Jos

Mon Apr 24 04:37:06 2006 The RT System itself - Status changed from 'new' to 'open'

Mon Apr 24 14:47:50 2006 ovid [...] cpan.org - Correspondence added

On Mon Apr 24 04:37:04 2006, kane@xs4all.nl wrote: Show quoted text

> The problem is with 'eval $_' -- i've made a minor change to M::L::C > that captures the whole string from the regex and evals that, making > it taint safe (not necessarily secure of course):

Well, it's not perfect, but it lets my code run. I think I might be able to work with that so long as I put a caveat in the Class::CGI docs. To be fair, I might just internally reimplement the portions I need so I can skip the version check altogether. <snip> Show quoted text

> However, as you see from the original comments, this regex was > originally taken from the EU::MM documentation, which means it's > probably present in quite a few more modules, > but at least in EU::MM -- whatever fix we can come up with that's > 'good enough' should at least be reported back there. > > Do you think this patch is workable?

When I call this: check_install( module => 'Data::Dumper' ); I'm not stating a particular version is required. Thus, if a version is not requested, could we skip the version check altogether? I've attached a patch which does that. This eliminates the security hole for those not requiring a particular version (and the code runs a bit faster as a bonus). It does introduce a tiny change, though. The return value no longer contains the version unless a specific version was requested. This patch would completely solve my immediate problem. However, whether or not it's suitable for your code or your users is another story. As for the EU::MM stuff, I've seen that and I've been trying to work on some code which solves the problem. Unfortunately, it's a very difficult problem to solve. Cheers, Ovid

diff -ruN Module-Load-Conditional-0.08.orig/lib/Module/Load/Conditional.pm Module-Load-Conditional-0.08/lib/Module/Load/Conditional.pm --- Module-Load-Conditional-0.08.orig/lib/Module/Load/Conditional.pm 2005-01-14 09:28:25.000000000 -0800 +++ Module-Load-Conditional-0.08/lib/Module/Load/Conditional.pm 2006-04-24 11:39:37.000000000 -0700 @@ -111,7 +111,8 @@ =item version The version number of the installed module - this will be C<undef> if -the module had no (or unparsable) version number. +the module had no (or unparsable) version number or if the user did not +specifically request a particular version (or greater) of a module. =item uptodate @@ -187,38 +188,44 @@ $filename = File::Spec->catfile($dir, $file); next unless -e $filename; - $fh = new FileHandle; - if (!$fh->open($filename)) { - warn loc(q[Cannot open file '%1': %2], $file, $!) - if $args->{verbose}; - next; + if (exists $hash{version}) { + ### the user has explicitly asked for the version + $fh = new FileHandle; + if (!$fh->open($filename)) { + warn loc(q[Cannot open file '%1': %2], $file, $!) + if $args->{verbose}; + next; + } } } $href->{file} = $filename; - while (local $_ = <$fh> ) { + if ($hash{version}) { + while (local $_ = <$fh> ) { - ### skip commented out lines, they won't eval to anything. - next if /^\s*#/; + ### skip commented out lines, they won't eval to anything. + next if /^\s*#/; - ### the following regexp comes from the ExtUtils::MakeMaker - ### documentation. - if ( /([\$*])(([\w\:\']*)\bVERSION)\b.*\=/ ) { + ### the following regexp comes from the ExtUtils::MakeMaker + ### documentation. + #if ( /([\$*])(([\w\:\']*)\bVERSION)\b.*\=/ ) { + if ( /([\$*][\w\:\']*\bVERSION\b.*\=.*)/ ) { + + ### this will eval the version in to $VERSION if it + ### was declared as $VERSION in the module. + ### else the result will be in $res. + ### this is a fix on skud's Module::InstalledVersion + + local $VERSION; + my $res = eval $1; + + ### default to '0.0' if there REALLY is no version + ### all to satisfy warnings + $href->{version} = $VERSION || $res || '0.0'; - ### this will eval the version in to $VERSION if it - ### was declared as $VERSION in the module. - ### else the result will be in $res. - ### this is a fix on skud's Module::InstalledVersion - - local $VERSION; - my $res = eval $_; - - ### default to '0.0' if there REALLY is no version - ### all to satisfy warnings - $href->{version} = $VERSION || $res || '0.0'; - - last DIR; + last DIR; + } } } } @@ -227,7 +234,8 @@ return unless defined $href->{file}; ### only complain if we expected fo find a version higher than 0.0 anyway - if( !defined $href->{version} ) { + ### and they asked for a version + if( $hash{version} && !defined $href->{version} ) { { ### don't warn about the 'not numeric' stuff ### local $^W; @@ -268,7 +276,8 @@ This is a hashref of module/version pairs. The version indicates the minimum version to load. If no version is provided, any version is -assumed to be good enough. +assumed to be good enough and no attempt is made to parse the version +number. =item verbose diff -ruN Module-Load-Conditional-0.08.orig/t/01_Module_Load_Conditional.t Module-Load-Conditional-0.08/t/01_Module_Load_Conditional.t --- Module-Load-Conditional-0.08.orig/t/01_Module_Load_Conditional.t 2005-01-12 22:10:04.000000000 -0800 +++ Module-Load-Conditional-0.08/t/01_Module_Load_Conditional.t 2006-04-24 11:37:57.000000000 -0700 @@ -4,7 +4,7 @@ use lib qw[../lib lib t/to_load to_load]; use File::Spec (); -use Test::More tests => 12; +use Test::More tests => 13; ### case 1 ### use_ok( 'Module::Load::Conditional' ) or diag "Module.pm not found. Dying", die; @@ -52,9 +52,10 @@ { my $rv = check_install( module => 'Module::Load::Conditional' ); - ok( $rv->{uptodate} && $rv->{version} && $rv->{file}, + ok( $rv->{uptodate} && $rv->{file}, q[Verify any module] ); + ok( !defined $rv->{version}, q[Versions not returned unless requested]); } {

Sat Apr 29 09:58:17 2006 kane [...] cpan.org - Correspondence added

On Mon Apr 24 14:47:50 2006, OVID wrote: Show quoted text

> check_install( module => 'Data::Dumper' ); > > I'm not stating a particular version is required. Thus, if a version is > not requested, could we skip the version check altogether? I've > attached a patch which does that.

I'm a bit hesitant to change the 'default behaviour' of the module, but i can see your point as well, so i've added support for the following: Show quoted text

> =head2 $Module::Load::Conditional::FIND_VERSION > > This controls whether Module::Load::Conditional will try to parse > (and eval) the version from the module you're trying to load. > > If you don't wish to do this, set this variable to C<false>. Understand > then that version comparisons are not possible, and Module::Load::Conditional > can not tell you what module version you have installed. > This may be desirable from a security or performance point of view. > Note that C<$FIND_VERSION> code runs safely under C<taint mode>. > > The default is 1;

And also applied the taint-safe way of obtaining the version information. I'll be releasing this shortly and hopefully this will do what you need. -- Jos

Sat Apr 29 09:58:19 2006 kane [...] cpan.org - Status changed from 'open' to 'resolved'