From perl-5.8.0@ton.iguana.be

Created by perl-5.8.0@ton.iguana.be

I was experimenting with an event-driven setup emulating
multiple concurrent CGI programs. To make the environments look
more "normal", I was doing local *ENV=$ref_to_the_current_cgi_env. And
as usual I used the -T option to protect myself from carelessness.
However, when I did a careless external call, I didn't get an error.
The following demonstrates the essence​:

perl -Twle '%a=(a=>4, b=>5); *ENV=\%a; system("echo unsafe")'
unsafe

while I expected what happens for this code​:
perl -Twle 'system("echo unsafe")'
Insecure $ENV{PATH} while running with -T switch at -e line 1.

So the system() safety checks are done against %ENV instead of
against the real environment. But the actual path search will
happen against the real environment. That's insecure.

Flags:
    category=core
    severity=medium

Site configuration information for perl v5.8.0:

Configured by ton at Tue Nov 12 01:56:18 CET 2002.

Summary of my perl5 (revision 5.0 version 8 subversion 0) configuration:
  Platform:
    osname=linux, osvers=2.4.19, archname=i686-linux-thread-multi-64int-ld
    uname='linux quasar 2.4.19 #5 wed oct 2 02:34:25 cest 2002 i686 unknown '
    config_args=''
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=define use5005threads=undef useithreads=define usemultiplicity=define
    useperlio=define d_sfio=undef uselargefiles=define usesocks=undef
    use64bitint=define use64bitall=undef uselongdouble=define
    usemymalloc=y, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2 -fomit-frame-pointer',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -I/usr/local/include'
    ccversion='', gccversion='2.95.3 20010315 (release)', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long long', ivsize=8, nvtype='long double', nvsize=12, Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lnsl -lndbm -ldb -ldl -lm -lpthread -lc -lposix -lcrypt -lutil
    perllibs=-lnsl -ldl -lm -lpthread -lc -lposix -lcrypt -lutil
    libc=/lib/libc-2.2.4.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.2.4'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-rdynamic'
    cccdlflags='-fpic', lddlflags='-shared -L/usr/local/lib'

Locally applied patches:
    


@INC for perl v5.8.0:
    /usr/lib/perl5/5.8.0/i686-linux-thread-multi-64int-ld
    /usr/lib/perl5/5.8.0
    /usr/lib/perl5/site_perl/5.8.0/i686-linux-thread-multi-64int-ld
    /usr/lib/perl5/site_perl/5.8.0
    /usr/lib/perl5/site_perl
    .


Environment for perl v5.8.0:
    HOME=/home/ton
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/ton/bin.Linux:/home/ton/bin:/home/ton/bin.SampleSetup:/usr/local/bin:/usr/local/sbin:/usr/local/jre/bin:/home/oracle/product/9.0.1/bin:/usr/local/ar/bin:/usr/games/bin:/usr/X11R6/bin:/usr/share/bin:/usr/bin:/usr/sbin:/bin:/sbin:.
    PERL_BADLANG (unset)
    SHELL=/bin/bash

From @hvds

"perl-5.8.0@​ton.iguana.be (via RT)" <perlbug-followup@​perl.org> wrote​:
:I was experimenting with an event-driven setup emulating
:multiple concurrent CGI programs. To make the environments look
:more "normal", I was doing local *ENV=$ref_to_the_current_cgi_env. And
:as usual I used the -T option to protect myself from carelessness.
:However, when I did a careless external call, I didn't get an error.
:The following demonstrates the essence​:
:
:perl -Twle '%a=(a=>4, b=>5); *ENV=\%a; system("echo unsafe")'
:unsafe
:
:while I expected what happens for this code​:
:perl -Twle 'system("echo unsafe")'
:Insecure $ENV{PATH} while running with -T switch at -e line 1.
:
:So the system() safety checks are done against %ENV instead of
:against the real environment. But the actual path search will
:happen against the real environment. That's insecure.

Ouch. I note that also if the new %ENV includes a PATH I get a
coredump instead​:

perl -Twe '%a=(PATH=>"util"); *ENV=\%a; system("echo unsafe")'
Segmentation fault (core dumped)
zen%
#0 Perl_mg_find (sv=0x814dee0, type=101) at mg.c​:327
#1 0x081129f3 in Perl_taint_env () at taint.c​:116
#2 0x080f96a7 in Perl_pp_system () at pp_sys.c​:4076
[..]

I believe this is happening because mg_find() assumes it will never
be called with an SV that isn't magical, and taint_env() assumes
that anything pulled out of %ENV will always have magic attached.

The main question I think is what C< *ENV = \%myenv > should mean​:
either it should act to replace the environment or not, and the
rest of the core should act appropriately either way. I think in
principle it would be most useful for it to replace the environment,
but I accept that that involves a lot of extra code and could also
make C< *ENV = \%myenv > unexpectedly slow. This implies that eg
C< local %ENV = ( PATH => $path ) > should also work.

(Hmm, can we even tell? C< *ENV = \*A; *A = \%myenv > may make
things tricky.)

Hugo

From @rgs

hv@​crypt.org wrote​:

The main question I think is what C< *ENV = \%myenv > should mean​:
either it should act to replace the environment or not, and the
rest of the core should act appropriately either way. I think in
principle it would be most useful for it to replace the environment,
but I accept that that involves a lot of extra code and could also
make C< *ENV = \%myenv > unexpectedly slow. This implies that eg
C< local %ENV = ( PATH => $path ) > should also work.

I recently documented in perlsub that C<local *foo> doesn't preserve
magic on $foo, @​foo, %foo etc. (perlsub/"Localization of globs").
This feature can be useful when doing C<local *_> in order to
localize $_ _and_ drop all previous magic on $_ that might get in
the way. In other words, C<local %ENV> allows to modify the environment,
while C<local *ENV> does not.

Considering this, I think that a glob assignment to *ENV should drop
the env magic from it, and that the core must be fixed to deal with
it gracefully. That's more consistent with the rest of perl (and the
rest of magical variables.)

From @hvds

hv@​crypt.org wrote​:
:This implies that eg :C< local %ENV = ( PATH => $path ) > should also work.

Oops, I should have tried it with a non-relative path​: it works fine.

Hugo

From perl5-porters@ton.iguana.be

In article <20031026130818.430969_4.rgarciasuarez@​_ree._r>,
  Rafael Garcia-Suarez <rgarciasuarez@​free.fr> writes​:

I recently documented in perlsub that C<local *foo> doesn't preserve
magic on $foo, @​foo, %foo etc. (perlsub/"Localization of globs").
This feature can be useful when doing C<local *_> in order to
localize $_ _and_ drop all previous magic on $_ that might get in
the way. In other words, C<local %ENV> allows to modify the environment,
while C<local *ENV> does not.

Considering this, I think that a glob assignment to *ENV should drop
the env magic from it, and that the core must be fixed to deal with
it gracefully. That's more consistent with the rest of perl (and the
rest of magical variables.)

What I had expected to happen is that perl internally has a reference
to the "real" magic env hash, and that my local *ENV = $hash_ref indeed
would replace the the HASH slot in *ENV with my new reference, but that
the real magic env keeps pointing to the old place. Then system/exec/qx
taint checks would still be done against against this preserved "real"
magic env. That would be enough to fix the security hole. The fact that
it's maybe unexpected that the real env isn't changed by local *ENV=$ref,
well, you can expect weird things if you write such weird code.

In my particular application I don't actually want all these setenv's to
happen on every little event. The only reason I set %ENV at all is to
make old CGI code happy. The only time I want the real env to change in
this application is when I'm indeed doing an external call, and I'm
prepared to do that "by hand" if needed.
--
void russian_roulette(void) { char *target; strcpy(target, "bullet"); }

From @rgs

hv@​crypt.org wrote​:

Ouch. I note that also if the new %ENV includes a PATH I get a
coredump instead​:

perl -Twe '%a=(PATH=>"util"); *ENV=\%a; system("echo unsafe")'
Segmentation fault (core dumped)
zen%
#0 Perl_mg_find (sv=0x814dee0, type=101) at mg.c​:327
#1 0x081129f3 in Perl_taint_env () at taint.c​:116
#2 0x080f96a7 in Perl_pp_system () at pp_sys.c​:4076
[..]

I believe this is happening because mg_find() assumes it will never
be called with an SV that isn't magical, and taint_env() assumes
that anything pulled out of %ENV will always have magic attached.

That's correct, and I fixed the coredump case by change #21542.
$ENV{PATH} is still checked for taintedness when %ENV is aliased to
another hash, although this might be a bit futile in this situation.

The main question I think is what C< *ENV = \%myenv > should mean​:
either it should act to replace the environment or not, and the
rest of the core should act appropriately either way. I think in
principle it would be most useful for it to replace the environment,
but I accept that that involves a lot of extra code and could also
make C< *ENV = \%myenv > unexpectedly slow. This implies that eg
C< local %ENV = ( PATH => $path ) > should also work.

(Hmm, can we even tell? C< *ENV = \*A; *A = \%myenv > may make
things tricky.)

Maybe do nothing and let people shoot in their feet.

Maybe just forbid aliasing *ENV at all. (with the collateral damage
on $ENV etc.) (or couldn't this chained alias thing be solved
by looking at the GvEGV ?)

From perl5-porters@ton.iguana.be

In article <20031027002524.7b444942.rgarciasuarez@​_ree._r>,
  Rafael Garcia-Suarez <rgarciasuarez@​free.fr> writes​:

Maybe do nothing and let people shoot in their feet.

Maybe just forbid aliasing *ENV at all. (with the collateral damage
on $ENV etc.) (or couldn't this chained alias thing be solved
by looking at the GvEGV ?)

Crashing, dieing, compile erors etc. are all solutions (though
certainly not my preferred solutions), but doing nothing isn't
acceptable since it's a potential security hole.

(my expectation and preferred solution was already mentioned in my
previous message)

From @nwc10

On Sun, Oct 26, 2003 at 11​:31​:42PM +0000, Ton Hospel wrote​:

In article <20031027002524.7b444942.rgarciasuarez@​_ree._r>,
Rafael Garcia-Suarez <rgarciasuarez@​free.fr> writes​:

Maybe do nothing and let people shoot in their feet.

Maybe just forbid aliasing *ENV at all. (with the collateral damage
on $ENV etc.) (or couldn't this chained alias thing be solved
by looking at the GvEGV ?)

Crashing, dieing, compile erors etc. are all solutions (though
certainly not my preferred solutions), but doing nothing isn't
acceptable since it's a potential security hole.

SEGVs better than continuing? I can't agree.

Nicholas Clark

From @rgs

Ton Hospel wrote​:

In article <20031027002524.7b444942.rgarciasuarez@​_ree._r>,
Rafael Garcia-Suarez <rgarciasuarez@​free.fr> writes​:

Maybe do nothing and let people shoot in their feet.

Maybe just forbid aliasing *ENV at all. (with the collateral damage
on $ENV etc.) (or couldn't this chained alias thing be solved
by looking at the GvEGV ?)

Crashing, dieing, compile erors etc. are all solutions (though
certainly not my preferred solutions), but doing nothing isn't
acceptable since it's a potential security hole.

I just fixed the coredump case. It was very data-dependent and didn't
always occur ; most of the time it only corrupted memory.

(my expectation and preferred solution was already mentioned in my
previous message)

I don't like special-casing "local *ENV" over all other "local *symbol".

Other proposal : in TAINT_ENV, the routine that checks for a tainted
environment, croak() if %ENV hasn't environment-magic, or if the hash
slot of the *ENV glob is empty. This modifies only the behaviour of perl
with -T.

From @rgs

I proposed :

Other proposal : in TAINT_ENV, the routine that checks for a tainted
environment, croak() if %ENV hasn't environment-magic, or if the hash
slot of the *ENV glob is empty. This modifies only the behaviour of perl
with -T.

With the patch below, I have these results :

  $ ./perl -Twle '%a=(a=>4, b=>5); *ENV=\%a; system("echo unsafe")'
  %ENV is aliased to another variable while running with -T switch at -e line 1.

  $ ./perl -Twle '%a=(a=>4, b=>5); *ENV=*a; system("echo unsafe")'
  %ENV is aliased to %a while running with -T switch at -e line 1.

i.e. when doing a TAINT_ENV check, immediately croak if %ENV is no
longer referring to the environment. This early failure approach
solves the security problem.

The behaviour of perl is not changed when not in -T or -t mode : i.e.
aliasing the *ENV glob works like regular glob aliasing.

If no objections or improvement proposals, I'll commit this.

Index​: pod/perldiag.pod

--- pod/perldiag.pod (revision 2730)
+++ pod/perldiag.pod (working copy)
@​@​ -1387,6 +1387,12 @​@​
(F) While under the C<use filetest> pragma, switching the real and
effective uids or gids failed.

+=item %ENV is aliased to %s
+
+(F) You're running under taint mode, and the C<%ENV> variable has been
+aliased to another hash, so it doesn't reflect anymore the state of the
+program's environment. This is potentially insecure.
+
=item Error converting file specification %s

(F) An error peculiar to VMS. Because Perl may have to deal with file
Index​: t/op/taint.t

--- t/op/taint.t (revision 2744)
+++ t/op/taint.t (working copy)
@​@​ -984,13 +984,12 @​@​
}

{
- # test with a non-magical %ENV (and non-magical %ENV elements)
- our %nonmagicalenv = ( PATH => $TAINT );
+ # [perl #24291] this used to dump core
+ our %nonmagicalenv = ( PATH => "util" );
  local *ENV = \%nonmagicalenv;
  eval { system("lskdfj"); };
- test 207, $@​ =~ /Insecure \$ENV{PATH} while running with -T switch/;
- # [perl #24291] this used to dump core
- %nonmagicalenv = ( PATH => "util" );
+ test 207, $@​ =~ /^%ENV is aliased to another variable while running with -T switch/;
+ local *ENV = *nonmagicalenv;
  eval { system("lskdfj"); };
- test 208, 1;
+ test 208, $@​ =~ /^%ENV is aliased to %nonmagicalenv while running with -T switch/;
}
Index​: taint.c

--- taint.c (revision 2744)
+++ taint.c (working copy)
@​@​ -80,9 +80,25 @​@​
  NULL
  };

- /* Don't bother if there's no %ENV hash */
- if (!PL_envgv || !GvHV(PL_envgv))
+ /* Don't bother if there's no *ENV glob */
+ if (!PL_envgv)
  return;
+ /* If there's no %ENV hash of if it's not magical, croak, because
+ * it probably doesn't reflect the actual environment */
+ if (!GvHV(PL_envgv) || !(SvRMAGICAL(GvHV(PL_envgv))
+ && mg_find((SV*)GvHV(PL_envgv), PERL_MAGIC_env))) {
+ bool was_tainted = PL_tainted;
+ char *name = GvENAME(PL_envgv);
+ PL_tainted = TRUE;
+ if (strEQ(name,"ENV"))
+ /* hash alias */
+ taint_proper("%%ENV is aliased to %s%s", "another variable");
+ else
+ /* glob alias​: report it in the error message */
+ taint_proper("%%ENV is aliased to %%%s%s", name);
+ /* this statement is reached under -t or -U */
+ PL_tainted = was_tainted;
+ }

#ifdef VMS
  {
@​@​ -99,9 +115,7 @​@​
  TAINT;
  taint_proper("Insecure %s%s", "$ENV{DCL$PATH}");
  }
- if (SvMAGICAL(*svp)
- && (mg = mg_find(*svp, PERL_MAGIC_envelem))
- && MgTAINTEDDIR(mg)) {
+ if ((mg = mg_find(*svp, PERL_MAGIC_envelem)) && MgTAINTEDDIR(mg)) {
  TAINT;
  taint_proper("Insecure directory in %s%s", "$ENV{DCL$PATH}");
  }
@​@​ -116,9 +130,7 @​@​
  TAINT;
  taint_proper("Insecure %s%s", "$ENV{PATH}");
  }
- if (SvMAGICAL(*svp)
- && (mg = mg_find(*svp, PERL_MAGIC_envelem))
- && MgTAINTEDDIR(mg)) {
+ if ((mg = mg_find(*svp, PERL_MAGIC_envelem)) && MgTAINTEDDIR(mg)) {
  TAINT;
  taint_proper("Insecure directory in %s%s", "$ENV{PATH}");
  }

From perl5-porters@ton.iguana.be

In article <20031027115225.7c21a92c.rgarciasuarez@​_ree._r>,
  Rafael Garcia-Suarez <rgarciasuarez@​free.fr> writes​:

With the patch below, I have these results :

$ \./perl \-Twle '%a=\(a=>4\, b=>5\); \*ENV=\\%a; system\("echo unsafe"\)'
%ENV is aliased to another variable while running with \-T switch at \-e line 1\.

$ \./perl \-Twle '%a=\(a=>4\, b=>5\); \*ENV=\*a; system\("echo unsafe"\)'
%ENV is aliased to %a while running with \-T switch at \-e line 1\.

i.e. when doing a TAINT_ENV check, immediately croak if %ENV is no
longer referring to the environment. This early failure approach
solves the security problem.

The behaviour of perl is not changed when not in -T or -t mode : i.e.
aliasing the *ENV glob works like regular glob aliasing.

If no objections or improvement proposals, I'll commit this.

If I understand correctly, the croak happens not on the localization
of ENV, but when the system() tries to use it ?

That sounds fine for my library. It fixes the security hole, I can still
fool CGI.pm with a fake env, and if the user tries to do a
system/exec/qr during that time, he will get an error (I wasn't too sure
about the proper behaviour for that last case anyways. The real PATH
etc. will in fact have been set to safe values, but the user maybe
expected his $ENV{PATH}=... to actually do something)

From @rgs

Ton Hospel wrote​:

If I understand correctly, the croak happens not on the localization
of ENV, but when the system() tries to use it ?

Yes. I added the check for aliased *ENV in the same routine that
checks for insecure $ENV{XYZ}. (It's thus only called with taint
checks turned on.)

That sounds fine for my library. It fixes the security hole, I can still
fool CGI.pm with a fake env, and if the user tries to do a
system/exec/qr during that time, he will get an error (I wasn't too sure
about the proper behaviour for that last case anyways. The real PATH
etc. will in fact have been set to safe values, but the user maybe
expected his $ENV{PATH}=... to actually do something)

Why do you want to alias *ENV by the way ?
You could have done
  local %ENV = (k1 => v1, ...);
or
  local %ENV = %ENV;
  $ENV{k1} = v1; ...

From perl5-porters@ton.iguana.be

In article <20031027160008.3b3377b_.rgarciasuarez@​_ree._r>,
  Rafael Garcia-Suarez <rgarciasuarez@​free.fr> writes​:

Ton Hospel wrote​:

If I understand correctly, the croak happens not on the localization
of ENV, but when the system() tries to use it ?

Yes. I added the check for aliased *ENV in the same routine that
checks for insecure $ENV{XYZ}. (It's thus only called with taint
checks turned on.)

Perfect.

That sounds fine for my library. It fixes the security hole, I can still
fool CGI.pm with a fake env, and if the user tries to do a
system/exec/qr during that time, he will get an error (I wasn't too sure
about the proper behaviour for that last case anyways. The real PATH
etc. will in fact have been set to safe values, but the user maybe
expected his $ENV{PATH}=... to actually do something)

Why do you want to alias *ENV by the way ?
You could have done
local %ENV = (k1 => v1, ...);
or
local %ENV = %ENV;
$ENV{k1} = v1; ...

It's an event driven setup to run many CGI's in parallel and it is meant
to be *FAST*. There is one fake environment hash reference per
connection object. Now if the corresponding input stream comes in in
many parts (consider many pending slow connections, or big fileuploads),
I will call the event handler many times, each time needing to switch in
about 40 environment variables. That's not only 40 useless calls to
setenv, but even worse, a lot of system calls.

Observe​:

strace perl -Twe 'local %ENV = (PATH =>"/a​:/b​:/c​:/d​:/e​:/f")'

....
stat64("/a", 0xbfffea9c) = -1 ENOENT (No such file or directory)
stat64("/b", 0xbfffea9c) = -1 ENOENT (No such file or directory)
stat64("/c", 0xbfffea9c) = -1 ENOENT (No such file or directory)
stat64("/d", 0xbfffea9c) = -1 ENOENT (No such file or directory)
stat64("/e", 0xbfffea9c) = -1 ENOENT (No such file or directory)
stat64("/f", 0xbfffea9c) = -1 ENOENT (No such file or directory)
.....

(there's also such a massive check for the original path on startup,
but I don't care about that, it's a persistent server)

Versus​:

strace perl -Twe 'local *ENV = {PATH =>"/a​:/b​:/c​:/d​:/e​:/f"}'

....
no stat calls on my new path (and no setenv calls)
....

So simply doing​:

sub io_readevent_callback {
  my $connection = shift;
  local *ENV = $connection->{env};
  ....
}

makes things a lot faster.

From @rgs

Ton Hospel wrote​:

In article <20031027160008.3b3377b_.rgarciasuarez@​_ree._r>,
Rafael Garcia-Suarez <rgarciasuarez@​free.fr> writes​:

Ton Hospel wrote​:

If I understand correctly, the croak happens not on the localization
of ENV, but when the system() tries to use it ?

Yes. I added the check for aliased *ENV in the same routine that
checks for insecure $ENV{XYZ}. (It's thus only called with taint
checks turned on.)

Perfect.

I've then applied the patch to bleadperl (21563).

@rgs - Status changed from 'new' to 'resolved'