Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calls to getenv are buggy in core C code #14476

Closed
p5pRT opened this issue Feb 5, 2015 · 12 comments
Closed

Calls to getenv are buggy in core C code #14476

p5pRT opened this issue Feb 5, 2015 · 12 comments

Comments

@p5pRT
Copy link

p5pRT commented Feb 5, 2015

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

Searchable as RT123748$

@p5pRT
Copy link
Author

p5pRT commented Feb 5, 2015

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
Copy link
Author

p5pRT commented Feb 6, 2015

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
Copy link
Author

p5pRT commented Feb 6, 2015

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

@p5pRT
Copy link
Author

p5pRT commented Feb 6, 2015

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
Copy link
Author

p5pRT commented Feb 6, 2015

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
Copy link
Author

p5pRT commented Feb 6, 2015

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
Copy link
Author

p5pRT commented Feb 6, 2015

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
Copy link
Author

p5pRT commented Feb 6, 2015

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
Copy link
Author

p5pRT commented Feb 6, 2015

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
Copy link
Author

p5pRT commented Feb 6, 2015

From @khwilliamson

getenv.log

@p5pRT
Copy link
Author

p5pRT commented Mar 9, 2015

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
Copy link
Author

p5pRT commented Mar 9, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant