Skip Menu |
Report information
Id: 123748
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: public [at] khwilliamson.com
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: low
Type: unknown
Perl Version: (no value)
Fixed In: (no value)



Date: Thu, 05 Feb 2015 15:31:18 -0700
Subject: Calls to getenv are buggy in core C code
To: perlbug [...] perl.org
From: Karl Williamson <public [...] khwilliamson.com>
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
To: Perl5 Porters <perl5-porters [...] perl.org>
From: "Matthew Horsfall (alh)" <wolfsage [...] gmail.com>
Subject: Re: [perl #123748] Calls to getenv are buggy in core C code
Date: Thu, 5 Feb 2015 22:58:14 -0500
CC: bugs-bitbucket [...] rt.perl.org
Download (untitled) / with headers
text/plain 1.6k
On Thu, Feb 5, 2015 at 5:31 PM, karl williamson <perlbug-followup@perl.org> wrote: Show quoted text
> 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)
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 479b
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. Show quoted text
>
-- <a href="http://www.fixithere.net/" rel="tag">fixithere</a>
Subject: Re: [perl #123748] Calls to getenv are buggy in core C code
CC: Perl5 Porters <perl5-porters [...] perl.org>, bugs-bitbucket [...] rt.perl.org
To: "Matthew Horsfall (alh)" <wolfsage [...] gmail.com>
From: bulk88 <bulk88 [...] hotmail.com>
Date: Fri, 6 Feb 2015 03:59:46 -0500
Download (untitled) / with headers
text/plain 3.3k
Matthew Horsfall (alh) wrote: Show quoted text
> 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.
Subject: Re: [perl #123748] Calls to getenv are buggy in core C code
To: "Perl5 Porters (E-mail)" <perl5-porters [...] perl.org>
From: "Craig A. Berry" <craig.a.berry [...] gmail.com>
Date: Fri, 6 Feb 2015 09:30:26 -0600
CC: bugs-bitbucket [...] rt.perl.org
On Thu, Feb 5, 2015 at 4:31 PM, karl williamson <perlbug-followup@perl.org> wrote: Show quoted text
> # 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.perl.org/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. Show quoted text
> 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. Show quoted text
> The core should be audited for occurrences of this issue.
I agree. Show quoted text
> Thanks to ilmari and alh for discussing this with me on #p5p
Date: Fri, 6 Feb 2015 11:26:50 -0500
CC: "Perl5 Porters (E-mail)" <perl5-porters [...] perl.org>, bugs-bitbucket [...] rt.perl.org
Subject: Re: [perl #123748] Calls to getenv are buggy in core C code
From: "Matthew Horsfall (alh)" <wolfsage [...] gmail.com>
To: "Craig A. Berry" <craig.a.berry [...] gmail.com>
Download (untitled) / with headers
text/plain 1.2k
On Fri, Feb 6, 2015 at 10:30 AM, Craig A. Berry <craig.a.berry@gmail.com> wrote: Show quoted text
> 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)
To: perlbug-followup [...] perl.org
From: Karl Williamson <public [...] khwilliamson.com>
Subject: Re: [perl #123748] Calls to getenv are buggy in core C code
Date: Fri, 06 Feb 2015 11:45:58 -0700
Download (untitled) / with headers
text/plain 336b
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.
Download getenv.log
text/x-log 4.5k

Message body is not shown because sender requested not to inline it.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 426b
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


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org