Bug #78272 for Package-Stash: Possible security risk w/ arbitrary code execution from $ENV

Mon Jul 09 09:01:01 2012 KENTNL [...] cpan.org - Ticket created

Subject:

Possible security risk w/ arbitrary code execution from $ENV

Seems like the sort of thing that could be easily resolved in a sane fashion.

export PACKAGE_STASH_IMPLEMENTATION="PP; print qq{hurr\n}; exit 1 "

and then any code that does "use Package::Stash" will execute the arbitrary code specified in $ENV and terminate.

https://metacpan.org/source/DOY/Package-Stash-0.33/lib/Package/Stash.pm#L17

I think its safer to validate the value "Package::Stash::$IMPLEMENTATION" against a known-safe package regexp, or use Module::Runtime to load the module seem like "Sane" approaches.

Thu Dec 06 13:29:59 2012 http://carloslima.myopenid.com/ - Correspondence added

From:

rtcpan.carlim [...] inboxclean.org

I've provided a patch to this at https://github.com/carloslima/package-stash/tree/fix-rt-78272-arbitrary- code-exec-from-env This is basically the same fix that was used on UNIVERSAL::require. I copied the fix as to not add a dependency to Package::Stash, but I did use Test::Exception on the tests, I hope it is ok. Please, do let me know if I did something which is not acceptable and I'd be more than happy to provide another patch. I'm not sure where is the right place to send this, so I'm sending the pull request on GH and also updating here.

Subject:

0001-Fixes-bug-RT-78272.patch

From d4b110bec32b93e3265763887f74de4d77bcb5cc Mon Sep 17 00:00:00 2001 From: Carlos Lima <carlos@multi> Date: Fri, 7 Dec 2012 01:08:23 +0800 Subject: [PATCH] Fixes bug RT-78272 https://rt.cpan.org/Public/Bug/Display.html?id=78272 Just copied UNIVERSAL::require's solution to the same problem. I didn't just use it as to not add any non-test dependency. --- lib/Package/Stash.pm | 4 +++- t/bug-rt-78272.t | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 t/bug-rt-78272.t diff --git a/lib/Package/Stash.pm b/lib/Package/Stash.pm index 605e97b..08a5e13 100644 --- a/lib/Package/Stash.pm +++ b/lib/Package/Stash.pm @@ -12,7 +12,9 @@ BEGIN { my $err; if ($IMPLEMENTATION) { - if (!eval "require Package::Stash::$IMPLEMENTATION; 1") { + my $file = "Package::Stash::$IMPLEMENTATION.pm"; + $file =~ s{::}{/}g; + if (!eval 'require($file) ; 1') { require Carp; Carp::croak("Could not load Package::Stash::$IMPLEMENTATION: $@"); } diff --git a/t/bug-rt-78272.t b/t/bug-rt-78272.t new file mode 100644 index 0000000..670782b --- /dev/null +++ b/t/bug-rt-78272.t @@ -0,0 +1,33 @@ +use strict; +use warnings; +use Test::More tests => 1; +use Test::Exception; + +subtest 'Bug RT-78272: Arbitrary code execution from $ENV' => sub { + + # https://rt.cpan.org/Public/Bug/Display.html?id=78272 + my $e = $ENV{PACKAGE_STASH_IMPLEMENTATION} = "PP; exit 1"; + throws_ok { + require Package::Stash; + } + qr/^Could not load Package::Stash::$e/, + 'Arbitrary code in $ENV throws exception'; + + throws_ok { + delete $INC{'Package/Stash.pm'}; + require Package::Stash; + } + qr/^Could not load Package::Stash::$e/, + 'Sanity check: forcing package reload throws the exception again'; + + lives_ok { + $ENV{PACKAGE_STASH_IMPLEMENTATION} = "PP"; + delete $INC{'Package/Stash.pm'}; + require Package::Stash; + new_ok( + 'Package::Stash' => ['Foo'], + 'Loaded and able to create instances' + ); + } + 'Valid $ENV value loads correctly'; +}; -- 1.7.10.4

Thu Dec 06 13:30:00 2012 The RT System itself - Status changed from 'new' to 'open'

Mon Dec 10 05:13:48 2012 ribasushi [...] leporine.io - Correspondence added

The real fix is to migrate the entire logic to the already existing (and used in the wild) Module::Implementation. E.g.: https://metacpan.org/source/BOBTFISH/B-Hooks-EndOfScope-0.12/lib/B/Hooks/EndOfScope.pm#L18

Fri Dec 28 11:09:08 2012 http://carloslima.myopenid.com/ - Correspondence added

From:

rtcpan.carlim [...] inboxclean.org

On Mon Dec 10 05:13:48 2012, RIBASUSHI wrote: Show quoted text

> The real fix is to migrate the entire logic to the already existing > (and > used in the wild) Module::Implementation. E.g.: > https://metacpan.org/source/BOBTFISH/B-Hooks-EndOfScope- > 0.12/lib/B/Hooks/EndOfScope.pm#L18

This bug was reported long ago and the fact that it is still open makes me believe it is considered low priority. With that in mind, I assumed adding an extra dependency would not be very welcome on such base-level module. Either way, if a patch using Module::Implementation is likely to be accepted, I'd be more than happy to provide one, just do let me know.

Fri Dec 28 23:08:51 2012 ribasushi [...] leporine.io - Correspondence added

On Fri Dec 28 11:09:08 2012, http://carloslima.myopenid.com/ wrote: Show quoted text

> On Mon Dec 10 05:13:48 2012, RIBASUSHI wrote:

> > The real fix is to migrate the entire logic to the already existing > > (and > > used in the wild) Module::Implementation. E.g.: > > https://metacpan.org/source/BOBTFISH/B-Hooks-EndOfScope- > > 0.12/lib/B/Hooks/EndOfScope.pm#L18

> > This bug was reported long ago and the fact that it is still open

makes me Show quoted text

> believe it is considered low priority. > With that in mind, I assumed adding an extra dependency would not be very > welcome on such base-level module.

M::I can be considered an even lower-level, so it should be fine. Show quoted text

> > Either way, if a patch using Module::Implementation is likely to be > accepted, I'd be more than happy to provide one, just do let me know.

I can not speak for the author but a patch using M::I is preferable for at least the following reasons: 1) Will solve RT#74151 https://rt.cpan.org/Public/Bug/Display.html?id=74151 2) Will allow taint mode to be used with P::S::XS on any 5.8 perl 3) Reduces code de-duplication 4) Author seemed to prefer the M::I route last time I spoke to him I myself am very interested in this bug being fixed that way (my motivation is far reaching 5.8 and 5.6 compat). I am also trying on and off to get doy to tend to the bug queue for the past couple months, and plan on persisting. So overall such a patch won't be a waste.

Fri Dec 28 23:09:30 2012 ribasushi [...] leporine.io - Cc RIBASUSHI added

Sat Dec 29 03:15:48 2012 ether [...] cpan.org - Correspondence added

Using Module::Implementation makes sense to me. The only other thing I'd to is s/Test::Exception/Test::Fatal/, but this is pretty trivial compared to getting some sort of fix up, as this looks like a security risk.

Sat Dec 29 03:16:55 2012 ether [...] cpan.org - Cc ETHER added

Sat Dec 29 06:46:01 2012 http://carloslima.myopenid.com/ - Correspondence added

From:

rtcpan.carlim [...] inboxclean.org

On Fri Dec 28 23:08:51 2012, RIBASUSHI wrote: Show quoted text

> On Fri Dec 28 11:09:08 2012, http://carloslima.myopenid.com/ wrote:

> > With that in mind, I assumed adding an extra dependency would not be

very Show quoted text

> > welcome on such base-level module.

> > M::I can be considered an even lower-level, so it should be fine. >

> > > > Either way, if a patch using Module::Implementation is likely to be > > accepted, I'd be more than happy to provide one, just do let me

know. Show quoted text

> > I can not speak for the author but a patch using M::I is preferable

for Show quoted text

> at least the following reasons: > 1) Will solve RT#74151 https://rt.cpan.org/Public/Bug/Display.html?

id=74151 Show quoted text

> 2) Will allow taint mode to be used with P::S::XS on any 5.8 perl > 3) Reduces code de-duplication > 4) Author seemed to prefer the M::I route last time I spoke to him > > I myself am very interested in this bug being fixed that way (my > motivation is far reaching 5.8 and 5.6 compat). I am also trying on

and Show quoted text

> off to get doy to tend to the bug queue for the past couple months,

and Show quoted text

> plan on persisting. > > So overall such a patch won't be a waste.

Thanks for all the pointers. doy confirmed he'd be interested in using Module::Implementation, so here it is: https://github.com/doy/package-stash/pull/4 This converts to Module::Implementation and also moves the tests to Test::Fatal Do let me know if there is any problem with that patch; again, I'd be happy to fix :)

Fri Jan 04 04:42:10 2013 DOY [...] cpan.org - Correspondence added

Fixed in 0.34.

Fri Jan 04 04:42:11 2013 DOY [...] cpan.org - Status changed from 'open' to 'resolved'

Bug #78272 for Package-Stash: Possible security risk w/ arbitrary code execution from $ENV

Preferred bug tracker