Provide -Dfortify_inc Configure option to remove . from @INC

p5pRT commented 8 years ago

Migrated from rt.perl.org#127810 (status was 'resolved')

Searchable as RT127810$

p5pRT commented 8 years ago

From @toddr

Created by @toddr

Several discussions have been had over the years about removing . from @INC.

In 2010\, Ansgar brought it up: http://www.nntp.perl.org/group/perl.perl5.porters/2010/08/msg162729.html In 2012\, I brought it up: http://code.activestate.com/lists/perl5-porters/176081/

My summary of the responses to these email chains would be:

1. A certain percentage of people do not agree that . in @INC is a security issue. Others feel it's "a basic sanity provision" 2. There is a general agreement that the Perl toolchain highly depends on this behavior so the toolchain would have to be fixed. 3. Some predicted disastrous consequences. 4. Many feel the problem is unfixable because of how long Perl has been this way.

I didn't quite make the Perl 5.18 deadline like I promised in the email\, but I now have a proposal complete with patches.

What I propose is a small patch to perl.c which causes . to be missing from @INC unless the environment variable PERL_USE_UNSAFE_INC=1 is present. This would only happen based on a Configure question which would default to being off so that the default Perl install does not change.

Cpanel currently ships and updates Perl 5.22 along with roughly 900 perl modules. In the coming version of our product\, we will be shipping a Perl that does not have . in @INC. These modules are all built as RPMs and I consider the RPMs a failed build if their unit tests cannot pass. There were about 3 of these 900 modules I had to do something weird with (because they were stripping %ENV or just being weird themselves). I did this by Simply adding PERL_USE_UNSAFE_INC=1 in the appropriate places to EU::MM\, M::B\, M::B::Tiny.

I am attaching the patches which will provide this option. I have updated no documentation yet. I can provide that if I can get some agreement for this to merge for 5.25.0 (I assume I've missed the 5.24 deadline for something like this?)

You can also find the commits here on github if you prefer to see them there: https://github.com/toddr/perl/compare/blead...toddr:pop_INC?diff=unified&expand=1&name=pop_INC

Once this merges\, it will provide an opportunity for me to begin providing patches to authors so that PERL_USE_UNSAFE_INC is for the most part unneeded.

Perl Info

``` Flags: category=core severity=medium Site configuration information for perl 5.22.1: Configured by cPanel at Wed Mar 2 15:47:40 CST 2016. Summary of my perl5 (revision 5 version 22 subversion 1) configuration: Platform: osname=linux, osvers=2.6.32-431.29.2.el6.i686, archname=i386-linux-64int uname='linux rpmb-32-centos-65.dev.cpanel.net 2.6.32-431.29.2.el6.i686 #1 smp tue sep 9 20:14:52 utc 2014 i686 i686 i386 gnulinux ' config_args='-des -Dusedevel -Darchname=i386-linux-64int -Dcc=/usr/bin/gcc -Dcpp=/usr/bin/cpp -DDEBUGGING=none -Doptimize=-Os -Dusemymalloc=n -Duseshrplib -Duselargefiles=yes -Duseposix=true -Dhint=recommended -Duseperlio=yes -Dccflags=-DPERL_DISABLE_PMC -I/usr/local/cpanel/3rdparty/perl/522/include -L/usr/local/cpanel/3rdparty/perl/522/lib -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib -Dcppflags=-I/usr/local/cpanel/3rdparty/perl/522/include -L/usr/local/cpanel/3rdparty/perl/522/lib -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib -Dldflags=-Wl,-rpath -Wl,/usr/local/cpanel/3rdparty/perl/522/lib -L/usr/local/cpanel/3rdparty/perl/522/lib -L/usr/local/cpanel/3rdparty/lib -Dprefix=/usr/local/cpanel/3rdparty/perl/522 -Dsiteprefix=/opt/cpanel/perl5/522 -Dsitebin=/opt/cpanel/perl5/522/bin -Dsitelib=/opt/cpanel/perl5/522/site_lib -Dusevendorprefix=true -Dvendorbin=/usr/local/cpanel/3rdparty/perl/522/bin -Dvendorprefix=/usr/local/cpanel/3rdparty/perl/522/lib/perl5 -Dvendorlib=/usr/local/cpanel/3rdparty/perl/522/lib/perl5/cpanel_lib -Dprivlib=/usr/local/cpanel/3rdparty/perl/522/lib/perl5/5.22.1 -Dman1dir=none -Dman3dir=none -Dscriptdir=/usr/local/cpanel/3rdparty/perl/522/bin -Dscriptdirexp=/usr/local/cpanel/3rdparty/perl/522/bin -Dsiteman1dir=none -Dsiteman3dir=none -Dinstallman1dir=none -Dversiononly=no -Dinstallusrbinperl=no -Dcf_by=cPanel -Dmyhostname=localhost -Dperladmin=root@localhost -Dcf_email=support@cpanel.net -Di_dbm=/usr/local/cpanel/3rdparty/include -Di_gdbm=/usr/local/cpanel/3rdparty/include -Di_ndbm=/usr/local/cpanel/3rdparty/include -DDB_File=true -Ud_dosuid -Uuserelocatableinc -Umad -Uusethreads -Uusemultiplicity -Uusesocks -Uuselongdouble -Aldflags=-L/usr/local/cpanel/3rdparty/perl/522/lib -L/usr/local/cpanel/3rdparty/lib -L/usr/lib -L/lib -lgdbm -Dlocincpth=/usr/local/cpanel/3rdparty/perl/522/include /usr/local/cpanel/3rdparty/include /usr/local/include -Duse64bitint -Uuse64bitall -Acflags=-fPIC -DPIC -m32 -I/usr/local/cpanel/3rdparty/perl/522/include -I/usr/local/cpanel/3rdparty/include -Dlibpth=/usr/local/cpanel/3rdparty/perl/522/lib /usr/local/cpanel/3rdparty/lib /usr/local/lib /lib /usr/lib ' hint=recommended, useposix=true, d_sigaction=define useithreads=undef, usemultiplicity=undef use64bitint=define, use64bitall=undef, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='/usr/bin/gcc', ccflags ='-DPERL_DISABLE_PMC -I/usr/local/cpanel/3rdparty/perl/522/include -L/usr/local/cpanel/3rdparty/perl/522/lib -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib -fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2', optimize='-Os', cppflags='-I/usr/local/cpanel/3rdparty/perl/522/include -L/usr/local/cpanel/3rdparty/perl/522/lib -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib -DPERL_DISABLE_PMC -I/usr/local/cpanel/3rdparty/perl/522/include -L/usr/local/cpanel/3rdparty/perl/522/lib -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib -fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include' ccversion='', gccversion='4.4.7 20120313 (Red Hat 4.4.7-4)', gccosandvers='' intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=12345678, doublekind=3 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12, longdblkind=3 ivtype='long long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=4, prototype=define Linker and Libraries: ld='/usr/bin/gcc', ldflags ='-Wl,-rpath -Wl,/usr/local/cpanel/3rdparty/perl/522/lib -L/usr/local/cpanel/3rdparty/perl/522/lib -L/usr/local/cpanel/3rdparty/lib -L/usr/local/cpanel/3rdparty/perl/522/lib -L/usr/local/cpanel/3rdparty/lib -L/usr/lib -L/lib -lgdbm -fstack-protector -L/usr/local/lib' libpth=/usr/local/cpanel/3rdparty/perl/522/lib /usr/local/cpanel/3rdparty/lib /usr/local/lib /lib /usr/lib /usr/local/lib /usr/lib libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.12.so, so=so, useshrplib=true, libperl=libperl.so gnulibc_version='2.12' Dynamic Linking: dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E -Wl,-rpath,/usr/local/cpanel/3rdparty/perl/522/lib/perl5/5.22.1/i386-linux-64int/CORE' cccdlflags='-fPIC', lddlflags='-shared -Os -L/usr/local/cpanel/3rdparty/perl/522/lib -L/usr/local/cpanel/3rdparty/lib -L/usr/lib -L/lib -L/usr/local/lib -fstack-protector' Locally applied patches: cPanel patches cPanel INC path changes Remove . from @INC @INC for perl 5.22.1: /usr/local/cpanel /usr/local/cpanel/3rdparty/perl/522/lib/perl5/cpanel_lib/i386-linux-64int /usr/local/cpanel/3rdparty/perl/522/lib/perl5/cpanel_lib /usr/local/cpanel/3rdparty/perl/522/lib/perl5/5.22.1/i386-linux-64int /usr/local/cpanel/3rdparty/perl/522/lib/perl5/5.22.1 /opt/cpanel/perl5/522/site_lib/i386-linux-64int /opt/cpanel/perl5/522/site_lib Environment for perl 5.22.1: HOME=/root LANG=en_US.UTF-8 LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/usr/local/cpanel/bin:/usr/local/cpanel/3rdparty/bin:/usr/local/cpanel/3rdparty/perl/522/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/cpanel/perl5/522/bin PERL_BADLANG (unset) SHELL=/bin/zsh ```

p5pRT commented 8 years ago

From @toddr

Patches for feature attached to RT.

p5pRT commented 8 years ago

From @toddr

0001-Patch-unit-tests-to-explicitly-insert-.-into-INC-whe.patch

```diff From c122e372fecc0e54621a3f69cc15707829537864 Mon Sep 17 00:00:00 2001 From: Todd Rinaldo Date: Thu, 31 Mar 2016 17:04:29 -0500 Subject: [PATCH 1/4] Patch unit tests to explicitly insert "." into @INC when needed. --- Porting/pod_rules.pl | 2 +- lib/strict.t | 2 +- lib/warnings.t | 2 +- makedef.pl | 2 +- regen.pl | 2 +- regen/ebcdic.pl | 3 +++ regen/genpacksizetables.pl | 2 +- regen/mg_vtable.pl | 2 +- t/comp/line_debug.t | 2 ++ t/lib/warnings/op | 1 + t/op/goto.t | 2 +- t/porting/regen.t | 2 +- t/re/pat.t | 4 ++-- t/run/runenv.t | 3 ++- t/run/switches.t | 6 +++--- t/test.pl | 4 ++-- 16 files changed, 24 insertions(+), 17 deletions(-) diff --git a/Porting/pod_rules.pl b/Porting/pod_rules.pl index 0d837bf..f4c094d 100644 --- a/Porting/pod_rules.pl +++ b/Porting/pod_rules.pl @@ -32,7 +32,7 @@ if (ord("A") == 193) { # plan9 => 'plan9/mkfile', ); -require 'Porting/pod_lib.pl'; +require './Porting/pod_lib.pl'; sub my_die; # process command-line switches diff --git a/lib/strict.t b/lib/strict.t index d6c6ed0..bfee762 100644 --- a/lib/strict.t +++ b/lib/strict.t @@ -1,7 +1,7 @@ #!./perl chdir 't' if -d 't'; -@INC = '../lib'; +@INC = ( '.', '../lib' ); our $local_tests = 6; require "../t/lib/common.pl"; diff --git a/lib/warnings.t b/lib/warnings.t index ee696fe..7c24f3a 100644 --- a/lib/warnings.t +++ b/lib/warnings.t @@ -1,7 +1,7 @@ #!./perl chdir 't' if -d 't'; -@INC = '../lib'; +@INC = ( '.', '../lib' ); our $UTF8 = (${^OPEN} || "") =~ /:utf8/; require "../t/lib/common.pl"; diff --git a/makedef.pl b/makedef.pl index 78ee0b1..5fd192e 100644 --- a/makedef.pl +++ b/makedef.pl @@ -70,7 +70,7 @@ BEGIN { } use constant PLATFORM => $ARGS{PLATFORM}; -require "$ARGS{TARG_DIR}regen/embed_lib.pl"; +require "./$ARGS{TARG_DIR}regen/embed_lib.pl"; # Is the following guard strictly necessary? Added during refactoring # to keep the same behaviour when merging other code into here. diff --git a/regen.pl b/regen.pl index 8788668..71a6eda 100644 --- a/regen.pl +++ b/regen.pl @@ -15,7 +15,7 @@ use strict; my $tap = $ARGV[0] && $ARGV[0] eq '--tap' ? '# ' : ''; foreach my $pl (map {chomp; "regen/$_"} ) { - my @command = ($^X, $pl, @ARGV); + my @command = ($^X, '-I.', $pl, @ARGV); print "$tap@command\n"; system @command and die "@command failed: $?" diff --git a/regen/ebcdic.pl b/regen/ebcdic.pl index fa8a051..718a7c8 100644 --- a/regen/ebcdic.pl +++ b/regen/ebcdic.pl @@ -1,6 +1,9 @@ use v5.16.0; use strict; use warnings; + +BEGIN { unshift @INC, '.' } + require 'regen/regen_lib.pl'; require 'regen/charset_translations.pl'; diff --git a/regen/genpacksizetables.pl b/regen/genpacksizetables.pl index 7a03dcd..d886822 100644 --- a/regen/genpacksizetables.pl +++ b/regen/genpacksizetables.pl @@ -3,7 +3,7 @@ # it will generate EBCDIC too. (TODO) use strict; use Encode; -require 'regen/regen_lib.pl'; +require './regen/regen_lib.pl'; sub make_text { my ($chrmap, $letter, $unpredictable, $nocsum, $size, $condition) = @_; diff --git a/regen/mg_vtable.pl b/regen/mg_vtable.pl index a05a7d4..342f5e0 100644 --- a/regen/mg_vtable.pl +++ b/regen/mg_vtable.pl @@ -20,7 +20,7 @@ require 5.004; BEGIN { # Get function prototypes - require 'regen/regen_lib.pl'; + require './regen/regen_lib.pl'; } my %mg = diff --git a/t/comp/line_debug.t b/t/comp/line_debug.t index 8361194..71626bb 100644 --- a/t/comp/line_debug.t +++ b/t/comp/line_debug.t @@ -1,5 +1,7 @@ #!./perl +BEGIN { unshift @INC, '.' } + chdir 't' if -d 't'; sub ok { diff --git a/t/lib/warnings/op b/t/lib/warnings/op index 8256c23..a87cbae 100644 --- a/t/lib/warnings/op +++ b/t/lib/warnings/op @@ -1435,6 +1435,7 @@ END { print "in end\n"; } print "in mainline\n"; 1; --FILE-- +BEGIN { unshift @INC, '.' } require abc; do "abc.pm"; EXPECT diff --git a/t/op/goto.t b/t/op/goto.t index aa2f24f..a65ede2 100644 --- a/t/op/goto.t +++ b/t/op/goto.t @@ -280,7 +280,7 @@ YYY: print "OK\n"; EOT close $f; -$r = runperl(prog => 'use Op_goto01; print qq[DONE\n]'); +$r = runperl(prog => 'BEGIN { unshift @INC, q[.] } use Op_goto01; print qq[DONE\n]'); is($r, "OK\nDONE\n", "goto within use-d file"); unlink_all "Op_goto01.pm"; diff --git a/t/porting/regen.t b/t/porting/regen.t index 5d08518..d234260 100644 --- a/t/porting/regen.t +++ b/t/porting/regen.t @@ -86,7 +86,7 @@ OUTER: foreach my $file (@files) { } foreach (@progs) { - my $command = "$^X $_ --tap"; + my $command = "$^X -I. $_ --tap"; system $command and die "Failed to run $command: $?"; } diff --git a/t/re/pat.t b/t/re/pat.t index 295a9f7..8652bf6 100644 --- a/t/re/pat.t +++ b/t/re/pat.t @@ -1663,7 +1663,7 @@ EOP # NOTE - Do not put quotes in the code! # NOTE - We have to triple escape the backref in the pattern below. my $code=' - BEGIN{require q(test.pl);} + BEGIN{require q(./test.pl);} watchdog(3); for my $len (1 .. 20) { my $eights= q(8) x $len; @@ -1679,7 +1679,7 @@ EOP # #123562] my $code=' - BEGIN{require q(test.pl);} + BEGIN{require q(./test.pl);} use Encode qw(_utf8_on); # \x80 and \x41 are continuation bytes in their respective # character sets diff --git a/t/run/runenv.t b/t/run/runenv.t index 82846a4..8861a3d 100644 --- a/t/run/runenv.t +++ b/t/run/runenv.t @@ -285,7 +285,8 @@ is ($err, '', 'No errors when determining @INC'); my @default_inc = split /\n/, $out; -is ($default_inc[-1], '.', '. is last in @INC'); +ok ! grep { $_ eq '.' } @default_inc, '. is not in @INC'; +#is ($default_inc[-1], '.', '. is last in @INC'); my $sep = $Config{path_sep}; foreach (['nothing', ''], diff --git a/t/run/switches.t b/t/run/switches.t index aa9bda3..5f03386 100644 --- a/t/run/switches.t +++ b/t/run/switches.t @@ -194,12 +194,12 @@ sub import { print map "<\$_>", \@_ } SWTESTPM close $f or die "Could not close: $!"; $r = runperl( - switches => [ "-M$package" ], + switches => [ "-I.", "-M$package" ], prog => '1', ); is( $r, "<$package>", '-M' ); $r = runperl( - switches => [ "-M$package=foo" ], + switches => [ "-I.", "-M$package=foo" ], prog => '1', ); is( $r, "<$package>", '-M with import parameter' ); @@ -213,7 +213,7 @@ SWTESTPM is( $r, '', '-m' ); } $r = runperl( - switches => [ "-m$package=foo,bar" ], + switches => [ "-I.", "-m$package=foo,bar" ], prog => '1', ); is( $r, "<$package>", '-m with import parameters' ); diff --git a/t/test.pl b/t/test.pl index 84475ea..9c35c2e 100644 --- a/t/test.pl +++ b/t/test.pl @@ -1244,8 +1244,8 @@ sub run_multiple_progs { close $fh or die "Cannot close $tmpfile: $!"; my $results = runperl( stderr => 1, progfile => $tmpfile, stdin => undef, $up - ? (switches => ["-I$up/lib", $switch], nolib => 1) - : (switches => [$switch]) + ? (switches => [ "-I.", "-I$up/lib", $switch], nolib => 1) + : (switches => [ "-I.", $switch]) ); my $status = $?; $results =~ s/\n+$//; -- 2.8.0 ```

p5pRT commented 8 years ago

From @toddr

0002-Add-PERL_USE_UNSAFE_INC-support-to-EU-MM-for-fortify.patch

```diff From 209aab49bf75216b4b63e77cf36a6764f3d34d85 Mon Sep 17 00:00:00 2001 From: Todd Rinaldo Date: Thu, 31 Mar 2016 17:04:42 -0500 Subject: [PATCH 2/4] Add PERL_USE_UNSAFE_INC support to EU::MM for fortify_inc support. This change allows the majority of Perl modules that cannot build/test/install without . in INC to be able to do so, while maintaining a safer perl under normal use. --- cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm | 4 ++-- cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker.pm | 5 +++++ t/porting/customized.dat | 4 ++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm b/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm index e24a61b..e2fd61c 100644 --- a/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm +++ b/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm @@ -3564,7 +3564,7 @@ PERL_DL_NONLAZY set for tests. sub test_via_harness { my($self, $perl, $tests) = @_; - return $self->SUPER::test_via_harness("PERL_DL_NONLAZY=1 $perl", $tests); + return $self->SUPER::test_via_harness("PERL_DL_NONLAZY=1 PERL_USE_UNSAFE_INC=1 $perl", $tests); } =item test_via_script (override) @@ -3575,7 +3575,7 @@ Again, the PERL_DL_NONLAZY thing. sub test_via_script { my($self, $perl, $script) = @_; - return $self->SUPER::test_via_script("PERL_DL_NONLAZY=1 $perl", $script); + return $self->SUPER::test_via_script("PERL_DL_NONLAZY=1 PERL_USE_UNSAFE_INC=1 $perl", $script); } diff --git a/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker.pm b/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker.pm index f9fb8fe..406d32b 100644 --- a/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker.pm +++ b/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker.pm @@ -5,6 +5,11 @@ use strict; BEGIN {require 5.006;} +# Assure anything called from Makefile.PL is allowed to have . in @INC. +BEGIN { + $ENV{PERL_USE_UNSAFE_INC} = 1; +} + require Exporter; use ExtUtils::MakeMaker::Config; use ExtUtils::MakeMaker::version; # ensure we always have our fake version.pm diff --git a/t/porting/customized.dat b/t/porting/customized.dat index c48ba7e..5f80d18 100644 --- a/t/porting/customized.dat +++ b/t/porting/customized.dat @@ -4,7 +4,7 @@ ExtUtils::Constant cpan/ExtUtils-Constant/t/Constant.t a0369c919e216fb02767a6376 ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/Command/MM.pm 8d772fbc6a57637ab24d12a02794073ee71b489c ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/Liblist.pm 9be9ac3fee6fd6df702469904e02c8b4c6f2502e ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/Liblist/Kid.pm bb2443c2314c50f09f7eab4aacc03ade8b9907dd -ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker.pm 830acdc810e2974d7fd4ec408ea1bfa825c75b69 +ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker.pm 50957b52a2e9e46851a2e7238a6dccd195ac824d ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker/Config.pm 5c41b40e33464c6635258061dff4ece018b46bd9 ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker/FAQ.pod 062e5d14a803fbbec8d61803086a3d7997e8a473 ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker/Tutorial.pod a8a9cab7d67922ed3d6883c864e1fe29aaa6ad89 @@ -23,7 +23,7 @@ ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_MacOS.pm 83601fa89eb ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_NW5.pm 8185a7db6c4d7e0fdc5001aeaa8c2b612a884a5e ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_OS2.pm 2fe66ca8a894d6a2ae340b8bf6f8d69c5e1f7fbe ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_QNX.pm e8a4dbba69a1d551bd581ea6a3f2415bacbc0ae5 -ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm d666ac424618c3e11b8549755c9646d942bd2d57 +ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm b31da2dae4f3c8769c5e9cb196cdaf6878e033cb ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_UWIN.pm f6581a0e75e45bfc26f343f173d3366c43fb1221 ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_VMS.pm 1997912b5018970cdeb3dae8fd7e0c24f6e5d567 ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_VOS.pm 210a4eda8b081d9986477e3a9762fce6ebea8474 -- 2.8.0 ```

p5pRT commented 8 years ago

From @toddr

0003-Set-PERL_USE_UNSAFE_INC-for-cpan-usage.patch

```diff From f22ff70044b523868bc1a79af08647b5cf434ebc Mon Sep 17 00:00:00 2001 From: Todd Rinaldo Date: Thu, 31 Mar 2016 17:04:47 -0500 Subject: [PATCH 3/4] Set PERL_USE_UNSAFE_INC for cpan usage This change allows the majority of Perl modules to build/test/install from the cpan client without having to modify them. --- cpan/CPAN/scripts/cpan | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cpan/CPAN/scripts/cpan b/cpan/CPAN/scripts/cpan index 5f4320e..d93b3bc 100644 --- a/cpan/CPAN/scripts/cpan +++ b/cpan/CPAN/scripts/cpan @@ -3,6 +3,11 @@ use strict; use vars qw($VERSION); +BEGIN { + # make sure we can install any modules from CPAN without patching them + $ENV{PERL_USE_UNSAFE_INC} = 1; +} + use App::Cpan '1.60_02'; $VERSION = '1.61'; -- 2.8.0 ```

p5pRT commented 8 years ago

From @toddr

0004-Provide-Dfortify_inc-Configure-option-to-remove-.-fr.patch

```diff From 4b8fb407013fa0b718978cbf3049ef17ab88d4aa Mon Sep 17 00:00:00 2001 From: Todd Rinaldo Date: Thu, 31 Mar 2016 17:04:53 -0500 Subject: [PATCH 4/4] Provide -Dfortify_inc Configure option to remove . from @INC This provides a Perl that removes . in the default @INC. Because the testing / make process for perl modules do not function well with . missing from @INC, Perl now supports the environment variable PERL_USE_UNSAFE_INC=1 which makes Perl behave as it prev iously did, returning . to @INC in all child processes. --- Configure | 26 ++++++++++++++++++++++++++ Makefile.SH | 2 +- config_h.SH | 7 +++++++ perl.c | 9 +++++++-- 4 files changed, 41 insertions(+), 3 deletions(-) diff --git a/Configure b/Configure index ff864b0..2f01eac 100755 --- a/Configure +++ b/Configure @@ -1384,6 +1384,8 @@ vendorscriptexp='' versiononly='' yacc='' yaccflags='' +fortify_inc='' + CONFIG='' : Detect odd OSs @@ -5102,6 +5104,29 @@ rp='What is the file extension used for shared libraries?' . ./myread so="$ans" +: Remove . from @INC +$cat << EOM + +Historically Perl has provided a final fallback of the current working +directory '.' when searching for a library. This, however, can lead to +problems when a Perl program which loads optional modules is called from +a shared directory. This can lead to executing unexpected code. + + +EOM + +case "$fortify_inc" in + $define|true|[yY]*) dflt="y";; + *) dflt='n';; +esac + +rp='Remove . from @INC unless $ENV{PERL_USE_UNSAFE_INC}=1 ?' +. ./myread +case "$ans" in + y*|define) fortify_inc="$define" ;; + *) fortify_inc="$undef" ;; +esac + : Does target system insist that shared library basenames are unique $cat << EOM @@ -25325,6 +25350,7 @@ vi='$vi' xlibpth='$xlibpth' yacc='$yacc' yaccflags='$yaccflags' +fortify_inc='$fortify_inc' zcat='$zcat' zip='$zip' EOT diff --git a/Makefile.SH b/Makefile.SH index e03a349..6323b57 100755 --- a/Makefile.SH +++ b/Makefile.SH @@ -345,7 +345,7 @@ RUN_PERL = \$(LDLIBPTH) \$(RUN) $perl\$(EXE_EXT) $spitshell >>$Makefile <$CONFIG_H -e 's!^#undef$.*/$\*!/\*#define\1 \*!' -e 's!^#un #$i_stdarg I_STDARG /**/ #$i_varargs I_VARARGS /**/ +/* FORTIFY_INC: + * This symbol, when defined, makes perl omit the '.' directory from + * @INC unless $ENV{PERL_USE_UNSAFE_INC}=1 is set when the interpreter starts. + */ +#$fortify_inc FORTIFY_INC /**/ + + /* PERL_INC_VERSION_LIST: * This variable specifies the list of subdirectories in over * which perl.c:incpush() and lib/lib.pm will automatically diff --git a/perl.c b/perl.c index 15e9150..3776256 100644 --- a/perl.c +++ b/perl.c @@ -4602,8 +4602,13 @@ S_init_perllib(pTHX) #endif #endif /* !PERL_IS_MINIPERL */ - if (!TAINTING_get) - S_incpush(aTHX_ STR_WITH_LEN("."), 0); + if (!TAINTING_get) { +#if !defined(PERL_IS_MINIPERL) && defined(FORTIFY_INC) + const char * const unsafe = PerlEnv_getenv("PERL_USE_UNSAFE_INC"); + if (unsafe && strEQ(unsafe, "1")) +#endif + S_incpush(aTHX_ STR_WITH_LEN("."), 0); + } } #if defined(DOSISH) || defined(__SYMBIAN32__) -- 2.8.0 ```

p5pRT commented 8 years ago

From [Unknown Contact. See original ticket]

Patches for feature attached to RT.

p5pRT commented 8 years ago

From @iabyn

On Thu\, Mar 31\, 2016 at 05:01:11PM -0700\, Todd Rinaldo wrote:

What I propose is a small patch to perl.c which causes . to be missing from @INC unless the environment variable PERL_USE_UNSAFE_INC=1 is present. This would only happen based on a Configure question which would default to being off so that the default Perl install does not change.

Cpanel currently ships and updates Perl 5.22 along with roughly 900 perl modules. In the coming version of our product\, we will be shipping a Perl that does not have . in @INC. These modules are all built as RPMs and I consider the RPMs a failed build if their unit tests cannot pass. There were about 3 of these 900 modules I had to do something weird with (because they were stripping %ENV or just being weird themselves). I did this by Simply adding PERL_USE_UNSAFE_INC=1 in the appropriate places to EU::MM\, M::B\, M::B::Tiny.

I am attaching the patches which will provide this option. I have updated no documentation yet. I can provide that if I can get some agreement for this to merge for 5.25.0 (I assume I've missed the 5.24 deadline for something like this?)

Yes\, 5.25.0 would be the earliest.

I have no objection in principle to something like this\, but haven't looked very closely at your patch yet. However I see that it needs more work. Building perl without -Dfortify_inc fails several tests. I've attached a patch that fixes one issue\, but there are still more to fix:

run/runenv.t (Wstat: 0 Tests: 104 Failed: 1) Failed test: 62 porting/checkcfgvar.t (Wstat: 0 Tests: 22 Failed: 11) Failed tests: 2\, 4\, 6\, 8\, 10\, 12\, 14\, 16\, 18\, 20\, 22 porting/cmp_version.t (Wstat: 0 Tests: 14 Failed: 2)

Also\, looking at run/runenv.t\, in the test in question the grep swallows the test description rather than providing it as an arg to is().

I'm not familiar enough with the config system to know whether the fortify_inc probe should be called d_fortify_inc or something instead\, I also wonder whether the define in config.h should be prefixed with PERL. 'FORTIFY_INC' is pretty generic and could conceivably clash with something else.

-- No man treats a motor car as foolishly as he treats another human being. When the car will not go\, he does not attribute its annoying behaviour to sin\, he does not say\, You are a wicked motorcar\, and I shall not give you any more petrol until you go. He attempts to find out what is wrong and set it right. -- Bertrand Russell\, Has Religion Made Useful Contributions to Civilization?

p5pRT commented 8 years ago

From @iabyn

0001-add-fortify_inc-to-unconfig.sh.patch

```diff From ab22ac7063d3c25182674e372e22c7acd1d609cc Mon Sep 17 00:00:00 2001 From: David Mitchell Date: Fri, 1 Apr 2016 15:11:57 +0100 Subject: [PATCH] add fortify_inc to unconfig.sh --- uconfig.h | 11 +++++++++-- uconfig.sh | 1 + 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/uconfig.h b/uconfig.h index cf3369d..e32c5e0 100644 --- a/uconfig.h +++ b/uconfig.h @@ -2955,6 +2955,13 @@ #define I_STDARG /**/ /*#define I_VARARGS / **/ +/* FORTIFY_INC: + * This symbol, when defined, makes perl omit the '.' directory from + * @INC unless {PERL_USE_UNSAFE_INC}=1 is set when the interpreter starts. + */ +/*#define FORTIFY_INC / **/ + + /* PERL_INC_VERSION_LIST: * This variable specifies the list of subdirectories in over * which perl.c:incpush() and lib/lib.pm will automatically @@ -5248,6 +5255,6 @@ #endif /* Generated from: - * dc6a0dd949dd1c707248914e2fdada06beb0e6193be5e94cb1423c6f050e65c3 config_h.SH - * ea0c70d2693a5911f8c16818794db0e782e9c4b40b9688a200dea7bcfcdfd820 uconfig.sh + * 848d7f737ef0b977f54b7fa31a106594a09fec9cf2e2285ab0ab990374a8d46b config_h.SH + * 2a6f0a9bc2ce421e971c7095f9641539f365ad63d9689645757987290427fe0a uconfig.sh * ex: set ro: */ diff --git a/uconfig.sh b/uconfig.sh index d0cb2de..1b773cb 100644 --- a/uconfig.sh +++ b/uconfig.sh @@ -900,4 +900,5 @@ vendorarchexp='' vendorlib_stem='' vendorlibexp='' versiononly='undef' +fortify_inc='undef' zip='' -- 2.4.3 ```

p5pRT commented 8 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 8 years ago

From @cpansprout

On Fri Apr 01 07:29:03 2016\, davem wrote:

I'm not familiar enough with the config system to know whether the fortify_inc probe should be called d_fortify_inc or something instead\,

Optionally\, one can side-step the whole issue by using -Accflags=-DFORTIFY_INC\, but that depends on how much you want this to be perceived as an option for ‘geeks’.

I also wonder whether the define in config.h should be prefixed with PERL. 'FORTIFY_INC' is pretty generic and could conceivably clash with something else.

PERL_FORTIFY_INC is definitely better.

--

Father Chrysostomos

p5pRT commented 8 years ago

From @tonycoz

On Thu Mar 31 17:01:11 2016\, TODDR wrote:

Several discussions have been had over the years about removing . from @INC.

In 2010\, Ansgar brought it up: http://www.nntp.perl.org/group/perl.perl5.porters/2010/08/msg162729.html In 2012\, I brought it up: http://code.activestate.com/lists/perl5-porters/176081/

My summary of the responses to these email chains would be:

1. A certain percentage of people do not agree that . in @INC is a security issue. Others feel it's "a basic sanity provision" 2. There is a general agreement that the Perl toolchain highly depends on this behavior so the toolchain would have to be fixed. 3. Some predicted disastrous consequences. 4. Many feel the problem is unfixable because of how long Perl has been this way.

I didn't quite make the Perl 5.18 deadline like I promised in the email\, but I now have a proposal complete with patches.

What I propose is a small patch to perl.c which causes . to be missing from @INC unless the environment variable PERL_USE_UNSAFE_INC=1 is present. This would only happen based on a Configure question which would default to being off so that the default Perl install does not change.

Cpanel currently ships and updates Perl 5.22 along with roughly 900 perl modules. In the coming version of our product\, we will be shipping a Perl that does not have . in @INC. These modules are all built as RPMs and I consider the RPMs a failed build if their unit tests cannot pass. There were about 3 of these 900 modules I had to do something weird with (because they were stripping %ENV or just being weird themselves). I did this by Simply adding PERL_USE_UNSAFE_INC=1 in the appropriate places to EU::MM\, M::B\, M::B::Tiny.

I am attaching the patches which will provide this option. I have updated no documentation yet. I can provide that if I can get some agreement for this to merge for 5.25.0 (I assume I've missed the 5.24 deadline for something like this?)

Is this intended to be a security measure?

I'm not sure how it can be if the user can set an environment variable to override it? (and in this case they can set PERL5LIB anyway).

An alternative might be a command-line option (like -T without the taint parts) to disable '.' in @INC.

Tony

p5pRT commented 8 years ago

From @toddr

On Fri Apr 01 14:24:46 2016\, sprout wrote:

On Fri Apr 01 07:29:03 2016\, davem wrote:

I'm not familiar enough with the config system to know whether the fortify_inc probe should be called d_fortify_inc or something instead\, I inquired about that a couple of weeks ago. I was informed there was no rhyme / reason / consistency to the use of d_ in Configure script variables. fortify_inc seems to be more intuitive so I went with that.

I also wonder whether the define in config.h should be prefixed with PERL. 'FORTIFY_INC' is pretty generic and could conceivably clash with something else.

PERL_FORTIFY_INC is definitely better.

Agreed. I'll update the patch on the github remote.

p5pRT commented 8 years ago

From @toddr

On Fri Apr 01 15:13:05 2016\, tonyc wrote:

Is this intended to be a security measure? Yes. When not doing development (which aside from toolchain was the other major argument for it being there always)\, I am asserting that it is a safer thing for . to not be in @INC by default.

I'm not sure how it can be if the user can set an environment variable to override it? (and in this case they can set PERL5LIB anyway). The goal here is not to deny . in INC. The goal is to provide a safer default so unexpected things happen less.

An alternative might be a command-line option (like -T without the taint parts) to disable '.' in @INC. Right but that would be the opposite of what I'm trying to achieve here. This whole thing could be argued as: "Just use taint"! The problem is that taint is easier said than done at a global level. It requires EVERY script be updated in order to take advantage of this.

The point of this change is to make the default behavior of a perl script not dependent on the current working directory. If the individual running the script wants the '.' restored to @INC\, they can use the environmental variable. If the script author wants this behavior\, they can trivially add the '.' to @INC in their code.

p5pRT commented 8 years ago

From @toddr

After some offline conversations\, I'm re-submitting this patch to remove . from @INC by default. A Configure variable will still be provided but it will be to put perl back in its old mode where . was at the end of @INC.

./Configure -Dunsafe_inc

There are 4 commits. Attaching them to the case appears to be a little noisy\, so for the time being\, you can track them in the pull request link I'm providing for reference.

https://github.com/toddr/perl/pull/1

or

https://github.com/toddr/perl.git branch pop_INC

p5pRT commented 8 years ago

From @tonycoz

On Mon\, Apr 11\, 2016 at 04:54:19PM -0700\, Todd Rinaldo via RT wrote:

There are 4 commits. Attaching them to the case appears to be a little noisy\, so for the time being\, you can track them in the pull request link I'm providing for reference.

Please attach them.

Tony

p5pRT commented 8 years ago

From @toddr

On Mon Apr 11 17:11:21 2016\, tonyc wrote:

On Mon\, Apr 11\, 2016 at 04:54:19PM -0700\, Todd Rinaldo via RT wrote:

There are 4 commits. Attaching them to the case appears to be a little noisy\, so for the time being\, you can track them in the pull request link I'm providing for reference.

Please attach them.

Tony

Attached.

p5pRT commented 8 years ago

From @toddr

0001-Patch-unit-tests-to-explicitly-insert-.-into-INC-whe.patch

```diff From c90c2af4aab19bb8a5c31edecfd147d8071da3a3 Mon Sep 17 00:00:00 2001 From: Todd Rinaldo Date: Thu, 31 Mar 2016 17:04:29 -0500 Subject: [PATCH 1/4] Patch unit tests to explicitly insert "." into @INC when needed. --- Makefile.SH | 2 +- Porting/pod_rules.pl | 2 +- TestInit.pm | 2 +- lib/strict.t | 2 +- lib/warnings.t | 2 +- makedef.pl | 2 +- regen.pl | 2 +- regen/ebcdic.pl | 3 +++ regen/genpacksizetables.pl | 2 +- regen/mg_vtable.pl | 2 +- t/comp/line_debug.t | 2 ++ t/lib/warnings/op | 1 + t/op/goto.t | 2 +- t/porting/regen.t | 2 +- t/re/pat.t | 6 +++--- t/run/runenv.t | 3 ++- t/run/switches.t | 6 +++--- t/test.pl | 3 ++- 18 files changed, 27 insertions(+), 19 deletions(-) diff --git a/Makefile.SH b/Makefile.SH index 916b332..8c5121c 100755 --- a/Makefile.SH +++ b/Makefile.SH @@ -345,7 +345,7 @@ RUN_PERL = \$(LDLIBPTH) \$(RUN) $perl\$(EXE_EXT) $spitshell >>$Makefile < 'plan9/mkfile', ); -require 'Porting/pod_lib.pl'; +require './Porting/pod_lib.pl'; sub my_die; # process command-line switches diff --git a/TestInit.pm b/TestInit.pm index f4ed6fd..f9a5e91 100644 --- a/TestInit.pm +++ b/TestInit.pm @@ -47,7 +47,7 @@ sub import { } elsif ($_ eq 'T') { $chdir = '..' unless -f 't/TEST' && -f 'MANIFEST' && -d 'lib' && -d 'ext'; - @INC = 'lib'; + @INC = qw/ lib . /; $setopt = 1; } else { die "Unknown option '$_'"; diff --git a/lib/strict.t b/lib/strict.t index d6c6ed0..bfee762 100644 --- a/lib/strict.t +++ b/lib/strict.t @@ -1,7 +1,7 @@ #!./perl chdir 't' if -d 't'; -@INC = '../lib'; +@INC = ( '.', '../lib' ); our $local_tests = 6; require "../t/lib/common.pl"; diff --git a/lib/warnings.t b/lib/warnings.t index ee696fe..7c24f3a 100644 --- a/lib/warnings.t +++ b/lib/warnings.t @@ -1,7 +1,7 @@ #!./perl chdir 't' if -d 't'; -@INC = '../lib'; +@INC = ( '.', '../lib' ); our $UTF8 = (${^OPEN} || "") =~ /:utf8/; require "../t/lib/common.pl"; diff --git a/makedef.pl b/makedef.pl index 104696c..c0a220a 100644 --- a/makedef.pl +++ b/makedef.pl @@ -70,7 +70,7 @@ BEGIN { } use constant PLATFORM => $ARGS{PLATFORM}; -require "$ARGS{TARG_DIR}regen/embed_lib.pl"; +require "./$ARGS{TARG_DIR}regen/embed_lib.pl"; # Is the following guard strictly necessary? Added during refactoring # to keep the same behaviour when merging other code into here. diff --git a/regen.pl b/regen.pl index 8788668..71a6eda 100644 --- a/regen.pl +++ b/regen.pl @@ -15,7 +15,7 @@ use strict; my $tap = $ARGV[0] && $ARGV[0] eq '--tap' ? '# ' : ''; foreach my $pl (map {chomp; "regen/$_"} ) { - my @command = ($^X, $pl, @ARGV); + my @command = ($^X, '-I.', $pl, @ARGV); print "$tap@command\n"; system @command and die "@command failed: $?" diff --git a/regen/ebcdic.pl b/regen/ebcdic.pl index fa8a051..718a7c8 100644 --- a/regen/ebcdic.pl +++ b/regen/ebcdic.pl @@ -1,6 +1,9 @@ use v5.16.0; use strict; use warnings; + +BEGIN { unshift @INC, '.' } + require 'regen/regen_lib.pl'; require 'regen/charset_translations.pl'; diff --git a/regen/genpacksizetables.pl b/regen/genpacksizetables.pl index 7a03dcd..d886822 100644 --- a/regen/genpacksizetables.pl +++ b/regen/genpacksizetables.pl @@ -3,7 +3,7 @@ # it will generate EBCDIC too. (TODO) use strict; use Encode; -require 'regen/regen_lib.pl'; +require './regen/regen_lib.pl'; sub make_text { my ($chrmap, $letter, $unpredictable, $nocsum, $size, $condition) = @_; diff --git a/regen/mg_vtable.pl b/regen/mg_vtable.pl index a05a7d4..342f5e0 100644 --- a/regen/mg_vtable.pl +++ b/regen/mg_vtable.pl @@ -20,7 +20,7 @@ require 5.004; BEGIN { # Get function prototypes - require 'regen/regen_lib.pl'; + require './regen/regen_lib.pl'; } my %mg = diff --git a/t/comp/line_debug.t b/t/comp/line_debug.t index 8361194..71626bb 100644 --- a/t/comp/line_debug.t +++ b/t/comp/line_debug.t @@ -1,5 +1,7 @@ #!./perl +BEGIN { unshift @INC, '.' } + chdir 't' if -d 't'; sub ok { diff --git a/t/lib/warnings/op b/t/lib/warnings/op index 528639e..df9dadb 100644 --- a/t/lib/warnings/op +++ b/t/lib/warnings/op @@ -1435,6 +1435,7 @@ END { print "in end\n"; } print "in mainline\n"; 1; --FILE-- +BEGIN { unshift @INC, '.' } require abc; do "abc.pm"; EXPECT diff --git a/t/op/goto.t b/t/op/goto.t index aa2f24f..a65ede2 100644 --- a/t/op/goto.t +++ b/t/op/goto.t @@ -280,7 +280,7 @@ YYY: print "OK\n"; EOT close $f; -$r = runperl(prog => 'use Op_goto01; print qq[DONE\n]'); +$r = runperl(prog => 'BEGIN { unshift @INC, q[.] } use Op_goto01; print qq[DONE\n]'); is($r, "OK\nDONE\n", "goto within use-d file"); unlink_all "Op_goto01.pm"; diff --git a/t/porting/regen.t b/t/porting/regen.t index 5d08518..d234260 100644 --- a/t/porting/regen.t +++ b/t/porting/regen.t @@ -86,7 +86,7 @@ OUTER: foreach my $file (@files) { } foreach (@progs) { - my $command = "$^X $_ --tap"; + my $command = "$^X -I. $_ --tap"; system $command and die "Failed to run $command: $?"; } diff --git a/t/re/pat.t b/t/re/pat.t index 295a9f7..ce9d324 100644 --- a/t/re/pat.t +++ b/t/re/pat.t @@ -1663,7 +1663,7 @@ EOP # NOTE - Do not put quotes in the code! # NOTE - We have to triple escape the backref in the pattern below. my $code=' - BEGIN{require q(test.pl);} + BEGIN{require q(./test.pl);} watchdog(3); for my $len (1 .. 20) { my $eights= q(8) x $len; @@ -1679,7 +1679,7 @@ EOP # #123562] my $code=' - BEGIN{require q(test.pl);} + BEGIN{require q(./test.pl);} use Encode qw(_utf8_on); # \x80 and \x41 are continuation bytes in their respective # character sets @@ -1747,7 +1747,7 @@ EOP my ($expr, $expect, $test_name, $cap1)= @$tuple; # avoid quotes in this code! my $code=' - BEGIN{require q(test.pl);} + BEGIN{require q(./test.pl);} watchdog(3); my $status= eval(q{ !(' . $expr . ') ? q(failed) : ' . ($cap1 ? '($1 ne q['.$cap1.']) ? qq(badmatch:$1) : ' : '') . diff --git a/t/run/runenv.t b/t/run/runenv.t index 82846a4..8861a3d 100644 --- a/t/run/runenv.t +++ b/t/run/runenv.t @@ -285,7 +285,8 @@ is ($err, '', 'No errors when determining @INC'); my @default_inc = split /\n/, $out; -is ($default_inc[-1], '.', '. is last in @INC'); +ok ! grep { $_ eq '.' } @default_inc, '. is not in @INC'; +#is ($default_inc[-1], '.', '. is last in @INC'); my $sep = $Config{path_sep}; foreach (['nothing', ''], diff --git a/t/run/switches.t b/t/run/switches.t index aa9bda3..5f03386 100644 --- a/t/run/switches.t +++ b/t/run/switches.t @@ -194,12 +194,12 @@ sub import { print map "<\$_>", \@_ } SWTESTPM close $f or die "Could not close: $!"; $r = runperl( - switches => [ "-M$package" ], + switches => [ "-I.", "-M$package" ], prog => '1', ); is( $r, "<$package>", '-M' ); $r = runperl( - switches => [ "-M$package=foo" ], + switches => [ "-I.", "-M$package=foo" ], prog => '1', ); is( $r, "<$package>", '-M with import parameter' ); @@ -213,7 +213,7 @@ SWTESTPM is( $r, '', '-m' ); } $r = runperl( - switches => [ "-m$package=foo,bar" ], + switches => [ "-I.", "-m$package=foo,bar" ], prog => '1', ); is( $r, "<$package>", '-m with import parameters' ); diff --git a/t/test.pl b/t/test.pl index 84475ea..991d8a6 100644 --- a/t/test.pl +++ b/t/test.pl @@ -646,7 +646,7 @@ sub _create_runperl { # Create the string to qx in runperl(). $runperl = "$ENV{PERL_RUNPERL_DEBUG} $runperl"; } unless ($args{nolib}) { - $runperl = $runperl . ' "-I../lib"'; # doublequotes because of VMS + $runperl = $runperl . ' "-I../lib" "-I." '; # doublequotes because of VMS } if ($args{switches}) { local $Level = 2; @@ -1235,6 +1235,7 @@ sub run_multiple_progs { open my $fh, '>', $tmpfile or die "Cannot open >$tmpfile: $!"; print $fh q{ BEGIN { + push @INC, '.'; open STDERR, '>&', STDOUT or die "Can't dup STDOUT->STDERR: $!;"; } -- 2.8.1 ```

p5pRT commented 8 years ago

From @toddr

0002-Add-PERL_USE_UNSAFE_INC-support-to-EU-MM-for-fortify.patch

```diff From 29b67a33c7f762f7c2352d4db4d6ff17995d77d5 Mon Sep 17 00:00:00 2001 From: Todd Rinaldo Date: Thu, 31 Mar 2016 17:04:42 -0500 Subject: [PATCH 2/4] Add PERL_USE_UNSAFE_INC support to EU::MM for fortify_inc support. This change allows the majority of Perl modules that cannot build/test/install without . in INC to be able to do so, while maintaining a safer perl under normal use. --- cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm | 6 +++--- cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker.pm | 7 ++++++- t/porting/customized.dat | 4 ++-- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm b/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm index e24a61b..85194ed 100644 --- a/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm +++ b/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm @@ -15,7 +15,7 @@ use ExtUtils::MakeMaker qw($Verbose neatvalue); # If we make $VERSION an our variable parse_version() breaks use vars qw($VERSION); -$VERSION = '7.10_01'; +$VERSION = '7.10_02'; $VERSION = eval $VERSION; ## no critic [BuiltinFunctions::ProhibitStringyEval] require ExtUtils::MM_Any; @@ -3564,7 +3564,7 @@ PERL_DL_NONLAZY set for tests. sub test_via_harness { my($self, $perl, $tests) = @_; - return $self->SUPER::test_via_harness("PERL_DL_NONLAZY=1 $perl", $tests); + return $self->SUPER::test_via_harness("PERL_DL_NONLAZY=1 PERL_USE_UNSAFE_INC=1 $perl", $tests); } =item test_via_script (override) @@ -3575,7 +3575,7 @@ Again, the PERL_DL_NONLAZY thing. sub test_via_script { my($self, $perl, $script) = @_; - return $self->SUPER::test_via_script("PERL_DL_NONLAZY=1 $perl", $script); + return $self->SUPER::test_via_script("PERL_DL_NONLAZY=1 PERL_USE_UNSAFE_INC=1 $perl", $script); } diff --git a/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker.pm b/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker.pm index f9fb8fe..686bebb 100644 --- a/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker.pm +++ b/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker.pm @@ -5,6 +5,11 @@ use strict; BEGIN {require 5.006;} +# Assure anything called from Makefile.PL is allowed to have . in @INC. +BEGIN { + $ENV{PERL_USE_UNSAFE_INC} = 1; +} + require Exporter; use ExtUtils::MakeMaker::Config; use ExtUtils::MakeMaker::version; # ensure we always have our fake version.pm @@ -24,7 +29,7 @@ my %Recognized_Att_Keys; our %macro_fsentity; # whether a macro is a filesystem name our %macro_dep; # whether a macro is a dependency -our $VERSION = '7.10_01'; +our $VERSION = '7.10_02'; $VERSION = eval $VERSION; ## no critic [BuiltinFunctions::ProhibitStringyEval] # Emulate something resembling CVS $Revision$ diff --git a/t/porting/customized.dat b/t/porting/customized.dat index f871a32..d4ff48e 100644 --- a/t/porting/customized.dat +++ b/t/porting/customized.dat @@ -4,7 +4,7 @@ ExtUtils::Constant cpan/ExtUtils-Constant/t/Constant.t a0369c919e216fb02767a6376 ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/Command/MM.pm 8d772fbc6a57637ab24d12a02794073ee71b489c ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/Liblist.pm 9be9ac3fee6fd6df702469904e02c8b4c6f2502e ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/Liblist/Kid.pm bb2443c2314c50f09f7eab4aacc03ade8b9907dd -ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker.pm 830acdc810e2974d7fd4ec408ea1bfa825c75b69 +ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker.pm 1d33d5891922dfd0b25b44712ea692b93598909b ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker/Config.pm 5c41b40e33464c6635258061dff4ece018b46bd9 ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker/FAQ.pod 062e5d14a803fbbec8d61803086a3d7997e8a473 ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker/Tutorial.pod a8a9cab7d67922ed3d6883c864e1fe29aaa6ad89 @@ -23,7 +23,7 @@ ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_MacOS.pm 83601fa89eb ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_NW5.pm 8185a7db6c4d7e0fdc5001aeaa8c2b612a884a5e ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_OS2.pm 2fe66ca8a894d6a2ae340b8bf6f8d69c5e1f7fbe ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_QNX.pm e8a4dbba69a1d551bd581ea6a3f2415bacbc0ae5 -ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm d666ac424618c3e11b8549755c9646d942bd2d57 +ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm 6944a58ce519707eb73a1ac3391ff1d8a329642d ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_UWIN.pm f6581a0e75e45bfc26f343f173d3366c43fb1221 ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_VMS.pm 1997912b5018970cdeb3dae8fd7e0c24f6e5d567 ExtUtils::MakeMaker cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_VOS.pm 210a4eda8b081d9986477e3a9762fce6ebea8474 -- 2.8.1 ```

p5pRT commented 8 years ago

From @toddr

0003-Set-PERL_USE_UNSAFE_INC-for-cpan-usage.patch

```diff From 918247a11f1a0989ef0f66a73c2992e1f4cbac4b Mon Sep 17 00:00:00 2001 From: Todd Rinaldo Date: Thu, 31 Mar 2016 17:04:47 -0500 Subject: [PATCH 3/4] Set PERL_USE_UNSAFE_INC for cpan usage This change allows the majority of Perl modules to build/test/install from the cpan client without having to modify them. --- cpan/CPAN/scripts/cpan | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cpan/CPAN/scripts/cpan b/cpan/CPAN/scripts/cpan index 5f4320e..d93b3bc 100644 --- a/cpan/CPAN/scripts/cpan +++ b/cpan/CPAN/scripts/cpan @@ -3,6 +3,11 @@ use strict; use vars qw($VERSION); +BEGIN { + # make sure we can install any modules from CPAN without patching them + $ENV{PERL_USE_UNSAFE_INC} = 1; +} + use App::Cpan '1.60_02'; $VERSION = '1.61'; -- 2.8.1 ```

p5pRT commented 8 years ago

From @toddr

0004-Remove-.-from-default-INC.patch

```diff From f712ba245f71688530553aa47471694565a3bb2f Mon Sep 17 00:00:00 2001 From: Todd Rinaldo Date: Thu, 31 Mar 2016 17:04:53 -0500 Subject: [PATCH 4/4] Remove "." from default @INC Perl now does not provide . in @INC by default. If you need this legacy feature, you can re-build with -Dunsafe_inc Because the testing / make process for perl modules do not function well with . missing from @INC, Perl now supports the environment variable PERL_USE_UNSAFE_INC=1 which makes Perl behave as it previously did, returning . to @INC in all child processes. Update unit tests and default value files to work with the new %Config variable "unsafe_inc" ExtUtils::MakeMaker was patched but has not yet been tested for windows. --- Configure | 28 ++++++++++++++++++++++++++++ Cross/config.sh-arm-linux | 1 + NetWare/config.wc | 1 + Porting/config.sh | 1 + config_h.SH | 6 ++++++ configure.com | 1 + perl.c | 9 +++++++-- plan9/config_sh.sample | 1 + regen/uconfig_h.pl | 2 +- symbian/config.sh | 1 + t/run/runenv.t | 9 +++++++-- uconfig.h | 10 ++++++++-- uconfig.sh | 1 + uconfig64.sh | 1 + win32/config.ce | 1 + win32/config.gc | 1 + win32/config.vc | 1 + 17 files changed, 68 insertions(+), 7 deletions(-) diff --git a/Configure b/Configure index ff864b0..f93cf6b 100755 --- a/Configure +++ b/Configure @@ -1384,6 +1384,8 @@ vendorscriptexp='' versiononly='' yacc='' yaccflags='' +unsafe_inc='' + CONFIG='' : Detect odd OSs @@ -5102,6 +5104,31 @@ rp='What is the file extension used for shared libraries?' . ./myread so="$ans" +: Include . in @INC +$cat << EOM + +Historically Perl has provided a final fallback of the current working +directory '.' when searching for a library. This, however, can lead to +problems when a Perl program which loads optional modules is called from +a shared directory. This can lead to executing unexpected code. + +Perl now no longer starts with '.' in @INC unless the environment variable +PERL_USE_UNSAFE_INC=1 is set. + +EOM + +case "$unsafe_inc" in + $define|true|[yY]*) dflt="y";; + *) dflt='n';; +esac + +rp='Provide '.' in @INC by default? ' +. ./myread +case "$ans" in + y*|define) unsafe_inc="$define" ;; + *) unsafe_inc="$undef" ;; +esac + : Does target system insist that shared library basenames are unique $cat << EOM @@ -25325,6 +25352,7 @@ vi='$vi' xlibpth='$xlibpth' yacc='$yacc' yaccflags='$yaccflags' +unsafe_inc='$unsafe_inc' zcat='$zcat' zip='$zip' EOT diff --git a/Cross/config.sh-arm-linux b/Cross/config.sh-arm-linux index 4857806..d52916b 100644 --- a/Cross/config.sh-arm-linux +++ b/Cross/config.sh-arm-linux @@ -1103,6 +1103,7 @@ uidsize='4' uidtype='uid_t' uname='uname' uniq='uniq' +unsafe_inc='' uquadtype='unsigned long long' use5005threads='undef' use64bitall='undef' diff --git a/NetWare/config.wc b/NetWare/config.wc index a06d89c..f6af454 100644 --- a/NetWare/config.wc +++ b/NetWare/config.wc @@ -1066,6 +1066,7 @@ uidsize='4' uidtype='uid_t' uname='uname' uniq='uniq' +unsafe_inc='' uquadtype='unsigned __int64' use5005threads='undef' use64bitall='undef' diff --git a/Porting/config.sh b/Porting/config.sh index aee9550..0ec31f3 100644 --- a/Porting/config.sh +++ b/Porting/config.sh @@ -1128,6 +1128,7 @@ uidsize='4' uidtype='uid_t' uname='uname' uniq='uniq' +unsafe_inc='' uquadtype='unsigned long long' use5005threads='undef' use64bitall='undef' diff --git a/config_h.SH b/config_h.SH index 6bd7c30..a1c98a4 100755 --- a/config_h.SH +++ b/config_h.SH @@ -2990,6 +2990,12 @@ sed <$CONFIG_H -e 's!^#undef$.*/$\*!/\*#define\1 \*!' -e 's!^#un #$i_stdarg I_STDARG /**/ #$i_varargs I_VARARGS /**/ +/* PERL_UNSAFE_INC_BY_DEFAULT: + * This symbol, when defined, causes @INC to include . as a final fallback. + */ +#$unsafe_inc PERL_UNSAFE_INC_BY_DEFAULT /**/ + + /* PERL_INC_VERSION_LIST: * This variable specifies the list of subdirectories in over * which perl.c:incpush() and lib/lib.pm will automatically diff --git a/configure.com b/configure.com index ffcbc22..3de173d 100644 --- a/configure.com +++ b/configure.com @@ -6777,6 +6777,7 @@ $ WC "u64size='" + u64size + "'" $ WC "u64type='" + u64type + "'" $ WC "u8size='" + u8size + "'" $ WC "u8type='" + u8type + "'" +$ WC "unsafe_inc=''" $ WC "uidformat='lu'" $ WC "uidsign='1'" $ WC "uidsize='4'" diff --git a/perl.c b/perl.c index 228a0d8..8a32a47 100644 --- a/perl.c +++ b/perl.c @@ -4606,8 +4606,13 @@ S_init_perllib(pTHX) #endif #endif /* !PERL_IS_MINIPERL */ - if (!TAINTING_get) - S_incpush(aTHX_ STR_WITH_LEN("."), 0); + if (!TAINTING_get) { +#if !defined(PERL_IS_MINIPERL) && !defined(PERL_UNSAFE_INC_BY_DEFAULT) + const char * const unsafe = PerlEnv_getenv("PERL_USE_UNSAFE_INC"); + if (unsafe && strEQ(unsafe, "1")) +#endif + S_incpush(aTHX_ STR_WITH_LEN("."), 0); + } } #if defined(DOSISH) || defined(__SYMBIAN32__) diff --git a/plan9/config_sh.sample b/plan9/config_sh.sample index fef2205..2dc9e79 100644 --- a/plan9/config_sh.sample +++ b/plan9/config_sh.sample @@ -1074,6 +1074,7 @@ uidsize='2' uidtype='uid_t' uname='uname' uniq='uniq' +unsafe_inc='' uquadtype='unsigned long long' use5005threads='undef' use64bitall='undef' diff --git a/regen/uconfig_h.pl b/regen/uconfig_h.pl index 99a74f1..1c3d1b2 100755 --- a/regen/uconfig_h.pl +++ b/regen/uconfig_h.pl @@ -10,7 +10,7 @@ use strict; use Config; -require 'regen/regen_lib.pl'; +require './regen/regen_lib.pl'; my ($uconfig_h, $uconfig_h_new, $config_h_sh) = ('uconfig.h', 'uconfig.h-new', 'config_h.SH'); diff --git a/symbian/config.sh b/symbian/config.sh index 48cb7a5..223162c 100644 --- a/symbian/config.sh +++ b/symbian/config.sh @@ -889,6 +889,7 @@ uidformat='"lu"' uidsign='1' uidsize='4' uidtype=int +unsafe_inc='' uquadtype='uint64_t' use5005threads='undef' use64bitall='undef' diff --git a/t/run/runenv.t b/t/run/runenv.t index 8861a3d..c8f7975 100644 --- a/t/run/runenv.t +++ b/t/run/runenv.t @@ -285,8 +285,13 @@ is ($err, '', 'No errors when determining @INC'); my @default_inc = split /\n/, $out; -ok ! grep { $_ eq '.' } @default_inc, '. is not in @INC'; -#is ($default_inc[-1], '.', '. is last in @INC'); +# Based on the unsafe_inc Configuration variable, we either do or don't expect . to be in the default @INC. +if ( $Config{unsafe_inc} && $Config{unsafe_inc} eq 'define' ) { + ok( ( grep { $_ eq '.' } @default_inc ), '. is in @INC' ); +} +else { + ok( ( !grep { $_ eq '.' } @default_inc ), '. is not in @INC' ); +} my $sep = $Config{path_sep}; foreach (['nothing', ''], diff --git a/uconfig.h b/uconfig.h index e14589e..b52c7ce 100644 --- a/uconfig.h +++ b/uconfig.h @@ -2955,6 +2955,12 @@ #define I_STDARG /**/ /*#define I_VARARGS / **/ +/* PERL_UNSAFE_INC_BY_DEFAULT: + * This symbol, when defined, causes @INC to include . as a final fallback. + */ +# PERL_UNSAFE_INC_BY_DEFAULT /**/ + + /* PERL_INC_VERSION_LIST: * This variable specifies the list of subdirectories in over * which perl.c:incpush() and lib/lib.pm will automatically @@ -5248,6 +5254,6 @@ #endif /* Generated from: - * dc6a0dd949dd1c707248914e2fdada06beb0e6193be5e94cb1423c6f050e65c3 config_h.SH - * fc611849cb5b1e14ec1687b255dac15414cc5e2e11b192d94e08136cfe277f75 uconfig.sh + * bf51571546f32a94d3b551ed5c839263a7d44664daa320efaf77cfde99da2c6a config_h.SH + * a45e3f02525748a692e593ed2ddff26f3d583c752839e98c6d273704c1881a19 uconfig.sh * ex: set ro: */ diff --git a/uconfig.sh b/uconfig.sh index 93a2ee3..a80e55d 100644 --- a/uconfig.sh +++ b/uconfig.sh @@ -856,6 +856,7 @@ uidformat='"lu"' uidsign='1' uidsize='4' uidtype=int +unsafe_inc='' uquadtype='uint64_t' use5005threads='undef' use64bitall='undef' diff --git a/uconfig64.sh b/uconfig64.sh index 0b12147..4e43657 100644 --- a/uconfig64.sh +++ b/uconfig64.sh @@ -857,6 +857,7 @@ uidformat='"u"' uidsign='1' uidsize='4' uidtype=int +unsafe_inc='' uquadtype='unsigned long' use5005threads='undef' use64bitall='define' diff --git a/win32/config.ce b/win32/config.ce index a5ee737..19dce09 100644 --- a/win32/config.ce +++ b/win32/config.ce @@ -1056,6 +1056,7 @@ uidsize='4' uidtype='uid_t' uname='uname' uniq='uniq' +unsafe_inc='' uquadtype='unsigned __int64' use5005threads='undef' use64bitall='undef' diff --git a/win32/config.gc b/win32/config.gc index e9cf4ed..356a482 100644 --- a/win32/config.gc +++ b/win32/config.gc @@ -1097,6 +1097,7 @@ uidsize='4' uidtype='uid_t' uname='uname' uniq='uniq' +unsafe_inc='' uquadtype='unsigned long long' use5005threads='undef' use64bitall='undef' diff --git a/win32/config.vc b/win32/config.vc index 2fc37b0..d4f16a6 100644 --- a/win32/config.vc +++ b/win32/config.vc @@ -1096,6 +1096,7 @@ uidsize='4' uidtype='uid_t' uname='uname' uniq='uniq' +unsafe_inc='' uquadtype='unsigned __int64' use5005threads='undef' use64bitall='undef' -- 2.8.1 ```

p5pRT commented 8 years ago

From @Leont

On Tue\, Apr 12\, 2016 at 2:15 AM\, Todd Rinaldo via RT \< perlbug-followup@perl.org> wrote:

On Mon Apr 11 17:11:21 2016\, tonyc wrote:

On Mon\, Apr 11\, 2016 at 04:54:19PM -0700\, Todd Rinaldo via RT wrote:

There are 4 commits. Attaching them to the case appears to be a little noisy\, so for the time being\, you can track them in the pull request link I'm providing for reference.

Please attach them.

Tony

Attached.

. is normally at the end of @INC\, so wouldn't it be more appropriate to push it instead of unshift it?

Leon

p5pRT commented 8 years ago

From @toddr

On Tue Apr 12 03:12:04 2016\, LeonT wrote:

On Tue\, Apr 12\, 2016 at 2:15 AM\, Todd Rinaldo via RT \< perlbug-followup@perl.org> wrote:

On Mon Apr 11 17:11:21 2016\, tonyc wrote:

On Mon\, Apr 11\, 2016 at 04:54:19PM -0700\, Todd Rinaldo via RT wrote:

There are 4 commits. Attaching them to the case appears to be a little noisy\, so for the time being\, you can track them in the pull request link I'm providing for reference.

Please attach them.

Tony

Attached.

. is normally at the end of @INC\, so wouldn't it be more appropriate to push it instead of unshift it?

Leon

The attached patches make it so . is exactly where it has always been when PERL_USE_UNSAFE_INC=1 is set.

There have been more offline conversations. I'm going to try to pull them into this public location so all are aware.

Q: We should hide the Configure option so people won't be able to compile perl with . in INC any more? (-Accflags=-DPERL_UNSAFE_INC) A: Sure you could and I don't feel strongly about it. But IMO\, we let people do much sillier things than this in Configure. I prefer to make the rope readily available to anyone who wants to hang themselves. I vote for leaving the option to put . back in @INC during Configure as long as it defaults to off.

Q: Couldn't we just use PERL5LIB? A: If you do so then you'll be putting "." in the first not the last spot so you'll end up chasing bugs that happen because the wrong libraries are chosen in the tool chain. You also risk toolchain code manipulating PERL5LIB and breaking this workaround.

Q: If people need to put . back in @INC like it was\, it should be a Perl command line option (--add-dot-to-inc-flag) or even -MDotInc A: If you take this approach\, you lose the advantage of the environment variable bleeding through to child processes unless you use PERL5OPT. This happens quite a bit in the tool chain\, so you'd have to end up fixing more stuff. Also I suspect that perl 5.8.8 will not take kindly to PERL5OPT="--add-dot-to-inc-flag".

Right now\, I've identified the following as work that has to be done outside perl once this merges. The needed patches are already staged in the issues.

1. 3 tickets to update (Makefile|Build).PL module installers to address PERL_USE_UNSAFE_INC: https://github.com/toddr/perl/issues?q=is%3Aissue+is%3Aopen+label%3AMakefile.PL

2. 3 tickets to fix the CPAN installers (cpanm/cpan/cpanplus) to make use of PERL_USE_UNSAFE_INC (just for good measure) https://github.com/toddr/perl/issues?q=is%3Aissue+is%3Aopen+label%3ACPAN-INSTALLERS

3. 3 tickets to address modules where PERL_USE_UNSAFE_INC doesn't blead through to all installer bits: https://github.com/toddr/perl/issues?q=is%3Aissue+is%3Aopen+label%3ACPAN

If we do not use PERL_USE_UNSAFE_INC=1\, it is my belief that we will end up with WAY more than 9 fixes to CPAN to get this in place. PERL_USE_UNSAFE_INC is also 100% backward compatible.

Todd

p5pRT commented 8 years ago

From @Leont

On Tue\, Apr 12\, 2016 at 7:24 PM\, Todd Rinaldo via RT \< perlbug-followup@perl.org> wrote:

Q: If people need to put . back in @INC like it was\, it should be a Perl command line option (--add-dot-to-inc-flag) or even -MDotInc A: If you take this approach\, you lose the advantage of the environment variable bleeding through to child processes unless you use PERL5OPT.

I don't see this as an advantage; I see it as an action at a distance and I generally don't like those. While they can be useful it should always be the second option\, just like how -I has PERL5LIB (though in this case I don't really see the advantage of a separate variable for this).

This happens quite a bit in the tool chain\, so you'd have to end up fixing

more stuff.

I'm not sure it really happens in so many places. It's mostly just running the Makefile.PL/Build.PL\, and running the tests. That's only two niches that need to be adapted\, essentially.

Not saying it's trivial\, but I don't think an environmental variable makes things less complicated than a command line argument.

Also I suspect that perl 5.8.8 will not take kindly to PERL5OPT="--add-dot-to-inc-flag".

"Doctor\, it hurts when I do this".

I can vaguely imagine workflows where this is an issue\, but that's the same sort of workflow where PERL5LIB falls on its face because perl-5.8.whatever can't load perl-5.22's modules.

Right now\, I've identified the following as work that has to be done outside perl once this merges. The needed patches are already staged in the issues.

1. 3 tickets to update (Makefile|Build).PL module installers to address PERL_USE_UNSAFE_INC:

https://github.com/toddr/perl/issues?q=is%3Aissue+is%3Aopen+label%3AMakefile.PL

The way these patches are currently written suggests to me they're intended for dealing with tests that assume . is in @INC. In MM these would be copied to blib/lib anyway\, for MB and MBT I suppose it would help if people put some testing library in a weird place. Adding it in Test::Harness/TAP::Harness would seem like a more obvious place to me though.

2. 3 tickets to fix the CPAN installers (cpanm/cpan/cpanplus) to make use of PERL_USE_UNSAFE_INC (just for good measure)

https://github.com/toddr/perl/issues?q=is%3Aissue+is%3Aopen+label%3ACPAN-INSTALLERS

Given that there are more than a thousand distributions (according to grep.cpan.me) on CPAN that do «use inc::\» I think it's fairly unavoidable to handle this here somehow; Module::Install is largely responsible for this. This is singlehandedly the one place where this change will hurt the most.

3. 3 tickets to address modules where PERL_USE_UNSAFE_INC doesn't blead through to all installer bits: https://github.com/toddr/perl/issues?q=is%3Aissue+is%3Aopen+label%3ACPAN

I suspect in these cases it's easier to add a BEGIN { push @INC\, '.' } to these script than to fiddle with the Makefile.PL.

Leon

p5pRT commented 8 years ago

From @ntyni

On Tue\, Apr 12\, 2016 at 10:24:04AM -0700\, Todd Rinaldo via RT wrote:

Q: We should hide the Configure option so people won't be able to compile perl with . in INC any more? (-Accflags=-DPERL_UNSAFE_INC) A: Sure you could and I don't feel strongly about it. But IMO\, we let people do much sillier things than this in Configure. I prefer to make the rope readily available to anyone who wants to hang themselves. I vote for leaving the option to put . back in @INC during Configure as long as it defaults to off.

Thanks very much for driving this.

So far\, this discussion seems to have been mostly about fixing toolchain issues. While those are of course a precondition for this\, I'd like to discuss the problem of potentially breaking user programs.

For people building perl themselves\, it seems fine to remove '.' from @INC by default and offer a Configure option (hidden or otherwise) to those who need it. However\, various distributions (GNU/Linux variants\, FreeBSD etc.) offer pre-built (binary) perl packages for a "system perl". The Configure option isn't much use in such cases. So once the distribution starts shipping perl with no '.' in @INC\, a system perl upgrade can break user scripts. Users then need to take action\, either by setting PERL_USE_UNSAFE_INC=1 in the script environment\, or otherwise fixing the actual issue.

It seems to me that deprecation warnings could be useful here\, so that distributors could turn them on for one release to notify users that things will need changing.

I'm thinking of something like

if @INC is unmodified and $ENV{PERL_USE_UNSAFE_INC} is not set and require() loads something under "." then warn "using unsafe @INC is deprecated and will break in the future"

I suppose this would need a tri-state Configure option or two separate ones\, but I'm sure such details are solvable.

It may be that I'm just too cautious about local breakage. Do people see something like this as feasible and/or desirable? -- Niko Tyni ntyni@debian.org

p5pRT commented 8 years ago

From @toddr

On Tue Apr 12 17:14:36 2016\, LeonT wrote:

On Tue\, Apr 12\, 2016 at 7:24 PM\, Todd Rinaldo via RT \< perlbug-followup@perl.org> wrote:

Q: If people need to put . back in @INC like it was\, it should be a Perl command line option (--add-dot-to-inc-flag) or even -MDotInc A: If you take this approach\, you lose the advantage of the environment variable bleeding through to child processes unless you use PERL5OPT.

I don't see this as an advantage; I see it as an action at a distance and I generally don't like those. While they can be useful it should always be the second option\, just like how -I has PERL5LIB (though in this case I don't really see the advantage of a separate variable for this).

PERL_USE_UNSAFE_INC is a shim to minimize the amount of CPAN installation processes that break immediately when we put the core perl patch in. It is the only approach I see that has little or no risk of side effects. I would envision following this up with a special smoker that would find and identify the CPAN modules that are leaning on . in @INC and submit patches to fix the modules.

This happens quite a bit in the tool chain\, so you'd have to end up fixing more stuff.

Not sure which stuff you mean. The patches I linked to were the only ones I seemed to need with my approach.

I'm not sure it really happens in so many places. It's mostly just running the Makefile.PL/Build.PL\, and running the tests. That's only two niches that need to be adapted\, essentially.

It happens in more places than you seem to think from what I've seen.

Not saying it's trivial\, but I don't think an environmental variable makes things less complicated than a command line argument.

Ok. I'm not sure what your proposal here is. When I proposed this years ago. One of the major objections was how much of the toolchain and CPAN modules we'd have to fix to make this work. It sounds like you're saying you want to take the pure approach of making everything break and then fixing it. Who's going to do that work? From what I've seen\, changes in perl that necessitate a small number of modules be fixed up end up being unachievable by the time we get to RC0. I suspect the pure approach would be an order of magnitude worse.

Also I suspect that perl 5.8.8 will not take kindly to PERL5OPT="--add-dot-to-inc-flag".

"Doctor\, it hurts when I do this".

I can vaguely imagine workflows where this is an issue\, but that's the same sort of workflow where PERL5LIB falls on its face because perl-5.8.whatever can't load perl-5.22's modules.

At this point\, the only proposal I've seen that I think has a chance to work without re-releasing 30% of CPAN immediately would be PERL_USE_UNSAFE_INC or something to inject . into PERL5LIB. I am very worried that PERL5LIB will have unexpected side effects since it puts . at the front of @INC\, not the end.

Right now\, I've identified the following as work that has to be done outside perl once this merges. The needed patches are already staged in the issues.

1. 3 tickets to update (Makefile|Build).PL module installers to address PERL_USE_UNSAFE_INC:

https://github.com/toddr/perl/issues?q=is%3Aissue+is%3Aopen+label%3AMakefile.PL

The way these patches are currently written suggests to me they're intended for dealing with tests that assume . is in @INC. In MM these would be copied to blib/lib anyway\, for MB and MBT I suppose it would help if people put some testing library in a weird place. Adding it in Test::Harness/TAP::Harness would seem like a more obvious place to me though.

They are not just for addressing unit tests. I initially tried patching *::Harness to fix this. It was insufficient to solve the problem.

Makefiles do silly things like generating files with external perl processes\, firing up servers for testing\, calling a secondary Makfile.PL from a Makefile\, etc. When I got to my 20th module that needed special patching beyond harness\, I abandoned the approach. You can't just assume unit tests are all that need a @INC fixup.

2. 3 tickets to fix the CPAN installers (cpanm/cpan/cpanplus) to make use of PERL_USE_UNSAFE_INC (just for good measure)

https://github.com/toddr/perl/issues?q=is%3Aissue+is%3Aopen+label%3ACPAN- INSTALLERS

Given that there are more than a thousand distributions (according to grep.cpan.me) on CPAN that do «use inc::\» I think it's fairly unavoidable to handle this here somehow; Module::Install is largely responsible for this. This is singlehandedly the one place where this change will hurt the most.

Yes. Though I really didn't need to mess with Module::Install modules at all with my approach.

If we were to immediately deprecate PERL_USE_UNSAFE_INC\, would this make you feel any more comfortable? We could then yank it out of perl 5.28. This would give people 2 years to fix their CPAN module\, etc so it would be able to work without the env var. I think that would be achievable.

Todd

p5pRT commented 8 years ago

From @ap

* Todd Rinaldo via RT \perlbug\-followup@perl\.org [2016-04-15 00:23]:

This would give people 2 years to fix their CPAN module\, etc so it would be able to work without the env var. I think that would be achievable.

Empirically speaking\, getting every distro that uses Module::Install re-released in 2 years is a pipe dream.

p5pRT commented 8 years ago

From @toddr

On Fri Apr 15 06:39:41 2016\, aristotle wrote:

* Todd Rinaldo via RT \perlbug\-followup@perl\.org [2016-04-15 00:23]:

This would give people 2 years to fix their CPAN module\, etc so it would be able to work without the env var. I think that would be achievable.

Empirically speaking\, getting every distro that uses Module::Install re-released in 2 years is a pipe dream.

My brief analysis is that if the CPAN clients did a perl -I. Makefile.PL\, this would solve Module::Install. It is not the same behavior since you're pusing . on the front\, but it would work. If we could move forward with this\, I would probably have to produce a "wall of todos (shame?)" that identified which modules failed build/test/install due to lack of support for a perl without . in INC.

Todd

p5pRT commented 8 years ago

From @toddr

More offline conversations...

1. Like others\, adding a new\, highly-specific environment variable\, PERL_USE_UNSAFE_INC\, makes me twitchy. How about instead: we add a new perl switch\, -J\, which is exactly the same as -I\, except that it *appends* to @INC rather than prepending. This sounds reasonably general-purpose and useful in its own right.

I'm twitchy about adding new args to Perl WAY more than I am to adding a new environment variable that gets otherwise silently ignored.

I'd rather we provide a module to do this if you really insist on it. (-Mdot)

The main issue is when setting this in places like MakeMaker and the cpan executable\, whether some parts of the build process invoked from there would want to run a sub-process using a different perl (and version) than that currently running? If so the child could croak with "Unrecognized switch: -J".

Agreed.

2) should perl also special-case Makefile.PL and Build.PL? That is to say\, if the name of the executable perl is about to execute looks like one of those two\, should it add back "."? Otherwise a lot of distributions will fail to build if done manually (rather than via cpan)\, e.g. $ perl Makefile.PL since things like 'use inc::Module::Install' in the script will fail.

Yes this is another way it could go wrong.

3) should the MakeMaker and cpan hacks only set the PERL_USE_UNSAFE_INC / PERL5OPT env var if $Config{inc_dot or whatever} has the right value? Thus a perl built with the fully-fascist option wouldn't be adding -J. to PERL5OPT or whatever.

If you take this approach then you end up with a development version of Perl and half of CPAN being broken. I thought we were trying to avoid this?

4) I'm a bit puzzled why PERL_USE_UNSAFE_INC is set in the cpan executable rather than in CPAN.pm?

Laziness\, Impatience\, Hubris. Pick any 3.

I think my original reason was that I wanted to address people who run cpan and then invoke a shell by doing "look Moo". This is also one of the places where everyone's alternatives to PERL_USE_UNSAFE_INC has issues. You're now expecting people who do the install by hand to know why perl Makefile.PL is hurling all over their terminal.

My fix wasn't elegant but allowed me to get my perl distro out with minimal effort. I would prefer an alternative patch with the environment variable in the correct place if someone can provide it.

5) I'm also a bit puzzled why MakeMaker sets PERL_USE_UNSAFE_INC=1 in MM_Unix.pm when it calls out to perl? Isn't this already covered by the $ENV{PERL_USE_UNSAFE_INC} = 1 in MakeMaker.pm?

Extra paranoia. No side effects. Maximum visibility that it's being done in the build logs.

Others seem keen to hide the unsafe_inc Configure option "a little deeper"\, e.g. make it done with -Accflags=-D... rather than as a Configure option. I don't yet have a strong opinion either way\, but I would be interested to hear arguments for and against.

If you hide it deeper then it's harder to interrogate %Config about what the current Perl behavior is. Your check would end up having to look like this and I suspect it wouldn't be the only var you'd have to check to be sure. Who are you trying to hide this option from? My impression is that Perl doesn't advocate gun control. Only gun education.

if ( $Config{cc_flags} && $Config{cc_flags} =~ m/-DPERL_UNSAFE_INC_BY_DEFAULT/ ) {

Should all scripts that come bundled with the perl core remove "." (if possible)? I think probably yes.

I have taken a look at an even more draconian approach to this patch where we simply remove . from INC and provide no specific environment variable to re-enable it. The uber curmudgeons of the community seem to prefer this. We'd then simply rely on PERL5LIB / -I. / PERL5OPT to accomplish what we need. The alternate approaches will ALL have side effects we will have to address. The patch to Perl to address all the places it requires a relative include path during build are significant. Getting this right will take MUCH more time than the PERL_USE_UNSAFE_INC approach. If everyone is behind the draconian approach\, I'm all for it\, but it will break many CPAN installs the second perl is released to CPAN with this change.

I'm all for discussing the alternatives but I am having trouble getting a sense of where people stand on my currently submitted patch set. The problem we're trying to solve has been acknowledged for years as an issue\, but overly complicated to fix. The solution I've provided is a manageable patch with minimal side effects. It comes at the cost of support for a new environment variable. I hate this too but I hate the status quo even more. The current solution is not ideal but if any of the existing solutions were workable\, we would have tried them already\, right?

If we take my patches as-is\, possibly with some minor fixups\, this would give us a way forward to piecemeal fix the rest of core perl incrementally so that it no longer has to depend on . in @INC. Any alternative requires a significant amount of up front re-work of perl core to even be able to build without support for . in INC.

I don't necessarily want to see this merged tomorrow but I do think it should happen soon so we have a full year to address the CPAN fall out.

How can we move forward on this conversation?

p5pRT commented 8 years ago

From @cpansprout

On Tue Apr 12 10:24:04 2016\, TODDR wrote:

There have been more offline conversations. I'm going to try to pull them into this public location so all are aware.

I have yet to see (maybe I missed it) what the reasoning was behind making it opt-out\, rather than opt-in. That happened at some point during the ‘offline conversations’ you mentioned.

(FWIW\, I depend heavily on having . in @INC in many web applications.)

--

Father Chrysostomos

p5pRT commented 8 years ago

From @jmdh

On Tue\, May 10\, 2016 at 12:51:51PM -0700\, Father Chrysostomos via RT wrote:

On Tue Apr 12 10:24:04 2016\, TODDR wrote:

There have been more offline conversations. I'm going to try to pull them into this public location so all are aware.

I have yet to see (maybe I missed it) what the reasoning was behind making it opt-out\, rather than opt-in. That happened at some point during the ‘offline conversations’ you mentioned.

(FWIW\, I depend heavily on having . in @INC in many web applications.)

Simply put\, it's a fail-safe design. If you want to sacrifice security for convenience\, you should have to explicitly specify this (and make sure you're otherwise playing safe through the application design).

FWIW\, Todd (and I haven't followed every last detail) I think the approach you've outlined is a fine one\, and I would happy to see it merged. If we (Debian) can help at all by providing mass-rebuild testing\, let us know - specifically\, we can probably provide a fair bit of automated test coverage for things not on CPAN but which are known to use perl.

On that note\, Niko Tyni's suggestion about how to announce/deprecate the previous behaviour in an earlier message on the ticket is still an open question. I think I come down on the side of arguing against it\, but mainly because it would delay the fix from actually landing until 5.28\, or Debian stable+2.

Cheers\, Dominic.

p5pRT commented 8 years ago

From @kentfredric

On 11 May 2016 at 07:17\, Todd Rinaldo via RT \perlbug\-followup@perl\.org wrote:

if $ $Config\{cc\_flags\} && $Config\{cc\_flags\} =~ m/\-DPERL\_UNSAFE\_INC\_BY\_DEFAULT/ $ \{
Should all scripts that come bundled with the perl core remove "." (if possible)? I think probably yes.

I feel like I want some intermediate stage\, where we can transition by slowing the bleeding:

Have a mechanism that permits loading from "."\, but warns when it happens. ( But only when the unmodified '.' target still exists ).

This would of course cause errors for anyone using `warnings FATAL => q[:all]` \, but they asked for this.

And the implementation details would hopefully be such that if you manually removed "." from @INC and then re-added it\, the warning would no longer occur.

I just don't know how easy it would be to implement such a scheme without horrible magic\, and without adding refs in @INC ( which have side effects of their own )

And of course\, there's code out there that will /traverse/ @INC manually and do path expansion manually\, and then load matching expanded paths manually\, and they will of course not be subject to this warning.

-- Kent

KENTNL - https://metacpan.org/author/KENTNL

p5pRT commented 8 years ago

From @toddr

On Tue May 10 13:16:33 2016\, dom wrote:

On that note\, Niko Tyni's suggestion about how to announce/deprecate the previous behaviour in an earlier message on the ticket is still an open question. I think I come down on the side of arguing against it\, but mainly because it would delay the fix from actually landing until 5.28\, or Debian stable+2.

I agree that a deprecation cycle is not worth it. I've seen quite a few deprecation cycles in other languages (php deprecation noise which people just patched off comes to mind). People are REALLY good at ignoring the deprecation messages until they are forced to fix it because it no longer works. In the mean time\, you're just going to be emitting warning noise and I suspect some of the warnings will be false positives depending on how people manipulate @INC.

I would instead argue for an education FAQ telling people how to fix the problem.

Q: My module doesn't build/test/install A: ...

Q: My perl script stopped working because I'm using local modules A: 'use FindBin; use lib "$FindBin::Bin/../lib";' is probably what you want. OR set PERL5LIB="." until you can get a proper fix.

p5pRT commented 8 years ago

From @toddr

On Tue May 10 12:51:50 2016\, sprout wrote:

On Tue Apr 12 10:24:04 2016\, TODDR wrote:

There have been more offline conversations. I'm going to try to pull them into this public location so all are aware.

I have yet to see (maybe I missed it) what the reasoning was behind making it opt-out\, rather than opt-in. That happened at some point during the ‘offline conversations’ you mentioned.

(FWIW\, I depend heavily on having . in @INC in many web applications.)

Are these options a possibility for you in your situation?

1. use FindBin; use lib "$FindBin::Bin/../lib"; 2. use lib "/some/specific/path"; 3. set PERL5LIB="." in httpd.conf (or equivalent)

p5pRT commented 8 years ago

From @cpansprout

On Tue May 10 14:08:06 2016\, TODDR wrote:

On Tue May 10 12:51:50 2016\, sprout wrote:

On Tue Apr 12 10:24:04 2016\, TODDR wrote:

There have been more offline conversations. I'm going to try to pull them into this public location so all are aware.

I have yet to see (maybe I missed it) what the reasoning was behind making it opt-out\, rather than opt-in. That happened at some point during the ‘offline conversations’ you mentioned.

(FWIW\, I depend heavily on having . in @INC in many web applications.)

Are these options a possibility for you in your situation?

1. use FindBin; use lib "$FindBin::Bin/../lib"; 2. use lib "/some/specific/path"; 3. set PERL5LIB="." in httpd.conf (or equivalent)

They are. The problem is not insurmountable. It’s just going to be a Big Pain. (The httpd.conf suggestion\, though\, is one I had not thought of. I might try that when the time comes. Thank you.)

--

Father Chrysostomos

p5pRT commented 8 years ago

From @kentfredric

On 11 May 2016 at 09:03\, Todd Rinaldo via RT \perlbug\-followup@perl\.org wrote:

People are REALLY good at ignoring the deprecation messages until they are forced to fix it because it no longer works. In the mean time\, you're just going to be emitting warning noise and I suspect some of the warnings will be false positives depending on how people manipulate @INC.

The difference between these 2 cases\, is with a warning\, there is an expectation that you received notice about the pending change\, and were given opportunity ( a whole year in fact )\, to either fix it yourself\, or have somebody report the bug and fix it for you.

The *latter* of these two is a significant driving force other languages simply don't have at the scale we have it.

Without the deprecation cycle\, you're just going "well\, your code broke\, too bad".

That's on my list of "anti-perl mentalities"\, and is contrary to Perl's Deprecation policy.

And for a minor change without significant impact\, I could see your point\, although I would still disagree.

For a major change like this\, a "no deprecation cycle" policy is foolhardy at best.

I'd rather no change than a change this big without a deprecation cycle.

-- Kent

KENTNL - https://metacpan.org/author/KENTNL

p5pRT commented 8 years ago

From @ilmari

Kent Fredric \kentfredric@gmail\.com writes:

I'd rather no change than a change this big without a deprecation cycle.

We all remember how much broke the last time we changed a single dot in a user-visible place.

-- "A disappointingly low fraction of the human race is\, at any given time\, on fire." - Stig Sandbeck Mathisen

p5pRT commented 8 years ago

From @ap

* Kent Fredric \kentfredric@gmail\.com [2016-05-10 22:41]:

I just don't know how easy it would be to implement such a scheme without horrible magic\, and without adding refs in @INC ( which have side effects of their own )

It seems to me that it’s possible to do this minimally invasively:

Just cast get-magic on $INC[-1] when @INC is populated at interpreter startup\, and have the magic set some flag. Then when pp_require iterates over @INC\, have it reset the flag just before fetching an element\, then copy the flag to a local variable immediately after fetch\, then later\, if it succeeds in loading a file\, look at the copy of the flag and throw a deprecation warning if it’s set.

That should ensure that the warning is only ever thrown in exactly those cases where a module is loaded through the . from the default @INC path\, without no false positives ever\, and I think no false negatives either.

(The point of the copy-the-flag dance is to make the check re-entrant – since `require` might be invoked by the code in the file being loaded.)

I am tempted to make a guess in a concrete number of lines for how small this patch would be.

And of course\, there's code out there that will /traverse/ @INC manually and do path expansion manually\, and then load matching expanded paths manually\, and they will of course not be subject to this warning.

It’s debatable whether they even ought to be subject to it.

I’ve written my fair share of “what modules does this perl have?”-type code that goes through @INC\, and if that simply silently reflects what perl is willing to load then I consider that correct\, and a warning thrown in that case would be useless to me.

Of course *some* stuff iterates @INC because it actually does emulate `require`. But I don’t see how that can be caught with any reasonable degree of effort. You would need a contagion that spreads from $INC[-1] to any string constructed from it to any filehandle opened from that to any data read from there\, until finally `eval` checks for that. Bottom line is you need a special new form of tainting which affects a dozen opcodes or more.

I am NOT tempted to make a guess in a concrete number of lines for how excessive that patch would be…

Given the S/N ratio and the level of complexity: not worth it.

Regards\, -- Aristotle Pagaltzis // \<http://plasmasturm.org/>

p5pRT commented 7 years ago

From @toddr

Now that CVE2016-1238 is public I am bumping this ticket.

Documentation can be found at: Initial disclosure: http://code.activestate.com/lists/perl5-porters/231168/ Security RT: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=127834

Given these security issues I re-submit the idea that: 1. . be removed from INC by default unless -Dfortify_inc is passed on command line to Configure. I am for there being a Config.pm variable to specify that this was done or chosen. We have much sillier options and hiding this in the bowels just makes it hard for maintainers who intentionally want this behavior to put it back.

2. If the environment variable PERL_USE_UNSAFE_INC=1 is set\, then . will be pushed back into @INC as it was in versions of Perl prior to now. This will allow the perl toolchain to maintain its functionality with only minor patches until we can address the toolchain dependance on . in a more reliable way.

3. There were some questions about where I applied patches some of the dual life tool chain modules. I'm open to tweaking those\, but I'd honestly prefer these tweaks to happen outside of the original submission.

This patch obsoletes the patches we just applied to 5.22 and 5.24. However it will break some small percentage of CPAN modules. I'd like to get this into blead as soon as possible so I can start addressing those issues with CPAN authors as soon as possible so that 5.26.0 goes off as smoothly as possible.

So I guess what remains is:

Given the nature of CVE-2016-1238\, does anyone object in principle to the patches as submitted?

Does anyone have any major objections to how I've addressed the toolchain for now. If you do\, do you have a workable alternative patch?

Thanks\, Todd

p5pRT commented 7 years ago

From @shadowcat-mst

I think\, on the whole\, a slightly more conservative approach might be more fruitful.

Something like:

1) Apply a patch now that provides an option to remove '.' from @INC\, but with the option disabled by default

2) Have some of us build perls with the option turned on and smoke ALL the things to figure out just how much fun this isn't going to be (because it's already clear that inc::latest\, inc::Module::Install\, t::lib and various other things that get used a bunch are going to die horribly).

3) Try and get the various toolchain stuff as robust against the various issues as possible\, then repeat step 2 until things start to look sane(r)

4) Then\, N blead releases onwards\, switch the default to 'enabled'\, and see how many BBC reports we get in spite of our efforts in steps 2 and 3

5) Make a final choice for the next 5.even.0 release before the 'disruptive changes' lockdown based on the information from 4 and whatever downstream contacts we can scrape up to express opinions

6) (hopefully) Profit.

-- Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue

http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/

Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN commercial support\, training and consultancy packages could help your team.

p5pRT commented 7 years ago

From @toddr

On Tue Jul 26 14:31:23 2016\, mst@shadowcat.co.uk wrote:

1) Apply a patch now that provides an option to remove '.' from @INC\, but with the option disabled by default

I'm up for this. It was the originally submitted patch actually.

2) Have some of us build perls with the option turned on and smoke ALL the things to figure out just how much fun this isn't going to be (because it's already clear that inc::latest\, inc::Module::Install\, t::lib and various other things that get used a bunch are going to die horribly).

Possibly. Based on offline conversations\, I think maybe we need to start a separate thread about how how the Perl repo needs something that smokes some experimental branch against ALL of CPAN and then reports it but maybe not to CPAN testers? I'll send an email shortly about it.

4) Then\, N blead releases onwards\, switch the default to 'enabled'\, and see how many BBC reports we get in spite of our efforts in steps 2 and 3

Yep changing the default later would be minor. This backs the idea of using a Configure option that some were against. IMO we have all sorts of Configure options there to shoot yourself in the foot with. I don't see why this would be different.

So there is 1 unaddressed problem in your approach I think.

Perl does not build well without PERL_USE_UNSAFE_INC=1 set. I *THINK* this means I still have to modify EU::MM and Test::Harness to get perl to compile. I will experiment to see how minimal I can make that today. However\, I suspect this will increase the size of my submission significantly. My original goal was to make the patch small since I didn't have much buy in but I think that's changed.

Todd

I will submit a re-based patch later today with my proposal based on ALL (one :) ) of the responses so far.

Todd

p5pRT commented 7 years ago

From @craigberry

On Wed\, Aug 3\, 2016 at 9:49 AM\, Todd Rinaldo via RT \perlbug\-followup@perl\.org wrote:

I will submit a re-based patch later today with my proposal based on ALL (one :) ) of the responses so far.

That sounds like a good plan to me. My only (late) suggestion would be to avoid words like "safe" and "fortify" in the names of Configure and environment variables that describe what we're doing to @INC. While I appreciate the intention of communicating that these things are security-related\, we can't actually guarantee there will never be other\, unrelated security problems with @INC that will need to be patched. What do we do then\, call the new thing extra-fortified\, double-safe @INC and explain to people that even though they have enabled safe @INC\, their @INC isn't actually safe? Remember safe.pm or safe signals as examples of things where the name promised more than could actually be delivered. If only we had called "safe" signals "coalesced" signals and documented that they provide a measure of safety against *one* of the things that can go wrong with signals\, I think promises and expectations would have aligned better.

Along those lines\, my suggestion would be to describe what we're actually doing to @INC and call it -Dinc_adds_dot_dir or -Dinc_omits_dot_dir. I think I favor the latter because $Config{inc_omits_dot_dir} will be false for every perl that has existed until now\, will continue to be false as long as we configure it to be undef (the initial default)\, and then will be true when we configure to refrain from adding '.' to @INC (the eventual default).

I guess this is bikeshedding. Sorry. Todd\, if this becomes controversial\, please ignore me and Just Do It with whatever name you want\, but as we've recently seen with base.pm\, setting more precise expectations can be important.

p5pRT commented 7 years ago

From @xsawyerx

On 08/03/2016 06:23 PM\, Craig A. Berry wrote:

On Wed\, Aug 3\, 2016 at 9:49 AM\, Todd Rinaldo via RT \perlbug\-followup@perl\.org wrote:

I will submit a re-based patch later today with my proposal based on ALL (one :) ) of the responses so far. [...] Along those lines\, my suggestion would be to describe what we're actually doing to @INC and call it -Dinc_adds_dot_dir or -Dinc_omits_dot_dir. I think I favor the latter because $Config{inc_omits_dot_dir} will be false for every perl that has existed until now\, will continue to be false as long as we configure it to be undef (the initial default)\, and then will be true when we configure to refrain from adding '.' to @INC (the eventual default).

+1 from me.

I'll add -Dabsolute_inc/-Dabsolute_inc or -Dcurdir_inc/-Dno_curdir_inc\, but I'll go with your suggestions just the same.

p5pRT commented 7 years ago

From @tonycoz

On Wed Aug 03 07:49:10 2016\, TODDR wrote:

I will submit a re-based patch later today with my proposal based on ALL (one :) ) of the responses so far.

Did you make any progress on this?

Tony

p5pRT commented 7 years ago

From @toddr

On Sun Aug 07 18:22:28 2016\, tonyc wrote:

On Wed Aug 03 07:49:10 2016\, TODDR wrote:

I will submit a re-based patch later today with my proposal based on ALL (one :) ) of the responses so far.

Did you make any progress on this?

It turns out it's not a good idea to promise commits in the middle of a vacation. I'm working on it now.

p5pRT commented 7 years ago

From @toddr

On Wed Aug 03 09:24:23 2016\, craig.a.berry@gmail.com wrote:

On Wed\, Aug 3\, 2016 at 9:49 AM\, Todd Rinaldo via RT \perlbug\-followup@perl\.org wrote:

I will submit a re-based patch later today with my proposal based on ALL (one :) ) of the responses so far.

That sounds like a good plan to me. My only (late) suggestion would be to avoid words like "safe" and "fortify" in the names of Configure and environment variables that describe what we're doing to @INC. While I appreciate the intention of communicating that these things are security-related\, we can't actually guarantee there will never be other\, unrelated security problems with @INC that will need to be patched. What do we do then\, call the new thing extra-fortified\, double-safe @INC and explain to people that even though they have enabled safe @INC\, their @INC isn't actually safe? Remember safe.pm or safe signals as examples of things where the name promised more than could actually be delivered. If only we had called "safe" signals "coalesced" signals and documented that they provide a measure of safety against *one* of the things that can go wrong with signals\, I think promises and expectations would have aligned better.

Along those lines\, my suggestion would be to describe what we're actually doing to @INC and call it -Dinc_adds_dot_dir or -Dinc_omits_dot_dir. I think I favor the latter because $Config{inc_omits_dot_dir} will be false for every perl that has existed until now\, will continue to be false as long as we configure it to be undef (the initial default)\, and then will be true when we configure to refrain from adding '.' to @INC (the eventual default).

I guess this is bikeshedding. Sorry. Todd\, if this becomes controversial\, please ignore me and Just Do It with whatever name you want\, but as we've recently seen with base.pm\, setting more precise expectations can be important.

Honestly this is what I thought I'd have to discuss when I originally submitted. I made my env var and build var a little inflammatory with the hopes of a better idea. So thank you for your thoughts.

I would tend towards -DNO_DOT_INC. I need it to be a negative since we're defaulting to "." being present by default in the initial patch.

I notice you didn't mention $ENV{PERL_USE_UNSAFE_INC} given the discussion is now going down the path of not supporting the environment variable at all once we get to 5.26\, I'm going to leave it as-is for now\, especially since Debian and cperl are already using that environment variable.

Todd

p5pRT commented 7 years ago

From @cpansprout

On Tue Aug 09 09:46:43 2016\, TODDR wrote:

I would tend towards -DNO_DOT_INC.

Since we are shedding bikes\, I think -DNO_DOT_IN_INC and -NO_INC_DOT read better in English.

--

Father Chrysostomos

p5pRT commented 7 years ago

From @craigberry

On Tue\, Aug 9\, 2016 at 1:34 PM\, Father Chrysostomos via RT \perlbug\-followup@perl\.org wrote:

On Tue Aug 09 09:46:43 2016\, TODDR wrote:

I would tend towards -DNO_DOT_INC.

Since we are shedding bikes\, I think -DNO_DOT_IN_INC and -NO_INC_DOT read better in English.

But say something that isn't necessarily true. We don't know whether there is a dot in @INC. We just know whether or not we put it there by default. Thus my suggestion of using a verb (omits or adds) that describes what what we are doing rather than making a claim about the state of @INC\, which may change under user action. Unless I missed something important\, we are not forbidding C\<use lib '.'>. If we at some point do so\, then that would be another option\, maybe -Dinc_forbids_dot_dir.

p5pRT commented 7 years ago

From @craigberry

On Tue\, Aug 9\, 2016 at 11:46 AM\, Todd Rinaldo via RT \perlbug\-followup@perl\.org wrote:

On Wed Aug 03 09:24:23 2016\, craig.a.berry@gmail.com wrote:

On Wed\, Aug 3\, 2016 at 9:49 AM\, Todd Rinaldo via RT \perlbug\-followup@perl\.org wrote:

I will submit a re-based patch later today with my proposal based on ALL (one :) ) of the responses so far.

That sounds like a good plan to me. My only (late) suggestion would be to avoid words like "safe" and "fortify" in the names of Configure and environment variables that describe what we're doing to @INC. While I appreciate the intention of communicating that these things are security-related\, we can't actually guarantee there will never be other\, unrelated security problems with @INC that will need to be patched. What do we do then\, call the new thing extra-fortified\, double-safe @INC and explain to people that even though they have enabled safe @INC\, their @INC isn't actually safe? Remember safe.pm or safe signals as examples of things where the name promised more than could actually be delivered. If only we had called "safe" signals "coalesced" signals and documented that they provide a measure of safety against *one* of the things that can go wrong with signals\, I think promises and expectations would have aligned better.

Along those lines\, my suggestion would be to describe what we're actually doing to @INC and call it -Dinc_adds_dot_dir or -Dinc_omits_dot_dir. I think I favor the latter because $Config{inc_omits_dot_dir} will be false for every perl that has existed until now\, will continue to be false as long as we configure it to be undef (the initial default)\, and then will be true when we configure to refrain from adding '.' to @INC (the eventual default).

I guess this is bikeshedding. Sorry. Todd\, if this becomes controversial\, please ignore me and Just Do It with whatever name you want\, but as we've recently seen with base.pm\, setting more precise expectations can be important.

Honestly this is what I thought I'd have to discuss when I originally submitted. I made my env var and build var a little inflammatory with the hopes of a better idea. So thank you for your thoughts.

I would tend towards -DNO_DOT_INC. I need it to be a negative since we're defaulting to "." being present by default in the initial patch.

I notice you didn't mention $ENV{PERL_USE_UNSAFE_INC} given the discussion is now going down the path of not supporting the environment variable at all once we get to 5.26\, I'm going to leave it as-is for now\, especially since Debian and cperl are already using that environment variable.

Oops. I replied to FC's reply before your message appeared on the list. Short version: -DNO_DOT_INC sounds (to me) like we guarantee no dot in @INC\, but we actually only refrain from putting it there by default. I was assuming the environment variable name would be aligned with the Configure variable\, but if it is disappearing before 5.26\, I agree it doesn't really matter that it gets changed.

p5pRT commented 7 years ago

From @kentfredric

On 10 August 2016 at 08:58\, Craig A. Berry \craig\.a\.berry@gmail\.com wrote:

Short version: -DNO_DOT_INC sounds (to me) like we guarantee no dot in @INC\, but we actually only refrain from putting it there by default.

Agree. -DNO_DEFAULT_INC_DOT sounds more explanatory to me.

-- Kent

KENTNL - https://metacpan.org/author/KENTNL

Perl / perl5