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

#

Sun Apr 23 16:01:00 2006

OVID - Ticket created

Subject:

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

[text/plain 1.4k]

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

[text/plain 1.8k]

> 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.*\=/ ) {
---
> ### 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 $_;
---
> 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	RT_System - Status changed from 'new' to 'open'

#

Mon Apr 24 14:47:50 2006

OVID - Correspondence added

[text/plain 1.6k]

On Mon Apr 24 04:37:04 2006, kane[...]xs4all.nl wrote:
> 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>
> 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

[application/octet-stream 4.7k]

Message body not shown because it is too large or is not plain text.

#

Sat Apr 29 09:58:17 2006

KANE - Correspondence added

[text/plain 1.1k]

On Mon Apr 24 14:47:50 2006, OVID wrote:
> 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:

> =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 - Status changed from 'open' to 'resolved'