Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.85k stars 527 forks source link

[engineering.redhat.com #385443][CVE-2016-2381] security issue with multiple environment entries with the same name #15118

Closed p5pRT closed 8 years ago

p5pRT commented 8 years ago

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

Searchable as RT127158$

p5pRT commented 8 years ago

From @stephane-chazelas

Hello\,

this is a follow-up on an issue I reported last year to the Debian security list and the sudo maintainer. As agreed with the Debian seclist\, I'm widening the discussion to maintainers of software affected by that issue (or at least software best placed to mitigate it)​: perl\, yash\, bash\, dash\, mksh\, fish. Debian\, Suse and Redhat seclists are included also in their quality of security point of contacts for GNU libc (non-GNU systems are also affected though).

It is a security issue in that it offers a way for attackers to bypass some sanitizing made to the environment. As noted by Florian Weimer\, it's already partly known (https://www.securecoding.cert.org/confluence/display/c/ENV02-C.+Beware+of+multiple+environment+variables+with+the+same+effective+name),

(issue tested on a current Debian testing system).

The issue​:

The execve(file\, argp[]\, envp[]) system call executes "file" and passes two lists of null-terminated strings. The second one is used to pass environment variables. By convention those strings are in the format "VARNAME=VARVALUE".

There is typically no sanitizing done by the libc or the kernel between that list being passed to execve() and the executed command receiving it in its environ[] (except for specific env vars blocked by glibc under AT_SECURE).

That envp[] may contain strings that don't follow that convention\, or\, and that's what's the issue here\, nothing prevents one to pass a "VAR=value" *and* a "VAR=other-value".

The essence of the problem is that when that happens\, executed applications behave differently allowing env vars with the wrong value to get through.

To test\, you can compile this simple executable​:

#include \<unistd.h> #include \<stdio.h> #include \<string.h> int main(int argc\, char* argv[]) {   int i;   for (i = 2; strcmp(argv[i]\, "EOA"); i++);   argv[i++] = 0;   execve(argv[1]\, &argv[1]\, &argv[i]);   perror(argv[1]);   return 1; }

Which is just a wrapper around execve(2).

Called as​:

$ ./execve /bin/bash -c 'echo "$a"; env' EOA a=1 a=2 a=3 3 PWD=/tmp a=3 SHLVL=1 _=/usr/bin/env

You can see here that bash sets its "a" variable from the *last* entry\, and removes all the other ones.

Other shells\, languages or libraries dealing with the environment behave differently.

The bash\, dash\, mksh\, fish\, rc\, yash shells and perl use the *last* entry. ksh93\, zsh and most other non-shells (libc's getenv\, python\, ruby\, tcl...) use the *first* entry.

Among shells\, only zsh and yash preserve the other entries.

yash and perl have a bug in that even though they use the *last* entry on input\, upon assigning a variable\, they update the first entry (also\, perl uses the last entry for %ENV but the library it uses will typically use the first entry)

$ ./execve /usr/bin/perl -le 'print $ENV{a}; $ENV{a} = "x";system("env")' EOA a=1 a=2 a=3 3 a=x a=2 a=3 $ ./execve /usr/bin/yash -c 'echo "$a"; a=x; /usr/bin/env' EOA a=1 a=2 a=3 3 a=x a=2 a=3 PWD=/tmp

Other tools I tested are consistent on that aspect.

When assigning a new value to an env var (for example with setenv()) that had multiple entries\, with those tools that don't remove duplicates\, only the first entry is updated.

What that means for instance is that on those systems where sh is based on bash\, dash or pdksh (that is most free software systems) a code (for instance for a setuid executable) that does​:

  setenv("PATH=/bin​:/sbin"\,1); /* sane value for PATH */   system("uname");

Would be vulnerable\, because if called with "PATH=/tmp/evil"\, "PATH=/tmp/evil"\, that setenv() would only update the *first* PATH variable\, while the "sh" invoked by system() would use the *last* one.

In the case of perl\, that allows bypassing the taint mode (set when perl is executed in setuids) in​:

$ ln -s /usr/bin/whoami uname $ ./execve /usr/bin/perl -Te 'system("uname;")' EOA PATH="$PWD" PATH="$PWD" Insecure $ENV{PATH} while running with -T switch at -e line 1.

Good\, perl didn't let us call uname with an unsafe PATH\, but​:

$ ./execve /usr/bin/perl -Te '$ENV{PATH}="/bin";system("uname;")' EOA PATH="$PWD" PATH="$PWD" chazelas

Here\, our "evil" (a symlink to whoami) uname was called\, because the second PATH variable was used.

For sudo\, that affected variables passed along by sudo (like LC_*) until Todd added some sanitizing​: https://www.sudo.ws/repos/sudo/rev/d4dfb05db5d7 (preserves only the first instance).

IMO\, here is what should be done​:

- fix perl and yash so they get the first entry for a variable - bash\, dash\, mksh\, fish changed so that they get the first   entry as well to be consistent with putenv/setenv that update   the first. - add some hardening to the libc so that under AT_SECURE\,   entries with duplicate env var names are removed (only first   one retained). - since most shells already remove duplicates\, yash and zsh   could probably do it as well. - one could argue that setenv\, putenv\, or assigning env vars in   other languages should also remove duplicates.

Some more information as already given for the benefit of the new participants​:

- I've verified that while one can modify the source code of an   openssh client to make it send several copies of an   environment variable to the server\, the server will only put   one of them (the last one) in the environment of the remote   shell. So the exploit cannot be carried out over ssh (at least   openssh).

- (only informational)\, the only case I know of an application   passing two env vars with the same name was bash prior to   shellshock where when you did​:

  foo() { echo foo; }; foo=bar; export foo; export -f foo; cmd

  bash would pass both foo=bar and foo="() { echo foo; }" to   cmd. (and if cmd was a bash script\, both the foo variable and   function would be imported).

  Since shellshock\, bash now passes foo=bar and   "BASH_FUNC_foo%%=() { echo foo; }"

-- Stephane

p5pRT commented 8 years ago

From tg@mirbsd.de

On Mon\, 4 Jan 2016 23​:21​:21 GMT\, stephane.chazelas@​gmail.com wrote​:

[…]

Ouch! Good point.

- bash\, dash\, mksh\, fish changed so that they get the first entry

Sorry\, that’s not going to happen. Assignment must stay consistent within the shell language.

If you run…

  foo() { echo $x; }   x=1 x=2 x=3 foo

… you expect it to show 3\, not 1\, too – independend of whether x is imported\, exported or neither.

I think the divergence is here that pdksh-based shells parse the entire environ into shell variables and then don’t use it any more so later lookup is a simple hash table lookup and fast\, whereas libc getenv(3) stops at the first match to be fast.

I would suggest letting the kernel deduplicate environ during exec… otherwise\, making libc getenv(3) not stop at the first match is something libc implementors will be unhappy about but possibly have to do (does environ have a size limit?).

- one could argue that setenv\, putenv\, or assigning env vars in other languages should also remove duplicates.

This is something that\, indeed\, should be done\, yes.

- (only informational)\, the only case I know of an application […] bash would pass both foo=bar and foo="() { echo foo; }" to

Ah\, okay.

Anyway\, thanks for uncovering this… “what fun” ☺

bye\, //mirabilos -- “It is inappropriate to require that a time represented as seconds since the Epoch precisely represent the number of seconds between the referenced time and the Epoch.”   -- IEEE Std 1003.1b-1993 (POSIX) Section B.2.2.2

p5pRT commented 8 years ago

From tg@mirbsd.de

Dixi quod…

be unhappy about but possibly have to do (does environ have a size limit?).

Yes\, ARG_MAX protects us from this being a DoS vector.

during exec… otherwise\, making libc getenv(3) not stop at the first match is something libc implementors will

Fun fact​: making getenv/putenv read and set the last item breaks Perl\, because Perl’s env import loop assigns the read environment variables to %ENV while the latter being magical already\, so this…

~/execve /usr/bin/perl -le 'print $ENV{a}; $ENV{a} = "x"; print $ENV{a}; system("env")' EOA a=1 a=2 a=3 a=4 a=5

… always prints the second-to-last value. Similarily\, from reading the code\, it will react allergically to shrinking environ during that loop (due to duplicate removal).

- one could argue that setenv\, putenv\, or assigning env vars in other languages should also remove duplicates.

This is something that\, indeed\, should be done\, yes.

This cannot be done\, thus\, without breaking existing binaries.

Then I see only one way out of this misery​: make the C startup (crt) remove the duplicates. Or the kernel\, since a csu fix may not catch all cases (asm binaries) and would need to be replicated e.g. in ld.so as well. (Using MirBSD terms here.)

In the meantime\, I studied what POSIX has to say; it doesn’t touch on duplicate keys but suggests implementations to use a hashtable for getenv() for speed anyway (unless the application uses putenv()).

(Note​: I looked at Perl 5.8.8; later versions may or may not differ.)

Although\, one thing that *could* be done is to set the _first_ entry and garbage-collect any later ones in setenv/putenv\, but if applications break like this I fear that may trigger bugs in other applications and believe normalisation in kernel/csu to be the best fix despite the startup cost for all applications.

bye\, //mirabilos -- (gnutls can also be used\, but if you are compiling lynx for your own use\, there is no reason to consider using that package)   -- Thomas E. Dickey on the Lynx mailing list\, about OpenSSL

p5pRT commented 8 years ago

From tg@mirbsd.de

Dixi quod…

Although\, one thing that *could* be done is to set the _first_ entry and garbage-collect any later ones in setenv/putenv

No\, this won’t work either; Perl with -DPERL_USE_SAFE_PUTENV would use the first value then.

So\, short of normalising the environment before any user code is run\, there’s no way to plug this problem without causing further breakage\, unfortunately.

(That being said\, I found a couple of bugs in MirBSD libc’s env code\, now I looked at it closely. Thanks.)

bye\, //mirabilos -- I believe no one can invent an algorithm. One just happens to hit upon it when God enlightens him. Or only God invents algorithms\, we merely copy them. If you don't believe in God\, just consider God as Nature if you won't deny existence. -- Coywolf Qi Hunt

p5pRT commented 8 years ago

From @stephane-chazelas

2016-01-05 15​:57​:57 +0000\, Thorsten Glaser​: [...]

- bash\, dash\, mksh\, fish changed so that they get the first entry

Sorry\, that’s not going to happen. Assignment must stay consistent within the shell language.

If you run…

foo\(\) \{ echo $x; \}
x=1 x=2 x=3 foo

… you expect it to show 3\, not 1\, too – independend of whether x is imported\, exported or neither.

Hi Thorsten\,

That's something different.

Of course in the shell command line​:

x=1 x=2 x=3 foo

we've got 3 variable assignments for the same variable\, so one overwrites the previous one. In the case of an external foo command\, we end up to​:

execve("foo"\, ["foo"]\, ["x=3"])

not

execve("foo"\, ["foo"]\, ["x=1"\, "x=2"\, "x=3"])

(note that the Bourne shell (and IIRC earlier versions of ash) parsed assignments from right to left\, so you'd end up with "x=1" with those shells)

What I'm talking of here is the processing of the environment the shell receives on startup. There\, having more than one entry for an environment variable is a pathological case\, variables are only meant to have *one* value. See how most shells (including pdksh/mksh do remove the duplicates). Shells that support exporting arrays (like rc\, es\, fish) do *not* do it by passing several a=1 a=2 (I suppose they can't rely on the order being preserved).

zsh and ksh93 already do the right thing.

The problem I'm talking of here (at least the particular attack vector I'm mentioning) is only because some shells (and perl) use the environment differently from the rest (from getenv/setenv to start with).

If the problem is fixed (shells assign $foo from the *first* "foo=whatever" it receives and ignore (like zsh) or discard (like ksh93) further entries for that same variable (and everything else does the same)\, the problem goes away.

Now that you mention hash tables\, I wonder if putenv/setenv getting and updating the first entry and do not reorder the entries is the norm or not.

-- Stephane

p5pRT commented 8 years ago

From @tonycoz

On Mon Jan 04 15​:22​:04 2016\, stephane.chazelas@​gmail.com wrote​:

- fix perl and yash so they get the first entry for a variable

The first of the attached patches does that by reversing the order we initialize %ENV.

- one could argue that setenv\, putenv\, or assigning env vars in other languages should also remove duplicates.

The second patch attempts to remove duplicates of an environment variable when it's set.

Tony

p5pRT commented 8 years ago

From @tonycoz

0001-perl-127158-reverse-initialization-of-ENV.patch ```diff From 9db49ead97699afd2030ae0c121a9ae7eb4a2a2f Mon Sep 17 00:00:00 2001 From: Tony Cook Date: Wed, 6 Jan 2016 10:30:08 +1100 Subject: [perl #127158] reverse initialization of %ENV On most platforms, if there are duplicate environment variables, getenv() and setenv() get/modify the first entry found, make %ENV consistent with that. --- perl.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/perl.c b/perl.c index b8d98ff..d33def9 100644 --- a/perl.c +++ b/perl.c @@ -4319,8 +4319,11 @@ S_init_postdump_symbols(pTHX_ int argc, char **argv, char **env) if (env) { char *s, *old_var; SV *sv; - for (; *env; env++) { - old_var = *env; + char **env_start = env; + while (*env) + ++env; + while (env > env_start) { + old_var = *--env; if (!(s = strchr(old_var,'=')) || s == old_var) continue; @@ -4330,10 +4333,10 @@ S_init_postdump_symbols(pTHX_ int argc, char **argv, char **env) (void)strupr(old_var); *s = '='; #endif - sv = newSVpv(s+1, 0); - (void)hv_store(hv, old_var, s - old_var, sv, 0); - if (env_is_not_environ) - mg_set(sv); + sv = newSVpv(s+1, 0); + (void)hv_store(hv, old_var, s - old_var, sv, 0); + if (env_is_not_environ) + mg_set(sv); } } #endif /* USE_ENVIRON_ARRAY */ -- 2.1.4 ```
p5pRT commented 8 years ago

From @tonycoz

0002-perl-127158-remove-duplicates-in-my_setenv.patch ```diff From 57bed4c34abe49405a03403cf3d4d26699ec0ad0 Mon Sep 17 00:00:00 2001 From: Tony Cook Date: Wed, 6 Jan 2016 11:43:18 +1100 Subject: [perl #127158] remove duplicates in my_setenv() It's possible for environ to contain duplicate definitions of an environment variable, to ensure a child process sees only the definition we remove any duplicates when a given variable is set. The implementations of unsetenv() I've tested removed all of the definitions, but POSIX is silent on how duplicate names are handled, so explicitly unsetenv() the name until we're sure it's gone. For the PERL_USE_SAFE_PUTENV case without unsetenv there's not really anything we can do. This needs tests, assuming I can find a way to to them. --- util.c | 74 ++++++++++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 47 insertions(+), 27 deletions(-) diff --git a/util.c b/util.c index 17b62dd..8f4514b 100644 --- a/util.c +++ b/util.c @@ -2148,30 +2148,47 @@ Perl_my_setenv(pTHX_ const char *nam, const char *val) tmpenv[max] = NULL; environ = tmpenv; /* tell exec where it is now */ } - if (!val) { - safesysfree(environ[i]); - while (environ[i]) { - environ[i] = environ[i+1]; - i++; + if (val) { + if (!environ[i]) { /* does not exist yet */ + environ = (char**)safesysrealloc(environ, (i+2) * sizeof(char*)); + environ[i+1] = NULL; /* make sure it's null terminated */ + } + else + safesysfree(environ[i]); + nlen = strlen(nam); + vlen = strlen(val); + + environ[i] = (char*)safesysmalloc((nlen+vlen+2) * sizeof(char)); + /* all that work just for this */ + my_setenv_format(environ[i], nam, nlen, val, vlen); + + /* look for a duplicate entry, if any */ + for (i++; environ[i]; i++) { + if (strnEQ(environ[i],nam,len) && environ[i][len] == '=') + break; } -#ifdef __amigaos4__ - goto my_setenv_out; -#else - return; -#endif - } - if (!environ[i]) { /* does not exist yet */ - environ = (char**)safesysrealloc(environ, (i+2) * sizeof(char*)); - environ[i+1] = NULL; /* make sure it's null terminated */ } - else + + if (environ[i]) { /* environ[i] is a match */ + I32 from = i+1; + + assert(strnEQ(environ[i],nam,len) && environ[i][len] == '='); + safesysfree(environ[i]); - nlen = strlen(nam); - vlen = strlen(val); - environ[i] = (char*)safesysmalloc((nlen+vlen+2) * sizeof(char)); - /* all that work just for this */ - my_setenv_format(environ[i], nam, nlen, val, vlen); + /* remove any duplicate definitions */ + while (environ[from]) { + if (strnEQ(environ[from],nam,len) && environ[from][len] == '=') { + safesysfree(environ[from]); + ++from; + } + else { + environ[i++] = environ[from++]; + } + } + /* copy the NULL */ + environ[i] = environ[from]; + } } else { # endif /* This next branch should only be called #if defined(HAS_SETENV), but @@ -2180,9 +2197,10 @@ Perl_my_setenv(pTHX_ const char *nam, const char *val) */ # if defined(__CYGWIN__)|| defined(__SYMBIAN32__) || defined(__riscos__) || (defined(__sun) && defined(HAS_UNSETENV)) # if defined(HAS_UNSETENV) - if (val == NULL) { - (void)unsetenv(nam); - } else { + /* ensure any duplicates are removed */ + while (PerlEnv_getenv(nam)) + unsetenv(nam); + if (val) { (void)setenv(nam, val, 1); } # else /* ! HAS_UNSETENV */ @@ -2190,10 +2208,12 @@ Perl_my_setenv(pTHX_ const char *nam, const char *val) # endif /* HAS_UNSETENV */ # else # if defined(HAS_UNSETENV) - if (val == NULL) { - if (environ) /* old glibc can crash with null environ */ - (void)unsetenv(nam); - } else { + /* ensure any duplicates are removed */ + if (environ) { /* old glibc can crash with null environ */ + while (PerlEnv_getenv(nam)) + unsetenv(nam); + } + if (val) { const int nlen = strlen(nam); const int vlen = strlen(val); char * const new_env = -- 2.1.4 ```
p5pRT commented 8 years ago

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

p5pRT commented 8 years ago

From krahmer@suse.com

On Tue\, Jan 05\, 2016 at 10​:03​:47PM +0000\, Stephane CHAZELAS wrote​:

[...]

The problem I'm talking of here (at least the particular attack vector I'm mentioning) is only because some shells (and perl) use the environment differently from the rest (from getenv/setenv to start with).

If the problem is fixed (shells assign $foo from the *first* "foo=whatever" it receives and ignore (like zsh) or discard (like ksh93) further entries for that same variable (and everything else does the same)\, the problem goes away.

Now that you mention hash tables\, I wonder if putenv/setenv getting and updating the first entry and do not reorder the entries is the norm or not.

Worse. putenv/setenv reallocate and move stuff across the heap\, resetting __environ and may leave envp[] argument of main() out of sync. (think threads)

I think the only thing we can do is sticking to what POSIX says about getenv/putenv etc. and making perl etc. aware that dups may happen and remove them on init (not ignoring them! that already caused root exploits via openbsd rtld double LD_PRELOAD).

glibc properly handles insecure dups on AT_SECURE. Maybe POSIX allows to erase dups for any startup by ld.so\, which would be prefered.

-s

--

~ perl self.pl ~ $_='print"\$_=\47$_\47;eval"';eval ~ krahmer@​suse.com - SuSE Security Team

p5pRT commented 8 years ago

From @stephane-chazelas

2016-01-06 11​:00​:42 +0100\, Sebastian Krahmer​: [...]

Worse. putenv/setenv reallocate and move stuff across the heap\, resetting __environ and may leave envp[] argument of main() out of sync. (think threads) [...]

But can any of the env entries that a program *received* (as opposed to the ones added later with setenv/putenv) be reordered with glibc (or other libcs)? I've not managed to.

Can setenv("x"\, "value") update anything else than the *first* entry? Or that upon the next execve()\, that "x=value" may not be the first one?

I think the only thing we can do is sticking to what POSIX says about getenv/putenv etc. and making perl etc. aware that dups may happen and remove them on init (not ignoring them! that already caused root exploits via openbsd rtld double LD_PRELOAD). [...]

Well\, the problem is that in​:

setenv("PATH=sane-value"); execlp("perl-script"); or system("cmd");

setenv updates only one of the PATH entries (in my experience the first)\, but perl or sh use a different one. Even if perl is modified to use only one and ignore the rest (and most sh do that)\, it has to be the same one as set by setenv().

Here\, if we were to cast blame\, that would partly on setenv() that fails to update *all* the PATH entries (or set one PATH env var and remove the other ones)\, or on bash/mksh/dash/fish that get a different PATH from the one set by setenv() (or the kernel's execve() for not removing duplicates).

glibc properly handles insecure dups on AT_SECURE. [...]

What do you mean?

They don't seem to be removed here (Linux Mint 17.3) for a setuid executable​:

$ ll env -rwsr-x--- 1 root chazelas 27232 Nov 27 2014 env* $ ./execve ./env EOA PATH=/tmp/evil PATH=/tmp/evil PATH=/tmp/evil PATH=/tmp/evil $ ./execve ./env PATH=/usr/bin env EOA PATH=/tmp/evil PATH=/tmp/evil PATH=/usr/bin PATH=/tmp/evil $ /lib/x86_64-linux-gnu/libc-2.19.so GNU C Library (Ubuntu EGLIBC 2.19-0ubuntu6.6) stable release version 2.19\, by Roland McGrath et al. Copyright (C) 2014 Free Software Foundation\, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. Compiled by GNU CC version 4.8.2. Compiled on a Linux 3.13.11 system on 2015-02-25. Available extensions​:   crypt add-on version 2.1 by Michael Glad and others   GNU Libidn by Simon Josefsson   Native POSIX Threads Library by Ulrich Drepper et al   BIND-8.2.3-T5B libc ABIs​: UNIQUE IFUNC For bug reporting instructions\, please see​: \https://bugs.launchpad.net/ubuntu/+source/eglibc/+bugs.

(according to ltrace\, that "env" uses putenv(3)).

-- Stephane

p5pRT commented 8 years ago

From krahmer@suse.com

On Wed\, Jan 06\, 2016 at 01​:08​:19PM +0000\, Stephane CHAZELAS wrote​:

2016-01-06 11​:00​:42 +0100\, Sebastian Krahmer​: [...]

Worse. putenv/setenv reallocate and move stuff across the heap\, resetting __environ and may leave envp[] argument of main() out of sync. (think threads) [...]

But can any of the env entries that a program *received* (as opposed to the ones added later with setenv/putenv) be reordered with glibc (or other libcs)? I've not managed to.

Can setenv("x"\, "value") update anything else than the *first* entry? Or that upon the next execve()\, that "x=value" may not be the first one?

Thats implementation dependend of corse\, so we have to assume the bad case that it may be re-ordered. If not now\, maybe in future implementations. Thats why I voted for removing (erasing) dups rather than ignoring them (and also because of the de-sync envp[] thing).

I think the only thing we can do is sticking to what POSIX says about getenv/putenv etc. and making perl etc. aware that dups may happen and remove them on init (not ignoring them! that already caused root exploits via openbsd rtld double LD_PRELOAD). [...]

Well\, the problem is that in​:

setenv("PATH=sane-value"); execlp("perl-script"); or system("cmd");

setenv updates only one of the PATH entries (in my experience the first)\, but perl or sh use a different one. Even if perl is modified to use only one and ignore the rest (and most sh do that)\, it has to be the same one as set by setenv().

Yes\, see above about _removing_ dups. Also note that this example is insecure either way since best practices is to either clearenv() and only then using setenv() or to pass the array itself to execve(). For exactly the reason of junk/dups that may be inherited. Using a polluted environment and then calling some setenv's never was secure. White lists vs. black lists approach.

Here\, if we were to cast blame\, that would partly on setenv() that fails to update *all* the PATH entries (or set one PATH env var and remove the other ones)\, or on bash/mksh/dash/fish that get a different PATH from the one set by setenv() (or the kernel's execve() for not removing duplicates).

glibc properly handles insecure dups on AT_SECURE. [...]

What do you mean?

They don't seem to be removed here (Linux Mint 17.3) for a setuid executable​:

Removing any and all occurences of insecure env vars\, including any dups.

TL;DR​: I ack the problem of environment variable dups\, but patching perl etc. to ignore them is no solution to me. Either patch perl etc. to remove the dups on early startup so they cant do harm\, or "enhance" ld.so to do so (if POSIX and ABI permits that).

Sebastian

--

~ perl self.pl ~ $_='print"\$_=\47$_\47;eval"';eval ~ krahmer@​suse.com - SuSE Security Team

p5pRT commented 8 years ago

From tg@mirbsd.de

Sebastian Krahmer dixit​:

Worse. putenv/setenv reallocate and move stuff across the heap\, resetting __environ and may leave envp[] argument of main() out of sync. (think threads)

POSIX says that​: – environment is not thread-safe at all anyway – envp out-of-sync is to be expected – applications shall use getenv/setenv/unsetenv\,   but the historic usage of looking at environ[]   is still allowed

glibc properly handles insecure dups on AT_SECURE. Maybe POSIX allows to erase dups for any startup by ld.so\, which would be prefered.

POSIX doesn’t say a single word about duplicates (I looked).

I also think fixing this in the startup (kernel\, ld.so\, crt0) is the only option not breaking any existing applicatiions.

As for the first-vs.-last debate​: I asked a random selection of Linux and Mac OSX users with some programming knowledge\, and they unambigously said they expect the *last* occurrence of the key to be used\, n̲o̲t̲ the first.

Tony Cook via RT dixit​:

The first of the attached patches does that by reversing the order we initialize %ENV.

Please do *not* do that!

bye\, //mirabilos -- This space for rent.

p5pRT commented 8 years ago

From tg@mirbsd.de

Sebastian Krahmer dixit​:

On Wed\, Jan 06\, 2016 at 01​:08​:19PM +0000\, Stephane CHAZELAS wrote​:

But can any of the env entries that a program *received* (as opposed to the ones added later with setenv/putenv) be reordered with glibc (or other libcs)? I've not managed to.

That’s possible\, and I had that as next step after…

Can setenv("x"\, "value") update anything else than the *first* entry? Or that upon the next execve()\, that "x=value" may not be the first one?

… this\, which was very easy to do in MirBSD libc\, but it turns out that doing this breaks at least Perl.

Thats why I voted for removing (erasing) dups rather than ignoring them

Exactly.

(and also because of the de-sync envp[] thing).

POSIX says envp[] is not available because it would be de-synched; setenv(3) is allowed to change environ.

Yes\, see above about _removing_ dups. Also note that this example is insecure either way since best practices is to either clearenv() and only

clearenv() is explicitly not in POSIX\, but you can use execve() or change environ in the application (which is also permissible).

then using setenv() or to pass the array itself to execve(). For exactly the reason of junk/dups that may be inherited.

Indeed\, which is why the de-duplication must happen at application start before any user code is run\, possibly in the kernel\, otherwise in the startup code.

ld.so to do so (if POSIX and ABI permits that).

POSIX doesn’t say anything about duplicates\, and I’d say go\, but don’t forget static executables (oh\, and those compiled with other runtimes… FreePascal comes to mind\, or anything Assembly).

bye\, //mirabilos -- This space for rent.

p5pRT commented 8 years ago

From @stephane-chazelas

2016-01-06 13​:40​:11 +0000\, Thorsten Glaser​: [...]

As for the first-vs.-last debate​: I asked a random selection of Linux and Mac OSX users with some programming knowledge\, and they unambigously said they expect the *last* occurrence of the key to be used\, n̲o̲t̲ the first. [...]

Not sure what you asked them exactly\, but I'd expect if you ask them which one should getenv() or setenv() update\, they would say the first and that's what most things do (getenv/setenv\, python\, lua\, ruby\, tcl\, zsh\, AT&T ksh at least).

That's a mute point as that's something that is not meant to happen anyway.

And which one it is doesn't matter anyway as long as everyone agrees on it. But as it looks like we may not be able to have a guarantee of order\, I agree it's probably safer to enforce unique entries at a lower level (kernel\, crt...) on startup.

Would that break things though? Until now\, at least on GNU/Linux\, argv and envp are interchangeable. It's still possible some things may use the environment in unconventional fashion? (unlikely when many shells do remove the unconventional entries).

-- Stephane

p5pRT commented 8 years ago

From tg@mirbsd.de

stephane.chazelas@​gmail.com via RT dixit​:

2016-01-06 13​:40​:11 +0000\, Thorsten Glaser​:

As for the first-vs.-last debate​: I asked a random selection of Linux and Mac OSX users with some programming knowledge\, and they unambigously said they expect the *last* occurrence of the key to be used\, n̲o̲t̲ the first.

Not sure what you asked them exactly\, but I'd expect if you ask

I asked them that\, when I pass x=1 x=2 x=3 to a program in the environment\, what they’d expect the value of x to be.

them which one should getenv() or setenv() update\, they would say the first and that's what most things do (getenv/setenv\, python\, lua\, ruby\, tcl\, zsh\, AT&T ksh at least).

This is probably either due to actually using getenv/setenv or not having thought about it enough. Consider​:

  env = {}   env['x'] = '1'   env['x'] = '2'   env['x'] = '3'

I fail to see why there ought to be a distinction between environment imports and in-language assignments in the first place; additionally\, for things like JSON\, the last key wins as well\, so this is overall more consistent.

And which one it is doesn't matter anyway as long as everyone agrees on it.

Getting everyone to agree probably will be a hell of fun… (including my own stubbornness) ☺

guarantee of order\, I agree it's probably safer to enforce unique entries at a lower level (kernel\, crt...) on startup.

Right.

Would that break things though? Until now\, at least on GNU/Linux\, argv and envp are interchangeable. It's still

You mean environ and envp.

They are interchangable until the first setenv() with a new value was called\, but no longer\, from that point.

But look at how main() gets called. I’m using the MirBSD code as reference here\, as I’m intimately familiar with it\, but I assume most ELF-based systems do the same. Let’s look at a statically linked executable.

The kernel loads the ELF binary and jumps to its entry point\, which is usually some variant of the _start symbol\, as gcc calls ld with -e _start (or -e __start). That is called “raw”\, i.e. not with a function frame.

The implementation of __start then reads some arguments from registers (such as ps_strings) and stack (argc\, for example) and calculates envp as the stack address just behind argc plus argc+1 times the size of a pointer. Then it constructs a C stack frame and calls some less machine-specific function which does a couple of things\, depending on the OS. For MirBSD\, that is​:

• set environ to envp • set __progname as basename(argv[0]) • store ps_strings aside • if profiling​: atexit(_mcleanup) and call monstartup • call atexit(_fini); and _init();   (_init() does things like constructors) and its last line is​: • exit(main(argc\, argv\, environ));

That means that the envp main() sees is not the envp passed by the OS (actually\, the OS doesn't pass one\, it just places it on the stack right after the argv array) but whatever environ is set to.

Furthermore\, we only d̲e̲duplicate here\, i.e. we don’t increase the size of the environment. environ[0]\, environ[1]\, etc. are all places on the stack\, pointing to strings also on the stack. To deduplicate\, we merely have to memmove() things inside the environ array around\, which means that we don’t even need to change the value of environ/envp but just its contents.

I remember looking at how Linux does things\, and it also just places the environment pointers on the stack after the argument vector pointers IIRC.

Of course\, doing this in the kernel would fix this the best way as no code duplication (ld.so for dynamically linked executables\, crt0 for statically linked ones\, and some unknown amount of custom runtimes\, different C libraries (dietlibc\, klibc\, µClibc-ng\, musl\, …) and who knows what else) is needed.

bye\, //mirabilos -- This space for rent.

p5pRT commented 8 years ago

From @stephane-chazelas

2016-01-06 18​:30​:07 +0000\, Thorsten Glaser​: [...]

Would that break things though? Until now\, at least on GNU/Linux\, argv and envp are interchangeable. It's still

You mean environ and envp. [...]

FWIW\, I did mean argv[] and envp[]\, as in they are just two lists of arbitrary strings. Some applications\, may use envp[] for other purposes than passing a list of environment variables.

[...]

That means that the envp main() sees is not the envp passed by the OS (actually\, the OS doesn't pass one\, it just places it on the stack right after the argv array) but whatever environ is set to.

But environ would be the address of the first pointer to the first env string\, that's something put there by execve().

Furthermore\, we only d̲e̲duplicate here\, i.e. we don’t increase the size of the environment. environ[0]\, environ[1]\, etc. are all places on the stack\, pointing to strings also on the stack. To deduplicate\, we merely have to memmove() things inside the environ array around\, which means that we don’t even need to change the value of environ/envp but just its contents.

Just the content of the pointers\, the duplicate env values would still stay on the stack\, but no longer reached.

I remember looking at how Linux does things\, and it also just places the environment pointers on the stack after the argument vector pointers IIRC.

Yes\, see http​://unix.stackexchange.com/questions/91561/ps-full-command-is-too-long/91562#91562

the bottom of the stack is​:

argc argv-pointers envp-pointers argv-strings envp-strings.

Of course\, doing this in the kernel would fix this the best way as no code duplication (ld.so for dynamically linked executables\, crt0 for statically linked ones\, and some unknown amount of custom runtimes\, different C libraries (dietlibc\, klibc\, µClibc-ng\, musl\, …) and who knows what else) is needed. [...]

Yes. True.

It can potentially break things though.

On the other hand\, changing bash\,dash\,yash\,mksh\,fish\,perl so they get the first duplicate would be harmless. That may not completely fix the problem\, but nobody has offered a counter-example yet where for example​:

putenv("PATH=sane-value"); system("foo");

with a fixed "sh" (one behaving like zsh or ksh93) can be fooled on current systems.

-- Stephane

p5pRT commented 8 years ago

From tg@mirbsd.de

(Why isn’t dgk (the Korn in ksh) in the Cc list\, anyway?)

stephane.chazelas@​gmail.com via RT dixit​:

lists of arbitrary strings. Some applications\, may use envp[] for other purposes than passing a list of environment variables.

Well POSIX doesn’t guarantee it anyway.

But environ would be the address of the first pointer to the first env string\, that's something put there by execve().

And that is not going to change when we deduplicate.

Just the content of the pointers\, the duplicate env values would still stay on the stack\, but no longer reached.

Exactly.

Even better if the kernel deduplicates\, of course. That needs the various kernels’ hackers to do it…

the bottom of the stack is​:

argc argv-pointers envp-pointers argv-strings envp-strings.

Right\, and we just make the envp-pointers a bit shorter (and fill it up to the original size with NULL pointers).

It can potentially break things though.

Every change can potentially break things. This is the place where I believe the least amount of them breaks ;)

On the other hand\, changing bash\,dash\,yash\,mksh\,fish\,perl so they get the first duplicate would be harmless. That may not

I don’t currently see enough reasons to change mksh to give the first value precedence. Later assignments a̲l̲w̲a̲y̲s̲ override earlier assignments.

Furthermore\, this would make 'sh' and '(env;cat)|sh' out of sync which can cause even more “fun”…

bye\, //mirabilos -- This space for rent.

p5pRT commented 8 years ago

From @hvds

"Tony Cook via RT" \perl5\-security\-report@&#8203;perl\.org wrote​: :On Mon Jan 04 15​:22​:04 2016\, stephane.chazelas@​gmail.com wrote​: :> - fix perl and yash so they get the first entry for a variable : :The first of the attached patches does that by reversing the order :we initialize %ENV.

Technically\, this looks good to me\, independent of the discussion on whether it's desirable.

:> - one could argue that setenv\, putenv\, or assigning env vars in :> other languages should also remove duplicates. : :The second patch attempts to remove duplicates of an environment variable :when it's set.

Other than minor comments below\, this also looks good. I'm a bit worried that Perl_vmssetenv() explicitly documents that it will set/remove only the first copy found\, it'd be useful to get some insight into the nature of this under VMS and what effect this change for other platforms will have on portability.

[...] :+ nlen = strlen(nam);

For later​: note that this (and the same in alternate branches below) is the same as len above\, other than type; surely we can avoid calling strlen() twice for these?

:+ if (environ[i]) { /* environ[i] is a match */

I think this could do with a clearer comment\, maybe something like​:   /* environ[i] now points to an entry to be removed; remove it\, and any   * further instances of the same name.   */

: # if defined(__CYGWIN__)|| defined(__SYMBIAN32__) || defined(__riscos__) || (defined(__sun) && defined(HAS_UNSETENV))

Note this context line will conflict on rebase\, since blead now has PERL_DARWIN too.

:+ /* ensure any duplicates are removed */ :+ while (PerlEnv_getenv(nam)) :+ unsetenv(nam);

I'm nervous of infinite loops here and in the unsetenv+putenv case below. I'd really like to see a test that would exercise these in the obscure platform smokes\, even if we have to release before we can see those.

: # else /* ! HAS_UNSETENV */

I think this and the putenv() case below need to comment that they won't (and can't) kill dupes. Longer term\, there might be value in a build option that would refuse to build in these cases.

Hugo

p5pRT commented 8 years ago

From @tonycoz

On Wed Jan 06 06​:13​:54 2016\, mirabilos wrote​:

Tony Cook via RT dixit​:

The first of the attached patches does that by reversing the order we initialize %ENV.

Please do *not* do that!

One problem with the current behaviour is that the internals of perl (and XS code) that use getenv() and perl code can see two different values for an environment variable\, eg​:

$ execve ./perl -Ilib -MPOSIX -le 'print $ENV{LANG}; print setlocale("");' EOA LANG=en_US.UTF-8 LANG=en_AU.UTF-8 en_AU.UTF-8 en_US.UTF-8

Reversing the order fixes that when getenv() works in what seems to me to be the most obvious way (a linear search from the start of environ.)

Another change I'm considering is aborting under -T if a duplicate is found.

I can see that causing problems if anyone is creating an envp[] for execve() by copying environ[] and appending new definitions to the end without removing duplicates.

Tony

p5pRT commented 8 years ago

From tg@mirbsd.de

Tony Cook via RT dixit​:

On Wed Jan 06 06​:13​:54 2016\, mirabilos wrote​:

Tony Cook via RT dixit​:

The first of the attached patches does that by reversing the order we initialize %ENV.

Please do *not* do that!

One problem with the current behaviour is that the internals of perl (and XS code) that use getenv() and perl code can see two different values for an environment variable\, eg​:

Unless that has changed since Perl 5.8.8\, if you enable putenv\, this will additionally break things (try with three or more values… in my test I didn’t reverse but assigning to %ENV during import would overwrite values when Perl order and setenv order don’t match).

I can see that causing problems if anyone is creating an envp[] for execve() by copying environ[] and appending new definitions to the end without removing duplicates.

Another point in favour of last-one-wins\, and another point in favour of deduplicating between handover to the kernel on execve() and running any user code in the new process.

bye\, //mirabilos -- This space for rent.

p5pRT commented 8 years ago

From @stephane-chazelas

2016-01-06 20​:02​:44 -0800\, Tony Cook via RT​: [...]

One problem with the current behaviour is that the internals of perl (and XS code) that use getenv() and perl code can see two different values for an environment variable\, eg​:

$ execve ./perl -Ilib -MPOSIX -le 'print $ENV{LANG}; print setlocale("");' EOA LANG=en_US.UTF-8 LANG=en_AU.UTF-8 en_AU.UTF-8 en_US.UTF-8

Yes\, I had already mentioned that in my initial report but forgot to add it when /widening/ the discussion\, sorry about that. Original report attached in case there's more I forgot to mention (my memory is not as good as it used to be...).

Reversing the order fixes that when getenv() works in what seems to me to be the most obvious way (a linear search from the start of environ.)

Yes\, and one of the big questions is indeed "can we rely (in practice\, in existing implementations\, I know POSIX gives no guarantee there) on getenv() returning the first entry and on setenv()/putenv() not re-shuffling the entries (at least the order of entries received on the last execve() preserved for the next execve())?"

Another change I'm considering is aborting under -T if a duplicate is found.

Sounds reasonable. As I noted earlier though\, pre-shellshock bashs could genuinely have two instances of a variable (but then again\, those duplicates would be stripped by some tools like most shells\, so it would already be broken)

I can see that causing problems if anyone is creating an envp[] for execve() by copying environ[] and appending new definitions to the end without removing duplicates. [...]

If they do\, they're broken already\, as that duplicate entry won't be accessible by most things including getenv().

-- Stephane

p5pRT commented 8 years ago

From @stephane-chazelas

Message RFC822: X-RT-Original-Encoding: utf-8 From: Stephane Chazelas stephane.chazelas@gmail.com User-Agent: Mutt/1.5.21 (2010-09-15) MIME-Version: 1.0 Date: Fri, 20 Nov 2015 15:21:46 +0000 To: "Todd C. Miller" Todd.Miller@courtesan.com, team@security.debian.org Subject: sudo/setuids and multiple environment entries with the same name Message-ID: 20151120152146.GA10325@chaz.gmail.com Content-Disposition: inline Content-Type: text/plain; charset="utf-8" Content-Length: 3845

Hello,

this is not an issue with sudo, but I think something sudo (and the libc/dynamic linker at least in setuid/setgid...) should guard against.

Consider this little C program:

define _GNU_SOURCE

include

int main(int argc, char argv[]) { char e[] = {"LC_ALL=tr_TR.UTF-8", "LC_ALL=tr_TR.UTF-8", 0}; execvpe(argv[1], &argv[1], e); return 1; }

to be used as:

./a.out cmd...

(on non-GNU systems, replace execvpe with execvp and cmd with /path/to/cmd).

That calls cmd with LC_ALL=tr_TR.UTF-8 twice in the environment.

If I run it as:

./a.out sudo cmd

That variable is still passed twice.

Now, if we have a variable passed twice like that, the behaviour varies between applications. getenv("LC_ALL") will return the first one, $ENV{"LC_ALL"} in perl will return the last one, but $ENV{"LC_ALL"} = "C" will update the first one (!).

Some shells $LC_ALL (zsh, AT&T ksh) will get the first one, dash, bash, yash, pdksh the last one. Of those, all but zsh and yash will remove the duplicates. perl/ruby/python won't.

setenv() and putenv() on GNU, like zsh, yash, perl, ruby, python when assigning to LC_ALL will update the first one and leave the other ones untouched.

What that means is that if I have a program that does:

putenv("LC_ALL=C"); / make sure we're in a sane locale / / or setenv("LC_ALL", "C", 1) / system("something");

Or:

!/usr/bin/perl

$ENV{LC_ALL}="C"; system("something");

Or:

! /usr/bin/ruby

ENV["LC_ALL"]="C"; exec("something");

Or:

!/bin/zsh

LC_ALL_C something

If the "sh" called by system() is based on dash/bash/pdksh which is usually the case on FOSS OSes, or "something" is a sh/perl script or calls a sh/perl script, then LC_ALL will still be tr_TR.UTF-8.

I like the tr_TR.UTF-8 locale in examples, because at least on Debian, that's one locale where uppercase i is not I (so echo evil | grep -i EVIL fails), the decimal separator is ",", thousand separator ".", month/day/AM/PM have multi-byte characters... and of course all the security issues related to UTF-8 and non-C collation.

LC_ALL is one of the variables passed along by sudo at least on Debian, but we could also have problems with any variable (like PATH) by any other setuid/setgid program.

Basically, the vulnerability we have here is a way to bypass environment sanitising in privilege escalation contexts by passing the env var several times.

I've not made a review of what software may be impacted, but there's bound to be some. PAM modules, setuid/setgids especially should be checked against that.

IMO, the libc and sudo should strip those duplicates at least for setuid/setgids. bash/dash/mksh already strip duplicates but should retain the first one to be consistent with getenv(). perl should also be fixed to get the first one in its $ENV (that's definitly a bug there as it updates the first one when you assign to $ENV elements, and when it calls setlocale(), it's the first one that is used).

One could also argue that putenv/setenv or assigning env vars in any of those languages should remove duplicates.

PoC:

$ cat a.c

define _GNU_SOURCE

include

int main(int argc, char argv[]) { char e[] = {"LC_ALL=tr_TR.UTF-8", "LC_ALL=tr_TR.UTF-8", 0}; execvpe(argv[1], &argv[1], e); return 1; } $ cat sanitize.c

include

include

int main(int argc, char*argv[]) {putenv("LC_ALL=C"); execvp(argv[1], &argv[1]);} $ make a cc a.c -o a $ make sanitize cc sanitize.c -o sanitize $ ./a sudo ./sanitize sh -c 'echo evil | grep -i EVIL' $ ./a sudo ./sanitize sh -c 'date' Cum Kas 20 14:21:49 GMT 2015 $ ./a sudo perl -e '$ENV{LC_ALL}="C"; system("date")' Fri Nov 20 14:22:25 GMT 2015 $ ./a sudo perl -e '$ENV{LC_ALL}="C"; system("date>&2")' Cum Kas 20 14:22:31 GMT 2015 $ ./a sudo zsh -c 'LC_ALL=C sh -c date' Cum Kas 20 14:23:32 GMT 2015

-- Stephane

p5pRT commented 8 years ago

From @stephane-chazelas

Hello\,

this is not an issue with sudo\, but I think something sudo (and the libc/dynamic linker at least in setuid/setgid...) should guard against.

Consider this little C program​:

#define _GNU_SOURCE #include \<unistd.h> int main(int argc\, char* argv[]) {   char *e[] = {"LC_ALL=tr_TR.UTF-8"\, "LC_ALL=tr_TR.UTF-8"\, 0};   execvpe(argv[1]\, &argv[1]\, e);   return 1; }

to be used as​:

./a.out cmd...

(on non-GNU systems\, replace execvpe with execvp and cmd with /path/to/cmd).

That calls cmd with LC_ALL=tr_TR.UTF-8 twice in the environment.

If I run it as​:

./a.out sudo cmd

That variable is still passed twice.

Now\, if we have a variable passed twice like that\, the behaviour varies between applications. getenv("LC_ALL") will return the first one\, $ENV{"LC_ALL"} in perl will return the last one\, but $ENV{"LC_ALL"} = "C" will update the first one (!).

Some shells $LC_ALL (zsh\, AT&T ksh) will get the first one\, dash\, bash\, yash\, pdksh the last one. Of those\, all but zsh and yash will remove the duplicates. perl/ruby/python won't.

setenv() and putenv() on GNU\, like zsh\, yash\, perl\, ruby\, python when assigning to LC_ALL will update the first one and leave the other ones untouched.

What that means is that if I have a program that does​:

putenv("LC_ALL=C"); /* make sure we're in a sane locale */ /* or setenv("LC_ALL"\, "C"\, 1) */ system("something");

Or​:

#!/usr/bin/perl $ENV{LC_ALL}="C"; system("something");

Or​:

#! /usr/bin/ruby ENV["LC_ALL"]="C"; exec("something");

Or​:

#!/bin/zsh LC_ALL_C something

If the "sh" called by system() is based on dash/bash/pdksh which is usually the case on FOSS OSes\, or "something" is a sh/perl script or calls a sh/perl script\, then LC_ALL will still be tr_TR.UTF-8.

I like the tr_TR.UTF-8 locale in examples\, because at least on Debian\, that's one locale where uppercase i is not I (so echo evil | grep -i EVIL fails)\, the decimal separator is "\,"\, thousand separator "."\, month/day/AM/PM have multi-byte characters... and of course all the security issues related to UTF-8 and non-C collation.

LC_ALL is one of the variables passed along by sudo at least on Debian\, but we could also have problems with any variable (like PATH) by any other setuid/setgid program.

Basically\, the vulnerability we have here is a way to bypass environment sanitising in privilege escalation contexts by passing the env var several times.

I've not made a review of what software may be impacted\, but there's bound to be some. PAM modules\, setuid/setgids especially should be checked against that.

IMO\, the libc and sudo should strip those duplicates at least for setuid/setgids. bash/dash/mksh already strip duplicates but should retain the first one to be consistent with getenv(). perl should also be fixed to get the first one in its $ENV (that's definitly a bug there as it updates the first one when you assign to $ENV elements\, and when it calls setlocale()\, it's the first one that is used).

One could also argue that putenv/setenv or assigning env vars in any of those languages should remove duplicates.

PoC​:

$ cat a.c #define _GNU_SOURCE #include \<unistd.h> int main(int argc\, char* argv[]) {   char *e[] = {"LC_ALL=tr_TR.UTF-8"\, "LC_ALL=tr_TR.UTF-8"\, 0};   execvpe(argv[1]\, &argv[1]\, e);   return 1; } $ cat sanitize.c #include \<stdlib.h> #include \<unistd.h> int main(int argc\, char*argv[]) {putenv("LC_ALL=C"); execvp(argv[1]\, &argv[1]);} $ make a cc a.c -o a $ make sanitize cc sanitize.c -o sanitize $ ./a sudo ./sanitize sh -c 'echo evil | grep -i EVIL' $ ./a sudo ./sanitize sh -c 'date' Cum Kas 20 14​:21​:49 GMT 2015 $ ./a sudo perl -e '$ENV{LC_ALL}="C"; system("date")' Fri Nov 20 14​:22​:25 GMT 2015 $ ./a sudo perl -e '$ENV{LC_ALL}="C"; system("date>&2")' Cum Kas 20 14​:22​:31 GMT 2015 $ ./a sudo zsh -c 'LC_ALL=C sh -c date' Cum Kas 20 14​:23​:32 GMT 2015

-- Stephane

p5pRT commented 8 years ago

From @rjbs

It seems to me like the sanest\, simplest\, and least confusing thing is to use Tony Cook's two patches to (a) make perl's env have the same contents you'd get from a linear search and (b) clean up dupes on set. Tony and I briefly discussed the possibility of cleaning up the environment itself on boot\, but it didn't seem like a good way forward.

As for barfing on ambiguous environments​:

* I'd love doing that always if I felt more sure that it was nearly never the case in sane operation (but I really don't feel that way) * I'm not keen on making it depend on -T\, as it turns -T into "more security" rather than "taint" * ...but that doesn't mean I think it's a bad idea\, either

-- rjbs

p5pRT commented 8 years ago

From @jhi

On Monday-201601-11 18​:37\, Ricardo SIGNES via RT wrote​:

* I'd love doing that always if I felt more sure that it was nearly never the case in sane operation (but I really don't feel that way) * I'm not keen on making it depend on -T\, as it turns -T into "more security" rather than "taint" * ...but that doesn't mean I think it's a bad idea\, either

Croak by default but have an environment variable

env PERL_ALLOW_DUP_ENV_FULL_SPEED_AHEAD_AND_DAMN_THE_TORPEDOES=1 ...

would be playing on the "safe by default" side\, while still allowing a foolhardy captain to steam ahead?

p5pRT commented 8 years ago

From @tonycoz

On Mon\, Jan 11\, 2016 at 06​:42​:36PM -0500\, Jarkko Hietaniemi wrote​:

On Monday-201601-11 18​:37\, Ricardo SIGNES via RT wrote​:

* I'd love doing that always if I felt more sure that it was nearly never the case in sane operation (but I really don't feel that way) * I'm not keen on making it depend on -T\, as it turns -T into "more security" rather than "taint" * ...but that doesn't mean I think it's a bad idea\, either

Croak by default but have an environment variable

env PERL_ALLOW_DUP_ENV_FULL_SPEED_AHEAD_AND_DAMN_THE_TORPEDOES=1 ...

would be playing on the "safe by default" side\, while still allowing a foolhardy captain to steam ahead?

Do you know if VMS can have multiple definitions of the same name in the environment in normal cases?

Tony

p5pRT commented 8 years ago

From @jhi

I don't know VMS that intimately but I would suspect it does not\, since I think env there is basically a system-level lookup table\, instead of a manually-diddled single block of memory. In fact\, it is a hierarchy of lookup tables.

If we don't have Craig in perl-security\, we should.

On Mon\, Jan 11\, 2016 at 6​:49 PM\, Tony Cook \tony@&#8203;develop\-help\.com wrote​:

On Mon\, Jan 11\, 2016 at 06​:42​:36PM -0500\, Jarkko Hietaniemi wrote​:

On Monday-201601-11 18​:37\, Ricardo SIGNES via RT wrote​:

* I'd love doing that always if I felt more sure that it was nearly never the case in sane operation (but I really don't feel that way) * I'm not keen on making it depend on -T\, as it turns -T into "more security" rather than "taint" * ...but that doesn't mean I think it's a bad idea\, either

Croak by default but have an environment variable

env PERL_ALLOW_DUP_ENV_FULL_SPEED_AHEAD_AND_DAMN_THE_TORPEDOES=1 ...

would be playing on the "safe by default" side\, while still allowing a foolhardy captain to steam ahead?

Do you know if VMS can have multiple definitions of the same name in the environment in normal cases?

Tony

-- There is this special biologist word we use for 'stable'. It is 'dead'. -- Jack Cohen

p5pRT commented 8 years ago

From @rjbs

On Mon Jan 11 15​:43​:05 2016\, jhi wrote​:

On Monday-201601-11 18​:37\, Ricardo SIGNES via RT wrote​:

* I'd love doing that always if I felt more sure that it was nearly never the case in sane operation (but I really don't feel that way) * I'm not keen on making it depend on -T\, as it turns -T into "more security" rather than "taint" * ...but that doesn't mean I think it's a bad idea\, either

Croak by default but have an environment variable

env PERL_ALLOW_DUP_ENV_FULL_SPEED_AHEAD_AND_DAMN_THE_TORPEDOES=1 ...

would be playing on the "safe by default" side\, while still allowing a foolhardy captain to steam ahead?

So\, my concern with that is something like this​:

If ambiguous environments are created by (possibly half-baked programs written in) shells\, and those are the only places where this is going to approach "common" in the wild\, then we're going to see zero reports of failure until things are failing on production systems.

Lots of people smoke their applications and libraries on bleadperl\, but nobody "smokes" their production systems\, cron jobs\, glue\, and so on. They also tend to run that stuff against system perl\, meaning that they won't see the breakage when v5.x.0 comes out\, but some years later when their scripts are ported to the new company standard\, Excited Egret. Changes that will affect "the stuff sysadmins do" are the changes where I feel we must be the most careful.

So\, with that in mind\, are we better off having perl bail or having it change to use the first value? It's not entirely clear. Either way (putting aside the case where (X=1) is found multiple times in env)\, the behavior changes. My gut says that we're better off having programs still run\, although I'm not sure I have any strong argument to make in favor of it.

-- rjbs

p5pRT commented 8 years ago

From @rjbs

On Mon Jan 11 15​:57​:32 2016\, jhi wrote​:

If we don't have Craig in perl-security\, we should.

Tony noted his absence and I sent him an invite earlier this evening. If he declines to join I'll at least get him to consult on this ticket.

-- rjbs

p5pRT commented 8 years ago

From @craigberry

On Mon Jan 11 16​:00​:13 2016\, rjbs wrote​:

On Mon Jan 11 15​:57​:32 2016\, jhi wrote​:

If we don't have Craig in perl-security\, we should.

Tony noted his absence and I sent him an invite earlier this evening. If he declines to join I'll at least get him to consult on this ticket.

I'm here (at least in the RT queue).

The Perl %ENV hash on VMS is constructed from a list of one or more of logical names\, CLI symbols\, and the CRTL environ array in a specified order. The first two are hashes so guaranteed unique within each when accessed from within the same mode (more on that in a sec). Don't know offhand about environ; it's probably like most Unix systems that haven't been specifically updated for this vulnerability.

As I think Hugo was hinting at up-thread\, there may be a special variant of the bug on VMS deriving from the fact that %ENV is concocted from multiple sources and even if each is unique within itself\, the flattened view in %ENV could have multiple definitions of the same key behind it. If I have a logical name FOO and an environ element FOO\, deleting $ENV{FOO} will remove whichever one is first in my search list and leave the other. The next lookup will find what's left. I guess this could be changed to delete it from all locations\, not just the first one found\, but that would be a change in documented behavior. I need to think more about this and come up with a test case or two.

Also\, logical names have layers called modes. It's more or less like the Windows registry where less privileged versions of the same key sit "in front of" the more privileged versions. Come to think of it Windows environment variables also exist at the user and system levels. In all of these cases\, a key can have a less privileged translation and a more privileged one\, and I think deleting the first will leave the second. Not sure if that's exploitable in any way\, but for the sake of thoroughness it is a form of having two translations for the same key.

p5pRT commented 8 years ago

From @craigberry

On Mon Jan 11 16​:00​:13 2016\, rjbs wrote​:

On Mon Jan 11 15​:57​:32 2016\, jhi wrote​:

If we don't have Craig in perl-security\, we should.

Tony noted his absence and I sent him an invite earlier this evening. If he declines to join I'll at least get him to consult on this ticket.

I'm here (at least in the RT queue).

The Perl %ENV hash on VMS is constructed from a list of one or more of logical names\, CLI symbols\, and the CRTL environ array in a specified order. The first two are hashes so guaranteed unique within each when accessed from within the same mode (more on that in a sec). Don't know offhand about environ; it's probably like most Unix systems that haven't been specifically updated for this vulnerability.

As I think Hugo was hinting at up-thread\, there may be a special variant of the bug on VMS deriving from the fact that %ENV is concocted from multiple sources and even if each is unique within itself\, the flattened view in %ENV could have multiple definitions of the same key behind it. If I have a logical name FOO and an environ element FOO\, deleting $ENV{FOO} will remove whichever one is first in my search list and leave the other. The next lookup will find what's left. I guess this could be changed to delete it from all locations\, not just the first one found\, but that would be a change in documented behavior. I need to think more about this and come up with a test case or two.

Also\, logical names have layers called modes. It's more or less like the Windows registry where less privileged versions of the same key sit "in front of" the more privileged versions. Come to think of it Windows environment variables also exist at the user and system levels. In all of these cases\, a key can have a less privileged translation and a more privileged one\, and I think deleting the first will leave the second. Not sure if that's exploitable in any way\, but for the sake of thoroughness it is a form of having two translations for the same key.

p5pRT commented 8 years ago

From tg@mirbsd.de

Ricardo SIGNES via RT dixit​:

So\, with that in mind\, are we better off having perl bail or having it change to use the first value? It's not entirely clear. Either way

Considering the entire scenario (not just Perl) it’s probably best to not change any existing applications (e.g. Perl or the shells) but change the OSes to sanitise the environment\, and possibly\, if at all\, warn (or\, in some strict mode\, die) if duplicates are found (but consider those warnings could also impact programs).

bye\, //mirabilos -- This space for rent.

p5pRT commented 8 years ago

From @jhi

On Monday-201601-11 18​:59\, Ricardo SIGNES via RT wrote​:

On Mon Jan 11 15​:43​:05 2016\, jhi wrote​:

On Monday-201601-11 18​:37\, Ricardo SIGNES via RT wrote​:

* I'd love doing that always if I felt more sure that it was nearly never the case in sane operation (but I really don't feel that way) * I'm not keen on making it depend on -T\, as it turns -T into "more security" rather than "taint" * ...but that doesn't mean I think it's a bad idea\, either

Croak by default but have an environment variable

env PERL_ALLOW_DUP_ENV_FULL_SPEED_AHEAD_AND_DAMN_THE_TORPEDOES=1 ...

would be playing on the "safe by default" side\, while still allowing a foolhardy captain to steam ahead?

So\, my concern with that is something like this​:

If ambiguous environments are created by (possibly half-baked programs written in) shells\, and those are the only places where this is going to approach "common" in the wild\, then we're going to see zero reports of failure until things are failing on production systems.

Lots of people smoke their applications and libraries on bleadperl\, but nobody "smokes" their production systems\, cron jobs\, glue\, and so on. They also tend to run that stuff against system perl\, meaning that they won't see the breakage when v5.x.0 comes out\, but some years later when their scripts are ported to the new company standard\, Excited Egret. Changes that will affect "the stuff sysadmins do" are the changes where I feel we must be the most careful.

So\, with that in mind\, are we better off having perl bail or having it change to use the first value? It's not entirely clear. Either way (putting aside the case where (X=1) is found multiple times in env)\, the behavior changes. My gut says that we're better off having programs still run\, although I'm not sure I have any strong argument to make in favor of it.

Personally I find it somewhat concerning to prioritize not breaking existing code over fixing security issues. But since I cannot make up my mind over how serious this particular issue is\, I don't feel particularly strongly either way.

p5pRT commented 8 years ago

From @craigberry

On Wed Jan 06 12​:59​:12 2016\, hv wrote​:

:+ /* ensure any duplicates are removed */ :+ while (PerlEnv_getenv(nam)) :+ unsetenv(nam);

I'm nervous of infinite loops here and in the unsetenv+putenv case below.

Me too. I haven't built with this patch yet but I think that could easily cause an infinite loop on VMS because %ENV is a tied hash to the real environment outside Perl and you wouldn't normally have privileges to delete everything in it.

Here's a taste for how the layers work where I've defined the same key in multiple different logical name tables​:

$ define/job PERLTMP JOB $ define/process PERLTMP PROCESS $ define/user PERLTMP USER $ perl -e "while (exists $ENV{PERLTMP}) {print qq/$ENV{PERLTMP}\n/; delete $ENV{PERLTMP};}" USER PROCESS JOB

If I had also defined PERLTMP system-wide and run the above code without privileges it would loop infinitely because it would not be able to delete the last entry.

Having multiple definitions for the same key in different layers is a natural and normal way to do things on VMS and lots of things depend on that working.

Now\, in addition to all of the above I could also have a definition for PERLTMP in the CRTL environ array that would be invisible in %ENV if the logical name was defined but visible if the logical name was not defined. That's documented behavior.

If the problem is just that a hidden value could avoid taint checks\, I don't think that happens for any of these cases. I infer that there's more to it than that but don't yet have a very good handle on what it is.

p5pRT commented 8 years ago

From @craigberry

On Wed Jan 06 12​:59​:12 2016\, hv wrote​:

:+ /* ensure any duplicates are removed */ :+ while (PerlEnv_getenv(nam)) :+ unsetenv(nam);

I'm nervous of infinite loops here and in the unsetenv+putenv case below.

Me too. I haven't built with this patch yet but I think that could easily cause an infinite loop on VMS because %ENV is a tied hash to the real environment outside Perl and you wouldn't normally have privileges to delete everything in it.

Here's a taste for how the layers work where I've defined the same key in multiple different logical name tables​:

$ define/job PERLTMP JOB $ define/process PERLTMP PROCESS $ define/user PERLTMP USER $ perl -e "while (exists $ENV{PERLTMP}) {print qq/$ENV{PERLTMP}\n/; delete $ENV{PERLTMP};}" USER PROCESS JOB

If I had also defined PERLTMP system-wide and run the above code without privileges it would loop infinitely because it would not be able to delete the last entry.

Having multiple definitions for the same key in different layers is a natural and normal way to do things on VMS and lots of things depend on that working.

Now\, in addition to all of the above I could also have a definition for PERLTMP in the CRTL environ array that would be invisible in %ENV if the logical name was defined but visible if the logical name was not defined. That's documented behavior.

If the problem is just that a hidden value could avoid taint checks\, I don't think that happens for any of these cases. I infer that there's more to it than that but don't yet have a very good handle on what it is.

p5pRT commented 8 years ago

From @rjbs

* Jarkko Hietaniemi \jhi@&#8203;iki\.fi [2016-01-13T09​:25​:50]

Personally I find it somewhat concerning to prioritize not breaking existing code over fixing security issues. But since I cannot make up my mind over how serious this particular issue is\, I don't feel particularly strongly either way.

I agree with you on both counts\, here​: we need to take action to fix things if they're really broken security-wise\, but I'm not sure how they are.

There's a related question​: if we have a security problem because existing code can behave unexpectedly\, what exactly would "expectedly" mean? The "die on ambiguous environment" solution is a concession to the notion that nobody knows. I think nobody knows\, which makes it appealing. The "don't break things" angle makes me want to downgrade to "we will tell you what to expect​: act like a linear scan."

-- rjbs

p5pRT commented 8 years ago

From @jhi

On Wednesday-201601-13 21​:47\, Ricardo Signes wrote​:

I agree with you on both counts\, here​: we need to take action to fix things if they're really broken security-wise\, but I'm not sure how they are.

Just for the sake of argument\, privilege escalation (local or remote) would be a security issue that I think should overrule the "not breaking" rule.

p5pRT commented 8 years ago

From tg@mirbsd.de

Jarkko Hietaniemi via RT dixit​:

On Wednesday-201601-13 21​:47\, Ricardo Signes wrote​:

I agree with you on both counts\, here​: we need to take action to fix things if they're really broken security-wise\, but I'm not sure how they are.

Just for the sake of argument\, privilege escalation (local or remote) would be a security issue that I think should overrule the "not breaking" rule.

Although\, this is an OS-level issue that cannot easily be fixed just in various userspace tools.

Though\, if you wanted to fix Perl\, ensure that the deduplication happens before putenv is called (or the environ pointers are manipulated\, in the !SAFE_PUTENV case). Then\, choose either the first or last to win (I’d say the last).

bye\, //mirabilos -- This space for rent.

p5pRT commented 8 years ago

From @craigberry

On Tue Jan 05 17​:12​:24 2016\, tonyc wrote​:

On Mon Jan 04 15​:22​:04 2016\, stephane.chazelas@​gmail.com wrote​:

- fix perl and yash so they get the first entry for a variable

The first of the attached patches does that by reversing the order we initialize %ENV.

Inline Patch ```diff diff --git a/perl.c b/perl.c index b8d98ff..d33def9 100644 --- a/perl.c +++ b/perl.c @@ -4319,8 +4319,11 @@ S_init_postdump_symbols(pTHX_ int argc, char **argv, char **env) if (env) { ```

I've been going a bit crazy trying to figure out how this code will ever fire. As far as I can see init_postdump_symbols is only ever called from perl_parse or S_parse_body via perl_parse. The one and only call to perl_parse in perlmain.c looks like:

  exitstatus = perl_parse(my_perl\, xs_init\, argc\, argv\, (char **)NULL);

So that last argument passing in the environment is always NULL\, and has been since 5.002 Beta 1​:

http​://perl5.git.perl.org/perl.git/commit/4633a7c4bad06b471d9310620b7fe8ddd158cccd?f=miniperlmain.c

So any references to the environment in perl_parse or anything it calls (including S_init_postdump_symbols) appear to be vestigial. What am I missing?

p5pRT commented 8 years ago

From @craigberry

On Tue Jan 05 17​:12​:24 2016\, tonyc wrote​:

On Mon Jan 04 15​:22​:04 2016\, stephane.chazelas@​gmail.com wrote​:

- fix perl and yash so they get the first entry for a variable

The first of the attached patches does that by reversing the order we initialize %ENV.

Inline Patch ```diff diff --git a/perl.c b/perl.c index b8d98ff..d33def9 100644 --- a/perl.c +++ b/perl.c @@ -4319,8 +4319,11 @@ S_init_postdump_symbols(pTHX_ int argc, char **argv, char **env) if (env) { ```

I've been going a bit crazy trying to figure out how this code will ever fire. As far as I can see init_postdump_symbols is only ever called from perl_parse or S_parse_body via perl_parse. The one and only call to perl_parse in perlmain.c looks like:

  exitstatus = perl_parse(my_perl\, xs_init\, argc\, argv\, (char **)NULL);

So that last argument passing in the environment is always NULL\, and has been since 5.002 Beta 1​:

http​://perl5.git.perl.org/perl.git/commit/4633a7c4bad06b471d9310620b7fe8ddd158cccd?f=miniperlmain.c

So any references to the environment in perl_parse or anything it calls (including S_init_postdump_symbols) appear to be vestigial. What am I missing?

p5pRT commented 8 years ago

From @tonycoz

On Thu\, Jan 14\, 2016 at 04​:06​:54PM -0800\, Craig A. Berry via RT wrote​:

On Tue Jan 05 17​:12​:24 2016\, tonyc wrote​:

On Mon Jan 04 15​:22​:04 2016\, stephane.chazelas@​gmail.com wrote​:

- fix perl and yash so they get the first entry for a variable

The first of the attached patches does that by reversing the order we initialize %ENV.

diff --git a/perl.c b/perl.c index b8d98ff..d33def9 100644 --- a/perl.c +++ b/perl.c @​@​ -4319\,8 +4319\,11 @​@​ S_init_postdump_symbols(pTHX_ int argc\, char **argv\, char **env) if (env) {

I've been going a bit crazy trying to figure out how this code will ever fire. As far as I can see init_postdump_symbols is only ever called from perl_parse or S_parse_body via perl_parse. The one and only call to perl_parse in perlmain.c looks like​:

exitstatus = perl\_parse\(my\_perl\, xs\_init\, argc\, argv\, \(char \*\*\)NULL\);

So that last argument passing in the environment is always NULL\, and has been since 5.002 Beta 1​:

http​://perl5.git.perl.org/perl.git/commit/4633a7c4bad06b471d9310620b7fe8ddd158cccd?f=miniperlmain.c

So any references to the environment in perl_parse or anything it calls (including S_init_postdump_symbols) appear to be vestigial. What am I missing?

Around line 4308​:

  if (!env)   env = environ;

Tony

p5pRT commented 8 years ago

From @craigberry

On Thu Jan 14 16​:36​:10 2016\, tonyc wrote​:

On Thu\, Jan 14\, 2016 at 04​:06​:54PM -0800\, Craig A. Berry via RT wrote​:

So any references to the environment in perl_parse or anything it calls (including S_init_postdump_symbols) appear to be vestigial. What am I missing?

Around line 4308​:

if (!env) env = environ;

Aha\, thanks. On VMS we define NO_ENVIRON_ARRAY in vmsish.h\, which means USE_ENVIRON_ARRAY will be false\, which explains why I stepped right past all this in the debugger without seeing it.

p5pRT commented 8 years ago

From @craigberry

On Thu Jan 14 16​:36​:10 2016\, tonyc wrote​:

On Thu\, Jan 14\, 2016 at 04​:06​:54PM -0800\, Craig A. Berry via RT wrote​:

So any references to the environment in perl_parse or anything it calls (including S_init_postdump_symbols) appear to be vestigial. What am I missing?

Around line 4308​:

if (!env) env = environ;

Aha\, thanks. On VMS we define NO_ENVIRON_ARRAY in vmsish.h\, which means USE_ENVIRON_ARRAY will be false\, which explains why I stepped right past all this in the debugger without seeing it.

p5pRT commented 8 years ago

From chet.ramey@case.edu

On 1/14/16 7​:13 AM\, Thorsten Glaser wrote​:

Jarkko Hietaniemi via RT dixit​:

On Wednesday-201601-13 21​:47\, Ricardo Signes wrote​:

I agree with you on both counts\, here​: we need to take action to fix things if they're really broken security-wise\, but I'm not sure how they are.

Just for the sake of argument\, privilege escalation (local or remote) would be a security issue that I think should overrule the "not breaking" rule.

Although\, this is an OS-level issue that cannot easily be fixed just in various userspace tools.

I agree that this is something that should be fixed in the C library or the kernel.

Doesn't it seem like the fact that we're proposing to fix so many separate pieces and tools implies that those proposals fix the problem in the wrong place?

-- ``The lyf so short\, the craft so long to lerne.'' - Chaucer   ``Ars longa\, vita brevis'' - Hippocrates Chet Ramey\, ITS\, CWRU chet@​case.edu http​://cnswww.cns.cwru.edu/~chet/

p5pRT commented 8 years ago

From @fweimer

* Chet Ramey​:

I agree that this is something that should be fixed in the C library or the kernel.

I agree. I would welcome a discussion on libc-alpha. Any fix will have to be subject to public review because it's a pretty significant change. One problem is that we have to implement very early in process startup in glibc\, where nothing of the library proper is available for use.

The issue has been in the public domain\, as a security issue\, since the mid-90s at least. It's mentioned in the second edition of “Practical UNIX & Internet Security”.

Florian

p5pRT commented 8 years ago

From tg@mirbsd.de

Florian Weimer via RT dixit​:

change. One problem is that we have to implement very early in process startup in glibc\, where nothing of the library proper is available for use.

Which is why I suggested to do it in the kernel instead.

bye\, //mirabilos -- Stéphane\, I actually don’t block Googlemail\, they’re just too utterly stupid to successfully deliver to me (or anyone else using Greylisting and not whitelisting their ranges). Same for a few other providers such as Hotmail. Some spammers (Yahoo) I do block.

p5pRT commented 8 years ago

From @craigberry

On Fri Jan 15 07​:32​:44 2016\, chet.ramey@​case.edu wrote​:

On 1/14/16 7​:13 AM\, Thorsten Glaser wrote​:

Although\, this is an OS-level issue that cannot easily be fixed just in various userspace tools.

I agree that this is something that should be fixed in the C library or the kernel.

Towards that end\, and FWIW\, here's a quick and dirty program that doesn't involve any utilities but just execs itself to demonstrate whether the execve implementation preserves or eliminates duplicate keys. Here I'm running on OS X 10.11.2 and the presence of both A=1 and A=3 in the output indicates that no deduplication is done.

$ cat execve_env_dups.c #include \<stdio.h> #include \<stdlib.h> #include \<unistd.h>

int main(int argc\, char *argv[]\, char *envp[]) {   char *newargv[] = { NULL\, "printenv"\, NULL };   char *newenvp[] = { "A=1"\, "B=2"\, "A=3"\, "C=4"\, NULL };

  if (argc == 1) {   newargv[0] = argv[0];   execve(argv[0]\, newargv\, newenvp);   perror("execve"); /* execve() shouldn't return */   exit(EXIT_FAILURE);   }   else {   int i = 0;   while (envp[i] != NULL) {   printf("%s\n"\, envp[i]); i++;   }   exit(EXIT_SUCCESS);   } } $ ./execve_env_dups A=1 B=2 A=3 C=4

p5pRT commented 8 years ago

From @fweimer

* Thorsten Glaser​:

Florian Weimer via RT dixit​:

change. One problem is that we have to implement very early in process startup in glibc\, where nothing of the library proper is available for use.

Which is why I suggested to do it in the kernel instead.

From a kernel perspective\, the environment list is just another argument list. It's elements do not even have to have the KEY=VALUE structure. Breaking that seems to be a bit drastic. It would need some sort of policy knob\, I think.

Is the general feeling to apply this sanitization only on an AT_SECURE/SUID boundary\, or always?

Florian

p5pRT commented 8 years ago

From tg@mirbsd.de

Florian Weimer via RT dixit​:

From a kernel perspective\, the environment list is just another argument list. It's elements do not even have to have the KEY=VALUE

POSIX requires that structure\, though; on the other hands\, keys can be pretty much arbitrary\, so the kernel should deduplicate everything up to the first = encountered and leave the string as is when it has no = at all.

Is the general feeling to apply this sanitization only on an AT_SECURE/SUID boundary\, or always?

Always\, this has too great a chance of introducing variations in behaviour that can be unexpected\, and we never know right now what other tools could use the environment for restrictions in some time\, or even now.

bye\, //mirabilos -- Stéphane\, I actually don’t block Googlemail\, they’re just too utterly stupid to successfully deliver to me (or anyone else using Greylisting and not whitelisting their ranges). Same for a few other providers such as Hotmail. Some spammers (Yahoo) I do block.