Calls to getenv are buggy in core C code #14476

p5pRT · 2015-02-05T22:31:40Z

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

Searchable as RT123748$

p5pRT · 2015-02-05T22:31:40Z

From @khwilliamson

This is a bug report for perl from khw@khw,
generated with the help of perlbug 1.40 running under perl 5.20.1.

A bug that is showing up in os390 appears to be caused by a general
problem. Calls to libc getenv() return a pointer to static storage
in libc. That storage can be overwritten by another call to one of the
environment handling functions, including getenv. If you do
something like

s = PerlEnv_getenv("PERL5OPT")

and then parse 's', 's' could be corrupted by any other call to one of
the C environment routines, including a getenv. One might not expect
that a read acess to the environment would destroy something else, but
it could. There are a bunch of these in the core. In the example
above,from perl.c, while parsing, it can call moreswitches(), which
actually does a putenv() in some circumstances. I haven't investigated
to see if the flow actually permits to this happen, but if not, it's
just by luck.

If the result of a getenv is saved for later use, it should be copied,
savepv(), or else the results are undefined.

The core should be audited for occurrences of this issue.

Thanks to ilmari and alh for discussing this with me on #p5p

Flags:
category=core
severity=high

Site configuration information for perl 5.21.9:

Configured by khw at Thu Feb 5 08:31:14 MST 2015.

Summary of my perl5 (revision 5 version 21 subversion 9) configuration:
Commit id: a9b5431
Platform:
osname=linux, osvers=3.16.0-30-generic,
archname=x86_64-linux-thread-multi-ld
uname='linux khw 3.16.0-30-generic #40-ubuntu smp mon jan 12
22:06:37 utc 2015 x86_64 x86_64 x86_64 gnulinux '
config_args='-des -Uversiononly -Dprefix=/home/khw/blead -Dusedevel
-D'optimize=-ggdb3' -A'optimize=-ggdb3' -A'optimize=-O0'
-Accflags='-DPERL_BOOL_AS_CHAR' -Dman1dir=none -Dman3dir=none
-DDEBUGGING -Dcc=g++ -Dusemorebits -Dusethreads'
hint=recommended, useposix=true, d_sigaction=define
useithreads=define, usemultiplicity=define
use64bitint=define, use64bitall=define, uselongdouble=define
usemymalloc=n, bincompat5005=undef
Compiler:
cc='g++', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DPERL_BOOL_AS_CHAR
-fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong
-I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64
-D_FORTIFY_SOURCE=2',
optimize=' -ggdb3 -O0',
cppflags='-D_REENTRANT -D_GNU_SOURCE -DPERL_BOOL_AS_CHAR -fwrapv
-DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong
-I/usr/local/include'
ccversion='', gccversion='4.9.1', gccosandvers=''
intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678,
doublekind=3
d_longlong=define, longlongsize=8, d_longdbl=define,
longdblsize=16, longdblkind=3
ivtype='long', ivsize=8, nvtype='long double', nvsize=16,
Off_t='off_t', lseeksize=8
alignbytes=16, prototype=define
Linker and Libraries:
ld='g++', ldflags =' -fstack-protector-strong -L/usr/local/lib'
libpth=/usr/include/c++/4.9 /usr/include/x86_64-linux-gnu/c++/4.9
/usr/include/c++/4.9/backward /usr/local/lib
/usr/lib/gcc/x86_64-linux-gnu/4.9/include-fixed
/usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib
/usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib
libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
libc=libc-2.19.so, so=so, useshrplib=false, libperl=libperl.a
gnulibc_version='2.19'
Dynamic Linking:
dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
cccdlflags='-fPIC', lddlflags='-shared -ggdb3 -ggdb3 -O0
-L/usr/local/lib -fstack-protector-strong'

@INC for perl 5.21.9:
/home/khw/blead/lib/perl5/site_perl/5.21.9/x86_64-linux-thread-multi-ld
/home/khw/blead/lib/perl5/site_perl/5.21.9
/home/khw/blead/lib/perl5/5.21.9/x86_64-linux-thread-multi-ld
/home/khw/blead/lib/perl5/5.21.9
/home/khw/blead/lib/perl5/site_perl/5.21.8
/home/khw/blead/lib/perl5/site_perl/5.21.7
/home/khw/blead/lib/perl5/site_perl/5.21.6
/home/khw/blead/lib/perl5/site_perl/5.21.5
/home/khw/blead/lib/perl5/site_perl/5.21.4
/home/khw/blead/lib/perl5/site_perl/5.21.3
/home/khw/blead/lib/perl5/site_perl/5.21.2
/home/khw/blead/lib/perl5/site_perl/5.21.1
/home/khw/blead/lib/perl5/site_perl/5.20.0
/home/khw/blead/lib/perl5/site_perl/5.19.12
/home/khw/blead/lib/perl5/site_perl/5.19.11
/home/khw/blead/lib/perl5/site_perl/5.19.10
/home/khw/blead/lib/perl5/site_perl
.

Environment for perl 5.21.9:
HOME=/home/khw
LANG=en_US.UTF-8
LANGUAGE (unset)
LD_LIBRARY_PATH (unset)
LOGDIR (unset)

PATH=/home/khw/bin:/home/khw/perl5/perlbrew/bin:/home/khw/print/bin:/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/usr/games:/usr/local/games:/home/khw/cxoffice/bin
PERL5OPT=-w
PERL_BADLANG (unset)
PERL_POD_PEDANTIC=1
SHELL=/bin/ksh

p5pRT · 2015-02-06T03:58:41Z

From @wolfsage

On Thu, Feb 5, 2015 at 5:31 PM, karl williamson
<perlbug-followup@perl.org> wrote:

A bug that is showing up in os390 appears to be caused by a general
problem. Calls to libc getenv() return a pointer to static storage
in libc. That storage can be overwritten by another call to one of the
environment handling functions, including getenv. If you do
something like
 s = PerlEnv\_getenv$"PERL5OPT"$

I've added a test case for this particular issue with
8d28fc8.

I can get this, and locale.t...

not ok 30 - LANG is used if LC_ALL, LC_NUMERIC are invalid

...to fail by LD_PRELOADING replacements for *env functions that share
storage when used and compiling
Perl with -Accflags="-DPERL_USE_SAFE_PUTENV".

The locale.t failures are because of in locale.c, we do 3 getenvs and
possibly stomp on the first two:

char * const lc_all = PerlEnv_getenv("LC_ALL");
char * const lang = PerlEnv_getenv("LANG");
bool setlocale_failure = FALSE;
unsigned int i;
char *p;
const bool locwarn = (printwarn > 1 ||
(printwarn &&
(!(p = PerlEnv_getenv("PERL_BADLANG")) ||
grok_atou(p, NULL))));

It's not an ideal testing scenario but I believe it behaves as the
docs for getenv claim that it could.

If I switch the problematic PerlEnv_getenv calls to something like:

char *s, *sx;
sx = PerlEnv_getenv("PERL5OPT");
s = savepv(sx);

The problem goes away with my test setup (but now we leak memory?).

There are a number of other suspicious calls that don't seem to fail
any of the existing tests for me though.

-- Matthew Horsfall (alh)

p5pRT · 2015-02-06T03:58:42Z

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

p5pRT · 2015-02-06T08:05:31Z

From editor.buzzfeed@yahoo.com

In the example above,from perl.c, while parsing, it can call moreswitches(), which actually does a putenv() in some circumstances. I haven't investigated to see if the flow actually permits to this happen, but if not, it's just by luck. If the result of a getenv is saved for later use, it should be copied, savepv(), or else the results are undefined. The core should be audited for occurrences of this issue.

--
<a href="http://www.fixithere.net/" rel="tag">fixithere</a>

p5pRT · 2015-02-06T08:05:32Z

From [Unknown Contact. See original ticket]

In the example above,from perl.c, while parsing, it can call moreswitches(), which actually does a putenv() in some circumstances. I haven't investigated to see if the flow actually permits to this happen, but if not, it's just by luck. If the result of a getenv is saved for later use, it should be copied, savepv(), or else the results are undefined. The core should be audited for occurrences of this issue.

--
<a href="http://www.fixithere.net/" rel="tag">fixithere</a>

p5pRT · 2015-02-06T09:00:25Z

From @bulk88

Matthew Horsfall (alh) wrote:

On Thu, Feb 5, 2015 at 5:31 PM, karl williamson
<perlbug-followup@perl.org> wrote:
A bug that is showing up in os390 appears to be caused by a general
problem. Calls to libc getenv() return a pointer to static storage
in libc. That storage can be overwritten by another call to one of the
environment handling functions, including getenv. If you do
something like
 s = PerlEnv\_getenv$"PERL5OPT"$
I've added a test case for this particular issue with
8d28fc8.

I can get this, and locale.t...

not ok 30 - LANG is used if LC_ALL, LC_NUMERIC are invalid

...to fail by LD_PRELOADING replacements for *env functions that share
storage when used and compiling
Perl with -Accflags="-DPERL_USE_SAFE_PUTENV".

The locale.t failures are because of in locale.c, we do 3 getenvs and
possibly stomp on the first two:
char \* const lc\_all     = PerlEnv\_getenv$"LC\_ALL"$;
char \* const lang       = PerlEnv\_getenv$"LANG"$;
bool setlocale\_failure = FALSE;
unsigned int i;
char \*p;
const bool locwarn = $printwarn > 1 ||
                \(printwarn &&
                 \(\!\(p = PerlEnv\_getenv\("PERL\_BADLANG"$\) ||
                  grok\_atou$p\, NULL$\)\)\);
It's not an ideal testing scenario but I believe it behaves as the
docs for getenv claim that it could.

If I switch the problematic PerlEnv_getenv calls to something like:

char *s, *sx;
sx = PerlEnv_getenv("PERL5OPT");
s = savepv(sx);

The problem goes away with my test setup (but now we leak memory?).

There are a number of other suspicious calls that don't seem to fail
any of the existing tests for me though.

-- Matthew Horsfall (alh)

background http://man7.org/linux/man-pages/man3/getenv.3.html

Indiscriminately duplicating the memory block is not something I support.

One way of implementing getenv is storing the environment as double null
terminated records, getenv simply returns a ptr to the middle of the
process global user mode env var malloc block. Multiple calls to getenv
for different vars dont make the old getenv ptrs invalid, but any
setenv() will invalidate those pointers. The downside to this is, if
your overwrite bytes in the returned getenv ptr, you change the
processes env var.

Implementation 2, to stop the "security exploit" of getenv's ptr
changing the processes env if you wrote to the getenv ptr, there is a
process global malloced "last getenv" buffer. The value of the var is
copied into the global "last getenv" buffer, and the ptr to the buffer
is returned by getenv. This way writing to it won't change the processes
env, but each getenv call will invalidate the ptr of the previous getenv
call, unless you call getenv twice on the same env var, and no realloc
or obseravable change happened in the global buffer.

Implementation 3, you supply your own buffer and its length,
https://msdn.microsoft.com/en-us/library/windows/desktop/ms683188%28v=vs.85%29.aspx

Win32 Perl uses a combination of #1
(http://perl5.git.perl.org/perl.git/blob/HEAD:/win32/perlhost.h#l2397
http://perl5.git.perl.org/perl.git/blob/HEAD:/win32/perlhost.h#l2085 )
and #3 ( http://perl5.git.perl.org/perl.git/blob/HEAD:/win32/win32.c#l1
780 ) . No #2. #3 in Win32 Perl is done with SV mortal PV buffers.

p5pRT · 2015-02-06T15:30:53Z

From @craigberry

On Thu, Feb 5, 2015 at 4:31 PM, karl williamson
<perlbug-followup@perl.org> wrote:

# New Ticket Created by karl williamson
# Please include the string: [perl #123748]
# in the subject line of all future correspondence about this issue.
# <URL: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=123748 >

This is a bug report for perl from khw@khw,
generated with the help of perlbug 1.40 running under perl 5.20.1.

-----------------------------------------------------------------
A bug that is showing up in os390 appears to be caused by a general
problem. Calls to libc getenv() return a pointer to static storage
in libc. That storage can be overwritten by another call to one of the
environment handling functions, including getenv. If you do
something like
 s = PerlEnv\_getenv$"PERL5OPT"$
and then parse 's', 's' could be corrupted by any other call to one of
the C environment routines, including a getenv. One might not expect
that a read acess to the environment would destroy something else, but
it could. There are a bunch of these in the core. In the example
above,from perl.c, while parsing, it can call moreswitches(), which
actually does a putenv() in some circumstances. I haven't investigated
to see if the flow actually permits to this happen, but if not, it's
just by luck.

I think in this particular case (S_parse_body), whenever moreswitches
is called after the getenv call you cite above, it is passed a copy
created with:

popt_copy = SvPVX(sv_2mortal(newSVpv(d,0)));

so in other words it's already following your advice. Not sure if
that's good enough for threads, though as it's doing some parsing of
*s before it makes the copy.

If the result of a getenv is saved for later use, it should be copied,
savepv(), or else the results are undefined.

And obviously mortalized to prevent memory leaks. Or copied to
automatic C variables if only used within a function.

The core should be audited for occurrences of this issue.

I agree.

Thanks to ilmari and alh for discussing this with me on #p5p

p5pRT · 2015-02-06T16:27:21Z

From @wolfsage

On Fri, Feb 6, 2015 at 10:30 AM, Craig A. Berry <craig.a.berry@gmail.com> wrote:

I think in this particular case (S_parse_body), whenever moreswitches
is called after the getenv call you cite above, it is passed a copy
created with:
popt\_copy = SvPVX$sv\_2mortal\(newSVpv\(d\,0$\)\);
so in other words it's already following your advice. Not sure if
that's good enough for threads, though as it's doing some parsing of
*s before it makes the copy.

That's only if PERL5OPT contains a space after the '-', like:

PERL5OPT="- d:Trace"

But if it's:

PERL5OPT="-d:Trace",

popt_copy never happens:

if (!*s)
break;
if (!strchr("CDIMUdmtwW", *s))
Perl_croak(aTHX_ "Illegal switch in PERL5OPT: -%c", *s);
while (++s && *s) {
-----> if (isSPACE(*s)) {
if (!popt_copy) {
popt_copy = SvPVX(sv_2mortal(newSVpv(d,0)));
s = popt_copy + (s - d);
d = popt_copy;
}
*s++ = '\0';
break;
}
}

-- Matthew Horsfall (alh)

p5pRT · 2015-02-06T18:46:38Z

From @khwilliamson

locale.c has now been fixed in this regard, and now passes on os390
(commit 175c4cf).

Attached is a list of potential problematic places to audit, based on a
grep, with no further winnowing, except that I have examined locale.c
and removed the ones from this list that are from that file and are ok.

p5pRT · 2015-02-06T18:46:38Z

From @khwilliamson

getenv.log

p5pRT · 2015-03-09T21:26:12Z

From @khwilliamson

I looked through the list and found only one place where I thought it was problematic, which I fixed in commit 9e0b0d6

I didn't look at the specific platform files, like os2 ones.

Feel free to look at the list, or generate your own. But I am satisfied enough that I'm closing this ticket. I will add some textin the next day or two mentioning this issue in perlhacktips
--
Karl Williamson

p5pRT · 2015-03-09T21:26:14Z

@khwilliamson - Status changed from 'open' to 'resolved'

p5pRT closed this as completed Mar 9, 2015

p5pRT added the Severity Low label Oct 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calls to getenv are buggy in core C code #14476

Calls to getenv are buggy in core C code #14476

p5pRT commented Feb 5, 2015

p5pRT commented Feb 5, 2015

p5pRT commented Feb 6, 2015

p5pRT commented Feb 6, 2015

p5pRT commented Feb 6, 2015

p5pRT commented Feb 6, 2015

p5pRT commented Feb 6, 2015

p5pRT commented Feb 6, 2015

p5pRT commented Feb 6, 2015

p5pRT commented Feb 6, 2015

p5pRT commented Feb 6, 2015

p5pRT commented Mar 9, 2015

p5pRT commented Mar 9, 2015

Calls to getenv are buggy in core C code #14476

Calls to getenv are buggy in core C code #14476

Comments

p5pRT commented Feb 5, 2015

p5pRT commented Feb 5, 2015

From @khwilliamson

p5pRT commented Feb 6, 2015

From @wolfsage

p5pRT commented Feb 6, 2015

p5pRT commented Feb 6, 2015

From editor.buzzfeed@yahoo.com

p5pRT commented Feb 6, 2015

From [Unknown Contact. See original ticket]

p5pRT commented Feb 6, 2015

From @bulk88

p5pRT commented Feb 6, 2015

From @craigberry

p5pRT commented Feb 6, 2015

From @wolfsage

p5pRT commented Feb 6, 2015

From @khwilliamson

p5pRT commented Feb 6, 2015

From @khwilliamson

p5pRT commented Mar 9, 2015

From @khwilliamson

p5pRT commented Mar 9, 2015