Skip Menu |

This queue is for tickets about the CPAN CPAN distribution.

Report information
The Basics
Id: 116507
Status: resolved
Priority: 0/
Queue: CPAN

People
Owner: Nobody in particular
Requestors: TONYC [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: 2.22



Subject: CVE-2016-1238: App::Cpan may load an optional module from the current directory
This can be used by another user on the system to attack the current user if this code is run from a globally writable directory such as /tmp. The attached patch temporarily removes . from @INC when attemptting to load optional modules. The patch is also available as a pull request at: https://github.com/andk/cpanpm/pull/101 This change is in maint-5.22, maint-5.24 and blead perl. Tony
Subject: 0001-CVE-2016-1238-don-t-load-optional-modules-from-defau.patch
From 394ac06dc5e9e94a81c39c43135d1635f516422e Mon Sep 17 00:00:00 2001 From: Tony Cook <tony@develop-help.com> Date: Wed, 27 Jul 2016 12:14:13 +1000 Subject: [PATCH] CVE-2016-1238: don't load optional modules from default . App::Cpan attempts to load several optional modules, which an attacker can use if cpan is run from a directory writable by other users, such as /tmp. --- lib/App/Cpan.pm | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/lib/App/Cpan.pm b/lib/App/Cpan.pm index f43dea9..c654c2c 100644 --- a/lib/App/Cpan.pm +++ b/lib/App/Cpan.pm @@ -549,9 +549,20 @@ sub AUTOLOAD { 1 } sub DESTROY { 1 } } +# load a module without searching the default entry for the current +# directory +sub _safe_load_module { + my $name = shift; + + local @INC = @INC; + pop @INC if $INC[-1] eq '.'; + + eval "require $name; 1"; +} + sub _init_logger { - my $log4perl_loaded = eval "require Log::Log4perl; 1"; + my $log4perl_loaded = _safe_load_module("Log::Log4perl"); unless( $log4perl_loaded ) { @@ -1020,7 +1031,7 @@ sub _load_local_lib # -I { $logger->debug( "Loading local::lib" ); - my $rc = eval { require local::lib; 1; }; + my $rc = _safe_load_module("local::lib"); unless( $rc ) { $logger->die( "Could not load local::lib" ); } @@ -1160,7 +1171,7 @@ sub _get_file { my $path = shift; - my $loaded = eval "require LWP::Simple; 1;"; + my $loaded = _safe_load_module("LWP::Simple"); croak "You need LWP::Simple to use features that fetch files from CPAN\n" unless $loaded; @@ -1182,7 +1193,7 @@ sub _gitify { my $args = shift; - my $loaded = eval "require Archive::Extract; 1;"; + my $loaded = _safe_load_module("Archive::Extract"); croak "You need Archive::Extract to use features that gitify distributions\n" unless $loaded; @@ -1245,7 +1256,7 @@ sub _show_Changes sub _get_changes_file { croak "Reading Changes files requires LWP::Simple and URI\n" - unless eval "require LWP::Simple; require URI; 1"; + unless _safe_load_module("LWP::Simple") && _safe_load_module("URI"); my $url = shift; -- 2.1.4
From: ppisar [...] redhat.com
Dne Út 26.čec.2016 22:19:21, TONYC napsal(a): Show quoted text
> This can be used by another user on the system to attack the current user if > this code is run from a globally writable directory such as /tmp. > > The attached patch temporarily removes . from @INC when attemptting to load > optional modules. >
I can see this change in commit 394ac06dc5e9e94a81c39c43135d1635f516422e. Unfortunately, the fix does not work because the $INC[-1] is not "." but an absolute path equivalent to "." when running the cpan tool.
From: ppisar [...] redhat.com
Dne St 12.říj.2016 10:22:47, ppisar napsal(a): Show quoted text
> I can see this change in commit > 394ac06dc5e9e94a81c39c43135d1635f516422e. > > Unfortunately, the fix does not work because the $INC[-1] is not "." > but an absolute path equivalent to "." when running the cpan tool.
What about the attached patch?
Subject: CPAN-2.14-Fix-CVE-2016-1238-properly.patch
From 9b0b275d923418306cb3c45bb380bd9dcc71476c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= <ppisar@redhat.com> Date: Wed, 12 Oct 2016 16:56:41 +0200 Subject: [PATCH] Fix CVE-2016-1238 properly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removing "." from @INC does not work because CPAN module translates all relative paths into absolute paths. Check for $INC[-1] eq '.' sooner. Signed-off-by: Petr Písař <ppisar@redhat.com> --- lib/App/Cpan.pm | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/App/Cpan.pm b/lib/App/Cpan.pm index c654c2c..ce7afe5 100644 --- a/lib/App/Cpan.pm +++ b/lib/App/Cpan.pm @@ -1,5 +1,11 @@ package App::Cpan; +# CPAN module translantes @INC, CPAN RT#116507 +my $last_inc_is_dot; +BEGIN { + $last_inc_is_dot = $INC[-1] eq '.'; +} + use strict; use warnings; use vars qw($VERSION); @@ -555,7 +561,7 @@ sub _safe_load_module { my $name = shift; local @INC = @INC; - pop @INC if $INC[-1] eq '.'; + pop @INC if $last_inc_is_dot; eval "require $name; 1"; } -- 2.7.4
From: ppisar [...] redhat.com
Dne St 12.říj.2016 11:41:51, ppisar napsal(a): Show quoted text
> Dne St 12.říj.2016 10:22:47, ppisar napsal(a):
> > I can see this change in commit > > 394ac06dc5e9e94a81c39c43135d1635f516422e. > > > > Unfortunately, the fix does not work because the $INC[-1] is not "." > > but an absolute path equivalent to "." when running the cpan tool.
> > What about the attached patch?
Looking at perl-v5.24.1-RC4, there is different fix that is missing from CPAN's git tree: --- a/cpan/CPAN/scripts/cpan +++ b/cpan/CPAN/scripts/cpan @@ -1,5 +1,6 @@ #!/usr/local/bin/perl +BEGIN { pop @INC if $INC[-1] eq '.' } use strict; use vars qw($VERSION);
From: ppisar [...] redhat.com
Dne Út 18.říj.2016 08:26:29, ppisar napsal(a): Show quoted text
> Dne St 12.říj.2016 11:41:51, ppisar napsal(a):
> > Dne St 12.říj.2016 10:22:47, ppisar napsal(a):
> > > I can see this change in commit > > > 394ac06dc5e9e94a81c39c43135d1635f516422e. > > > > > > Unfortunately, the fix does not work because the $INC[-1] is not > > > "." > > > but an absolute path equivalent to "." when running the cpan tool.
> > > > What about the attached patch?
> > Looking at perl-v5.24.1-RC4, there is different fix that is missing
Actually the code on CPAN is missing more bits comparing the perl sources. The attached patch are all of them I found.
Subject: 0001-Fix-CVE-2016-1238-completely.patch
From 705b9f68906d584e2d0bf9c2fd634778f1ba9b35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= <ppisar@redhat.com> Date: Tue, 18 Oct 2016 14:35:09 +0200 Subject: [PATCH] Fix CVE-2016-1238 completely MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These are remains ported from perl-v5.24.1-RC4 commit: commit 5f66e9fffdc3d0c6e0846cd1f11298e70c786c30 Author: Tony Cook <tony@develop-help.com> Date: Tue Jun 21 10:02:02 2016 +1000 (perl #127834) remove . from the end of @INC if complex modules are loaded While currently Encode and Storable are know to attempt to load modules not included in the core, updates to other modules may lead to those also attempting to load new modules, so be safe and remove . for those as well. Signed-off-by: Petr Písař <ppisar@redhat.com> --- lib/CPAN.pm | 4 ++++ scripts/cpan | 1 + 2 files changed, 5 insertions(+) diff --git a/lib/CPAN.pm b/lib/CPAN.pm index 69cc7b8..ae66eaf 100644 --- a/lib/CPAN.pm +++ b/lib/CPAN.pm @@ -1128,6 +1128,8 @@ sub has_usable { ] }; if ($usable->{$mod}) { + local @INC = @INC; + pop @INC if $INC[-1] eq '.'; for my $c (0..$#{$usable->{$mod}}) { my $code = $usable->{$mod}[$c]; my $ret = eval { &$code() }; @@ -1170,6 +1172,8 @@ sub has_inst { $CPAN::META->{dontload_hash}{$mod}||=1; # unsafe meta access, ok return 0; } + local @INC = @INC; + pop @INC if $INC[-1] eq '.'; my $file = $mod; my $obj; $file =~ s|::|/|g; diff --git a/scripts/cpan b/scripts/cpan index 5555090..cceab30 100644 --- a/scripts/cpan +++ b/scripts/cpan @@ -1,5 +1,6 @@ #!/usr/local/bin/perl +BEGIN { pop @INC if $INC[-1] eq '.' } use strict; use vars qw($VERSION); -- 2.7.4
From: ppisar [...] redhat.com
Dne Út 18.říj.2016 09:19:36, ppisar napsal(a): Show quoted text
> Dne Út 18.říj.2016 08:26:29, ppisar napsal(a):
> > Looking at perl-v5.24.1-RC4, there is different fix that is missing
> > Actually the code on CPAN is missing more bits comparing the perl > sources. The attached patch are all of them I found.
After applying this patch, perl panics in Carp: $ perl -Ilib scripts/cpan -j t/97-lib_cpan1/CPAN/Config.pm -J panic: attempt to copy freed scalar 6b4d88 to 1ed9010 at /usr/share/perl5/Carp.pm line 229. The trigger is the BEGIN block in scripts/cpan. This looks like a perl problem with reference counting discussed in Carp <https://rt.cpan.org/Public/Bug/Display.html?id=72467> and perl <https://rt.perl.org/Public/Bug/Display.html?id=125907>.
From: ppisar [...] redhat.com
Dne Út 18.říj.2016 09:51:20, ppisar napsal(a): Show quoted text
> Dne Út 18.říj.2016 09:19:36, ppisar napsal(a):
> > Dne Út 18.říj.2016 08:26:29, ppisar napsal(a):
> > > Looking at perl-v5.24.1-RC4, there is different fix that is missing
> > > > Actually the code on CPAN is missing more bits comparing the perl > > sources. The attached patch are all of them I found.
> > After applying this patch, perl panics in Carp: > > $ perl -Ilib scripts/cpan -j t/97-lib_cpan1/CPAN/Config.pm -J > panic: attempt to copy freed scalar 6b4d88 to 1ed9010 at > /usr/share/perl5/Carp.pm line 229. > > The trigger is the BEGIN block in scripts/cpan. > > This looks like a perl problem with reference counting discussed in > Carp <https://rt.cpan.org/Public/Bug/Display.html?id=72467> and perl > <https://rt.perl.org/Public/Bug/Display.html?id=125907>.
After putting a work-around into Carp, the t/97-return_values.t fails because the `-j' option loads the configuration using: my $rc = eval "require '$file'"; That means it searches through @INC and there is no "." because of the CVE fix. perl tests do not fail because the t/97-return_values.t test is not there. A work-around is to change the test to start the -j arguments with "./". I'm not sure what's the semantics of the "-j" option. I did not expected it undergoes the @INC look-up. Then the code should be changed to threat -j argument as a path and make it absolute before passing it to require function. By the way the Carp panic can be triggered even with current CPAN code simply by passing non-existing file: $ perl -Ilib scripts/cpan -j foo.pm Loading internal null logger. Install Log::Log4perl for logging messages panic: attempt to copy freed scalar db74b0 to de6300 at /usr/share/perl5/vendor_perl/Carp.pm line 229.
Subject: Re: [rt.cpan.org #116507] CVE-2016-1238: App::Cpan may load an optional module from the current directory
Date: Tue, 18 Oct 2016 16:09:34 -0400
To: "bug-CPAN [...] rt.cpan.org" <bug-CPAN [...] rt.cpan.org>
From: brian d foy <brian.d.foy [...] gmail.com>
I've created a pull request for this: https://github.com/andk/cpanpm/pull/105 It's in my repo in the master branch: https://github.com/briandfoy/cpanpm at e96764e. I didn't get the panic on v5.24.0 on Mac OS X, so I'm curious if you still get it.
From: ppisar [...] redhat.com
Dne Út 18.říj.2016 16:10:28, brian.d.foy@gmail.com napsal(a): Show quoted text
> I've created a pull request for this: > https://github.com/andk/cpanpm/pull/105 > > It's in my repo in the master branch: > https://github.com/briandfoy/cpanpm at e96764e. I didn't get the > panic on v5.24.0 on Mac OS X, so I'm curious if you still get it.
I still get the panic with v5.24.0 on Linux (perl-5.24.0-378.fc26.x86_64 from Fedora). I don't think the panic is something CPAN can solve.
Marking as resolved, Thanks!