Calls to getenv are buggy in core C code

p5pRT commented 9 years ago

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

Searchable as RT123748$

p5pRT commented 9 years ago

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: a9b5431bbad036dcb9773ff97b10a02d6cf706a0 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 commented 9 years ago

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 8d28fc8f69270cc75d9564b369ac6008c5b5d617.

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 commented 9 years ago

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

p5pRT commented 9 years ago

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.

-- \fixithere\

p5pRT commented 9 years ago

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.

-- \fixithere\

p5pRT commented 9 years ago

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 8d28fc8f69270cc75d9564b369ac6008c5b5d617.

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

From @khwilliamson

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

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 commented 9 years ago

From @khwilliamson

getenv.log

p5pRT commented 9 years ago

From @khwilliamson

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

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 commented 9 years ago

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

Perl / perl5