svn commit: r199983 - in head: lib/libc/stdlib tools/regression/environ

[Brian Feldman]

Author: green
Date: Tue Dec 1 05:04:31 2009
New Revision: 199983
URL: http://svn.freebsd.org/changeset/base/199983

Log:
Do not gratuitously fail *env(3) operations due to corrupt ('='-less)
**environ entries. This puts non-getenv(3) operations in line with
getenv(3) in that bad environ entries do not cause all operations to
fail. There is still some inconsistency in that getenv(3) in the
absence of any environment-modifying operation does not emit corrupt
environ entry warnings.

I also fixed another inconsistency in getenv(3) where updating the
global environ pointer would not be reflected in the return values.
It would have taken an intermediary setenv(3)/putenv(3)/unsetenv(3)
in order to see the change.

Modified:
head/lib/libc/stdlib/getenv.c
head/tools/regression/environ/Makefile.envctl
head/tools/regression/environ/envctl.c
head/tools/regression/environ/envtest.t

Modified: head/lib/libc/stdlib/getenv.c
==============================================================================
--- head/lib/libc/stdlib/getenv.c Tue Dec 1 03:52:50 2009 (r199982)
+++ head/lib/libc/stdlib/getenv.c Tue Dec 1 05:04:31 2009 (r199983)
@@ -96,6 +96,8 @@ static int envVarsTotal = 0;

/* Deinitialization of new environment. */
static void __attribute__ ((destructor)) __clean_env_destructor(void);
+/* Resetting the environ pointer will affect the env functions. */
+static int __merge_environ(void);


/*
@@ -190,6 +192,9 @@ __findenv_environ(const char *name, size
{
int envNdx;

+ if (environ == NULL)
+ return (NULL);
+
/* Find variable within environ. */
for (envNdx = 0; environ[envNdx] != NULL; envNdx++)
if (strncmpeq(environ[envNdx], name, nameLen))
@@ -328,6 +333,7 @@ __build_env(void)
{
char **env;
int activeNdx;
+ int checking;
int envNdx;
int savedErrno;
size_t nameLen;
@@ -339,6 +345,23 @@ __build_env(void)
/* Count environment variables. */
for (env = environ, envVarsTotal = 0; *env != NULL; env++)
envVarsTotal++;
+ /* Remove any corrupt variable entries, but do not error out. */
+ checking = 0;
+ while (checking < envVarsTotal) {
+ if (strchr(environ[checking], '=') != NULL) {
+ checking++;
+ } else {
+ __env_warnx(CorruptEnvValueMsg,
+ environ[checking], strlen(environ[checking]));
+ /*
+ * Pull back all remaining entries from checking + 1
+ * through envVarsTotal, including the NULL at the end.
+ */
+ memmove(&environ[checking], &environ[checking + 1],
+ sizeof(char *) * (envVarsTotal - checking));
+ envVarsTotal--;
+ }
+ }
envVarsSize = envVarsTotal * 2;

/* Create new environment. */
@@ -353,18 +376,8 @@ __build_env(void)
strdup(environ[envVarsTotal - envNdx - 1]);
if (envVars[envNdx].name == NULL)
goto Failure;
- envVars[envNdx].value = strchr(envVars[envNdx].name, '=');
- if (envVars[envNdx].value != NULL) {
- envVars[envNdx].value++;
- envVars[envNdx].valueSize =
- strlen(envVars[envNdx].value);
- } else {
- __env_warnx(CorruptEnvValueMsg, envVars[envNdx].name,
- strlen(envVars[envNdx].name));
- errno = EFAULT;
- goto Failure;
- }
-
+ envVars[envNdx].value = strchr(envVars[envNdx].name, '=') + 1;
+ envVars[envNdx].valueSize = strlen(envVars[envNdx].value);
/*
* Find most current version of variable to make active. This
* will prevent multiple active variables from being created
@@ -426,22 +439,18 @@ getenv(const char *name)
}

/*
- * An empty environment (environ or its first value) regardless if
- * environ has been copied before will return a NULL.
- *
- * If the environment is not empty, find an environment variable via
- * environ if environ has not been copied via an *env() call or been
- * replaced by a running program, otherwise, use the rebuilt
- * environment.
+ * If we have not already allocated memory by performing
+ * write operations on the environment, avoid doing so now.
*/
- if (environ == NULL || environ[0] == NULL)
- return (NULL);
- else if (envVars == NULL || environ != intEnviron)
+ if (envVars == NULL)
return (__findenv_environ(name, nameLen));
- else {
- envNdx = envVarsTotal - 1;
- return (__findenv(name, nameLen, &envNdx, true));
- }
+
+ /* Synchronize environment. */
+ if (__merge_environ() == -1)
+ return (NULL);
+
+ envNdx = envVarsTotal - 1;
+ return (__findenv(name, nameLen, &envNdx, true));
}


@@ -559,8 +568,7 @@ __merge_environ(void)
if ((equals = strchr(*env, '=')) == NULL) {
__env_warnx(CorruptEnvValueMsg, *env,
strlen(*env));
- errno = EFAULT;
- return (-1);
+ continue;
}
if (__setenv(*env, equals - *env, equals + 1,
1) == -1)

Modified: head/tools/regression/environ/Makefile.envctl
==============================================================================
--- head/tools/regression/environ/Makefile.envctl Tue Dec 1 03:52:50 2009 (r199982)
+++ head/tools/regression/environ/Makefile.envctl Tue Dec 1 05:04:31 2009 (r199983)
@@ -13,4 +13,4 @@ NO_MAN= yes
.include <bsd.prog.mk>

test: ${PROG}
- @sh envtest.t
+ @env -i sh envtest.t

Modified: head/tools/regression/environ/envctl.c
==============================================================================
--- head/tools/regression/environ/envctl.c Tue Dec 1 03:52:50 2009 (r199982)
+++ head/tools/regression/environ/envctl.c Tue Dec 1 05:04:31 2009 (r199983)
@@ -60,7 +60,7 @@ dump_environ(void)
static void
usage(const char *program)
{
- fprintf(stderr, "Usage: %s [-DGUchrt] [-c 1|2|3|4] [-gu name] "
+ fprintf(stderr, "Usage: %s [-DGUchrt] [-c 1|2|3|4] [-bgu name] "
"[-p name=value]\n"
"\t[(-S|-s name) value overwrite]\n\n"
"Options:\n"
@@ -68,6 +68,7 @@ usage(const char *program)
" -G name\t\t\tgetenv(NULL)\n"
" -S value overwrite\t\tsetenv(NULL, value, overwrite)\n"
" -U\t\t\t\tunsetenv(NULL)\n"
+ " -b name\t\t\tblank the 'name=$name' entry, corrupting it\n"
" -c 1|2|3|4\t\t\tClear environ variable using method:\n"
"\t\t\t\t1 - set environ to NULL pointer\n"
"\t\t\t\t2 - set environ[0] to NULL pointer\n"
@@ -98,6 +99,28 @@ print_rtrn_errno(int rtrnVal, const char
return;
}

+static void
+blank_env(const char *var)
+{
+ char **newenviron;
+ int n, varlen;
+
+ if (environ == NULL)
+ return;
+
+ for (n = 0; environ[n] != NULL; n++)
+ ;
+ newenviron = malloc(sizeof(char *) * (n + 1));
+ varlen = strlen(var);
+ for (; n >= 0; n--) {
+ newenviron[n] = environ[n];
+ if (newenviron[n] != NULL &&
+ strncmp(newenviron[n], var, varlen) == 0 &&
+ newenviron[n][varlen] == '=')
+ newenviron[n] += strlen(newenviron[n]);
+ }
+ environ = newenviron;
+}

int
main(int argc, char **argv)
@@ -114,8 +137,12 @@ main(int argc, char **argv)
}

/* The entire program is basically executed from this loop. */
- while ((arg = getopt(argc, argv, "DGS:Uc:g:hp:rs:tu:")) != -1) {
+ while ((arg = getopt(argc, argv, "DGS:Ub:c:g:hp:rs:tu:")) != -1) {
switch (arg) {
+ case 'b':
+ blank_env(optarg);
+ break;
+
case 'c':
switch (atoi(optarg)) {
case 1:

Modified: head/tools/regression/environ/envtest.t
==============================================================================
--- head/tools/regression/environ/envtest.t Tue Dec 1 03:52:50 2009 (r199982)
+++ head/tools/regression/environ/envtest.t Tue Dec 1 05:04:31 2009 (r199983)
@@ -232,3 +232,18 @@ check_result "${BAR} 0 0 ${BAR} 0 0 ${NU

run_test -r -g FOO -u FOO -g FOO -s FOO ${BAR} 1 -g FOO
check_result "${BAR} 0 0 ${NULL} 0 0 ${BAR}"
+
+
+# corruption (blanking) of environ members.
+export BLANK_ME=
+export AFTER_BLANK=blanked
+run_test -b BLANK_ME -p MORE=vars -g FOO -g BLANK_ME -g AFTER_BLANK
+check_result "0 0 ${FOO} ${NULL} ${AFTER_BLANK}"
+
+run_test -b BLANK_ME -u FOO -g FOO -g AFTER_BLANK
+check_result "0 0 ${NULL} ${AFTER_BLANK}"
+
+export BLANK_ME2=
+export AFTER_BLANKS=blankD
+run_test -b BLANK_ME -b AFTER_BLANK -b BLANK_ME2 -g FOO -g AFTER_BLANKS
+check_result "${FOO} ${AFTER_BLANKS}"

[Colin Percival]

Post by Brian Feldman
Do not gratuitously fail *env(3) operations due to corrupt ('='-less)
**environ entries. This puts non-getenv(3) operations in line with
getenv(3) in that bad environ entries do not cause all operations to
fail. There is still some inconsistency in that getenv(3) in the
absence of any environment-modifying operation does not emit corrupt
environ entry warnings.
I also fixed another inconsistency in getenv(3) where updating the
global environ pointer would not be reflected in the return values.
It would have taken an intermediary setenv(3)/putenv(3)/unsetenv(3)
in order to see the change.

The FreeBSD Security Team is currently dealing with a security issue relating
to this code. Please back out your change (at least to getenv.c; I don't
particularly care about the regression tests) until we've finished, and then
submit the patch to us for review along with a detailed explanation of what
it does.

We've already had two major security issues arising out of getenv.c in the
past year, and I'd like to make sure we don't have a third.

[Robert Watson]

Post by Colin Percival

Post by Brian Feldman
Do not gratuitously fail *env(3) operations due to corrupt ('='-less)
**environ entries. This puts non-getenv(3) operations in line with
getenv(3) in that bad environ entries do not cause all operations to
fail. There is still some inconsistency in that getenv(3) in the
absence of any environment-modifying operation does not emit corrupt
environ entry warnings.
I also fixed another inconsistency in getenv(3) where updating the
global environ pointer would not be reflected in the return values.
It would have taken an intermediary setenv(3)/putenv(3)/unsetenv(3)
in order to see the change.

The FreeBSD Security Team is currently dealing with a security issue
relating to this code. Please back out your change (at least to getenv.c; I
don't particularly care about the regression tests) until we've finished,
and then submit the patch to us for review along with a detailed explanation
of what it does.
We've already had two major security issues arising out of getenv.c in the
past year, and I'd like to make sure we don't have a third.

I think it's fair to say that the POSIXization of the environment code has
been an unmitigated disaster, and speaks to the necessity for careful review
of those sorts of code changes.

Robert N M Watson
Computer Laboratory
University of Cambridge

[Sean C. Farley]

*snip*

Post by Robert Watson

Post by Colin Percival
We've already had two major security issues arising out of getenv.c
in the past year, and I'd like to make sure we don't have a third.

I think it's fair to say that the POSIXization of the environment code
has been an unmitigated disaster, and speaks to the necessity for
careful review of those sorts of code changes.

As the author of the environment code, I agree that it has been a
painful process.

Interestingly, the security issue was a combination of r169661 to
rtld.c, which is a correct action, and the new environ code which was
developed, as opposed to committed, at the same time. Separately, the
security issue would not have existed.

Sean

[Robert N. M. Watson]

We've already had two major security issues arising out of getenv.c in the past year, and I'd like to make sure we don't have a third.

I think it's fair to say that the POSIXization of the environment code has been an unmitigated disaster, and speaks to the necessity for careful review of those sorts of code changes.

As the author of the environment code, I agree that it has been a painful process.
Interestingly, the security issue was a combination of r169661 to rtld.c, which is a correct action, and the new environ code which was developed, as opposed to committed, at the same time. Separately, the security issue would not have existed.

One immediately pressing question is whether we can mitigate future possible problems along the same lines. The obvious thing is a further (and very careful) audit if all environmental variable use in the base system. But I wonder if there are some other things we could do, such as:

- libc environment scrubbing: try to be more robust in the presence of the unexpected (for example, if you find corrupted stuff, ignore it more robustly); another variation might be to have libc abort(2) if issetugid() and unsetenv(3) would fail.
- kernel environment scrubbing: the kernel is already responsible for getting those variables across the execve(2) boundary, so is already copying (and to a lesser extent, validating) it, and could learn to be a bit more rigorous in its expectations, perhaps more so for security-sensitive transitions (setuid/setgid/MAC/...)

Brian's changes, although poorly timed, seem like a reasonable direction in this regard: we're stuck with unhelpful APIs, but maybe we can do a better job.

Robert

[Sean C. Farley]

Post by Robert N. M. Watson

Post by Robert Watson
I think it's fair to say that the POSIXization of the environment
code has been an unmitigated disaster, and speaks to the necessity
for careful review of those sorts of code changes.

As the author of the environment code, I agree that it has been a painful process.
Interestingly, the security issue was a combination of r169661 to
rtld.c, which is a correct action, and the new environ code which was
developed, as opposed to committed, at the same time. Separately,
the security issue would not have existed.

One immediately pressing question is whether we can mitigate future
possible problems along the same lines. The obvious thing is a further
(and very careful) audit if all environmental variable use in the base
system. But I wonder if there are some other things we could do, such
- libc environment scrubbing: try to be more robust in the presence of
the unexpected (for example, if you find corrupted stuff, ignore it
more robustly); another variation might be to have libc abort(2) if
issetugid() and unsetenv(3) would fail.

The preliminary patch I sent earlier should at least make the calls
behave more like they used to do (go through each variable even if
corrupt). However, I do agree that more code (getenv.c and any code
that calls into it) needs to be verified for more paranoid use of the
environment.

As for abort(), I was/still am considering having that be the result of
a corrupt environ array. If it is corrupt, why attempt to use it?
unsetenv() may still fail, so it may not abort() for other scenarios.

Post by Robert N. M. Watson
- kernel environment scrubbing: the kernel is already responsible for
getting those variables across the execve(2) boundary, so is already
copying (and to a lesser extent, validating) it, and could learn to be
a bit more rigorous in its expectations, perhaps more so for
security-sensitive transitions (setuid/setgid/MAC/...)

That is a good point. I had not thought about kernel validation of the
environment in addition to the validation performed in libc.

Post by Robert N. M. Watson
Brian's changes, although poorly timed, seem like a reasonable
direction in this regard: we're stuck with unhelpful APIs, but maybe
we can do a better job.

Getting rid of putenv() and especially removing direct access of environ
(replaced with API call(s)) would be my favorite API changes.

Sean

[Brian Fundakowski Feldman]

Post by Robert N. M. Watson

We've already had two major security issues arising out of getenv.c in the past year, and I'd like to make sure we don't have a third.

I think it's fair to say that the POSIXization of the environment code has been an unmitigated disaster, and speaks to the necessity for careful review of those sorts of code changes.

As the author of the environment code, I agree that it has been a painful process.
Interestingly, the security issue was a combination of r169661 to rtld.c, which is a correct action, and the new environ code which was developed, as opposed to committed, at the same time. Separately, the security issue would not have existed.

- libc environment scrubbing: try to be more robust in the presence of the unexpected (for example, if you find corrupted stuff, ignore it more robustly); another variation might be to have libc abort(2) if issetugid() and unsetenv(3) would fail.

An abort() makes more sense than an error return where no error is ever
expected (i.e. expecting unsetenv(3) to never fail is quite reasonable).

Post by Robert N. M. Watson
- kernel environment scrubbing: the kernel is already responsible for getting those variables across the execve(2) boundary, so is already copying (and to a lesser extent, validating) it, and could learn to be a bit more rigorous in its expectations, perhaps more so for security-sensitive transitions (setuid/setgid/MAC/...)
Brian's changes, although poorly timed, seem like a reasonable direction in this regard: we're stuck with unhelpful APIs, but maybe we can do a better job.

Do you see a clear advantage to scrubbing environment in the kernel versus libc?
The libc environ functions will still need to be able to do it upon change
of the environ pointer, so there's some duplication of functionality; but then
again there may be iffy environ-using logic in things you run which are not
using the /lib/libc.so.

[M. Warner Losh]

In message: <***@fledge.watson.org>
Robert Watson <***@FreeBSD.org> writes:
: On Mon, 30 Nov 2009, Colin Percival wrote:
:
: > Brian Feldman wrote:
: >> Do not gratuitously fail *env(3) operations due to corrupt ('='-less)
: >> **environ entries. This puts non-getenv(3) operations in line with
: >> getenv(3) in that bad environ entries do not cause all operations to
: >> fail. There is still some inconsistency in that getenv(3) in the
: >> absence of any environment-modifying operation does not emit corrupt
: >> environ entry warnings.
: >>
: >> I also fixed another inconsistency in getenv(3) where updating the
: >> global environ pointer would not be reflected in the return values.
: >> It would have taken an intermediary setenv(3)/putenv(3)/unsetenv(3)
: >> in order to see the change.
: >
: > The FreeBSD Security Team is currently dealing with a security issue
: > relating to this code. Please back out your change (at least to getenv.c; I
: > don't particularly care about the regression tests) until we've finished,
: > and then submit the patch to us for review along with a detailed explanation
: > of what it does.
: >
: > We've already had two major security issues arising out of getenv.c in the
: > past year, and I'd like to make sure we don't have a third.
:
: I think it's fair to say that the POSIXization of the environment code has
: been an unmitigated disaster, and speaks to the necessity for careful review
: of those sorts of code changes.

Why we're not just reverting the whole thing as a bad idea is beyond
me. Clearly the tiny incremental benefits have been far overshadowed
by this fiasco.

Warner

[Sean C. Farley]

Post by M. Warner Losh
: >> Do not gratuitously fail *env(3) operations due to corrupt ('='-less)
: >> **environ entries. This puts non-getenv(3) operations in line with
: >> getenv(3) in that bad environ entries do not cause all operations to
: >> fail. There is still some inconsistency in that getenv(3) in the
: >> absence of any environment-modifying operation does not emit corrupt
: >> environ entry warnings.
: >>
: >> I also fixed another inconsistency in getenv(3) where updating the
: >> global environ pointer would not be reflected in the return values.
: >> It would have taken an intermediary setenv(3)/putenv(3)/unsetenv(3)
: >> in order to see the change.
: >
: > The FreeBSD Security Team is currently dealing with a security
: > issue relating to this code. Please back out your change (at
: > least to getenv.c; I don't particularly care about the regression
: > tests) until we've finished, and then submit the patch to us for
: > review along with a detailed explanation of what it does.
: >
: > We've already had two major security issues arising out of
: > getenv.c in the past year, and I'd like to make sure we don't have
: > a third.
: I think it's fair to say that the POSIXization of the environment
: code has been an unmitigated disaster, and speaks to the necessity
: for careful review of those sorts of code changes.
Why we're not just reverting the whole thing as a bad idea is beyond
me. Clearly the tiny incremental benefits have been far overshadowed
by this fiasco.

Which "whole thing"? The code or the POSIX-compliance? Technically, it
is not pure compliance because the code has a few BSD requirements in it
such as keeping old name=value entries even when new ones are created.

It was my fault for not checking how unsetenv() was used in all of base.
The change to use unsetenv() in rtld.c was committed just prior to my
change which introduced a version of unsetenv() that returned an int to
allow checking.

I am testing a change to have unsetenv() not stop in its attempt to
unset a variable which should mimic the old behavior.

One difference between our man page and IEEE Std 1003.1-2008 is the part
concerning that the "environment shall be unchanged" which I will
introduce to the man page:

Upon successful completion, zero shall be returned. Otherwise, -1
shall be returned, errno set to indicate the error, and the
environment shall be unchanged.

After my change, unsetenv() will not return an error if environ is
corrupt. It will wipe the variable. The only time errors will be
returned are when the name passed to unsetenv() is invalid or when
memory allocation fails.

One question is whether the code should abort() when it detects a
corrupt environ array or do its best to complete the request from the
caller.

Sean

[Brian Fundakowski Feldman]

Post by M. Warner Losh
: >> Do not gratuitously fail *env(3) operations due to corrupt ('='-less)
: >> **environ entries. This puts non-getenv(3) operations in line with
: >> getenv(3) in that bad environ entries do not cause all operations to
: >> fail. There is still some inconsistency in that getenv(3) in the
: >> absence of any environment-modifying operation does not emit corrupt
: >> environ entry warnings.
: >>
: >> I also fixed another inconsistency in getenv(3) where updating the
: >> global environ pointer would not be reflected in the return values.
: >> It would have taken an intermediary setenv(3)/putenv(3)/unsetenv(3)
: >> in order to see the change.
: >
: > The FreeBSD Security Team is currently dealing with a security : >
issue relating to this code. Please back out your change (at : > least to
getenv.c; I don't particularly care about the regression : > tests) until
we've finished, and then submit the patch to us for : > review along with
a detailed explanation of what it does.
: >
: > We've already had two major security issues arising out of : >
getenv.c in the past year, and I'd like to make sure we don't have : > a
third.
: I think it's fair to say that the POSIXization of the environment : code
has been an unmitigated disaster, and speaks to the necessity : for
careful review of those sorts of code changes.
Why we're not just reverting the whole thing as a bad idea is beyond me.
Clearly the tiny incremental benefits have been far overshadowed by this
fiasco.

Which "whole thing"? The code or the POSIX-compliance? Technically, it is
not pure compliance because the code has a few BSD requirements in it such
as keeping old name=value entries even when new ones are created.
It was my fault for not checking how unsetenv() was used in all of base.
The change to use unsetenv() in rtld.c was committed just prior to my
change which introduced a version of unsetenv() that returned an int to
allow checking.
I am testing a change to have unsetenv() not stop in its attempt to unset a
variable which should mimic the old behavior.
One difference between our man page and IEEE Std 1003.1-2008 is the part
concerning that the "environment shall be unchanged" which I will introduce
Upon successful completion, zero shall be returned. Otherwise, -1
shall be returned, errno set to indicate the error, and the
environment shall be unchanged.
After my change, unsetenv() will not return an error if environ is corrupt.
It will wipe the variable. The only time errors will be returned are when
the name passed to unsetenv() is invalid or when memory allocation fails.
One question is whether the code should abort() when it detects a corrupt
environ array or do its best to complete the request from the caller.

The consensus seems to be that either of those two behaviors is okay. I lean
more heavily toward repairing environ and moving on because it seems likely
that as often as not, *env(3) will not occur immediately after (unintentional)
environ corruption and abort() won't make debugging much easier, only make
lives harder. Of course, we could also see what Solaris, Linux, NetBSD and
friends do...

Using abort() and assert() against data provided directly by the user
(that is, data that is not supposed to be already "owned" by the library)
is somewhat evil. It annoys me to no end as a developer when libraries
do that (one of the guiltiest examples being libdbus.)

[M. Warner Losh]

In message: <***@thor.farley.org>
"Sean C. Farley" <***@FreeBSD.org> writes:
: On Tue, 1 Dec 2009, M. Warner Losh wrote:
:
: > In message: <***@fledge.watson.org>
: > Robert Watson <***@FreeBSD.org> writes:
: > : On Mon, 30 Nov 2009, Colin Percival wrote:
: > :
: > : > Brian Feldman wrote:
: > : >> Do not gratuitously fail *env(3) operations due to corrupt ('='-less)
: > : >> **environ entries. This puts non-getenv(3) operations in line with
: > : >> getenv(3) in that bad environ entries do not cause all operations to
: > : >> fail. There is still some inconsistency in that getenv(3) in the
: > : >> absence of any environment-modifying operation does not emit corrupt
: > : >> environ entry warnings.
: > : >>
: > : >> I also fixed another inconsistency in getenv(3) where updating the
: > : >> global environ pointer would not be reflected in the return values.
: > : >> It would have taken an intermediary setenv(3)/putenv(3)/unsetenv(3)
: > : >> in order to see the change.
: > : >
: > : > The FreeBSD Security Team is currently dealing with a security
: > : > issue relating to this code. Please back out your change (at
: > : > least to getenv.c; I don't particularly care about the regression
: > : > tests) until we've finished, and then submit the patch to us for
: > : > review along with a detailed explanation of what it does.
: > : >
: > : > We've already had two major security issues arising out of
: > : > getenv.c in the past year, and I'd like to make sure we don't have
: > : > a third.
: > :
: > : I think it's fair to say that the POSIXization of the environment
: > : code has been an unmitigated disaster, and speaks to the necessity
: > : for careful review of those sorts of code changes.
: >
: > Why we're not just reverting the whole thing as a bad idea is beyond
: > me. Clearly the tiny incremental benefits have been far overshadowed
: > by this fiasco.
:
: Which "whole thing"? The code or the POSIX-compliance? Technically, it
: is not pure compliance because the code has a few BSD requirements in it
: such as keeping old name=value entries even when new ones are created.

I'm calling for something fairly radical: Go back to the code that was
working before and abandon all this POSIX code. If someone wants to
reimplement it correctly, securely and audits the system to prove it,
then it can go back in. We've had two black eyes from the current
code and I have little confidence that all the subtle problems have
been resolved with it. My gut tells me there are more lurking,
although that can be hard to quantify into an actionable item.

I don't think there's much support for this position, so the next best
thing is what consensus appears to be calling for: fix the current
code in a fail-safe manner.

Warner

[Brian Fundakowski Feldman]

Post by Brian Feldman
Author: green
Date: Tue Dec 1 05:04:31 2009
New Revision: 199983
URL: http://svn.freebsd.org/changeset/base/199983
Do not gratuitously fail *env(3) operations due to corrupt ('='-less)
**environ entries. This puts non-getenv(3) operations in line with
getenv(3) in that bad environ entries do not cause all operations to
fail. There is still some inconsistency in that getenv(3) in the
absence of any environment-modifying operation does not emit corrupt
environ entry warnings.
I also fixed another inconsistency in getenv(3) where updating the
global environ pointer would not be reflected in the return values.
It would have taken an intermediary setenv(3)/putenv(3)/unsetenv(3)
in order to see the change.

^^^
I'm apparently pretty sleepy, because I had forgotten what the second half
of the change was for :P

It makes getenv(3) capable of emitting the corrupted environ warnings.
However, this is still avoided in the no-memory-allocation scenario.

[Andrey Chernov]

Post by Brian Feldman
- if (environ == NULL || environ[0] == NULL)
- return (NULL);
- else if (envVars == NULL || environ != intEnviron)
+ if (envVars == NULL)
return (__findenv_environ(name, nameLen));
- else {
- envNdx = envVarsTotal - 1;
- return (__findenv(name, nameLen, &envNdx, true));
- }
+
+ /* Synchronize environment. */
+ if (__merge_environ() == -1)
+ return (NULL);
+
+ envNdx = envVarsTotal - 1;
+ return (__findenv(name, nameLen, &envNdx, true));
}

__merge_environ() should be avoided here for speed reasons.

[Brian Fundakowski Feldman]

Post by Andrey Chernov

Post by Brian Feldman
- if (environ == NULL || environ[0] == NULL)
- return (NULL);
- else if (envVars == NULL || environ != intEnviron)
+ if (envVars == NULL)
return (__findenv_environ(name, nameLen));
- else {
- envNdx = envVarsTotal - 1;
- return (__findenv(name, nameLen, &envNdx, true));
- }
+
+ /* Synchronize environment. */
+ if (__merge_environ() == -1)
+ return (NULL);
+
+ envNdx = envVarsTotal - 1;
+ return (__findenv(name, nameLen, &envNdx, true));
}

__merge_environ() should be avoided here for speed reasons.

If the corrupt environment warnings are not actually that useful, then
I agree that there's no other reason not to defer __merge_environ().

I actually wanted to have getenv(3) produce the warnings even if one of
the other (modification) functions was never called, but that seemed
impossible to do correctly without also having it call __build_environ().

The warnings I don't care much about, but having getenv(3) treat the
environment as sane when setenv(3)/unsetenv(3)/putenv(3) operations
are indefinitely failing is wrong.

[Sean C. Farley]

Post by Brian Feldman
Author: green
Date: Tue Dec 1 05:04:31 2009
New Revision: 199983
URL: http://svn.freebsd.org/changeset/base/199983
Do not gratuitously fail *env(3) operations due to corrupt ('='-less)
**environ entries. This puts non-getenv(3) operations in line with
getenv(3) in that bad environ entries do not cause all operations to
fail. There is still some inconsistency in that getenv(3) in the
absence of any environment-modifying operation does not emit corrupt
environ entry warnings.
I also fixed another inconsistency in getenv(3) where updating the
global environ pointer would not be reflected in the return values.
It would have taken an intermediary setenv(3)/putenv(3)/unsetenv(3)
in order to see the change.

A simpler patch[1] is to have the build and merge routines skip unusable
environ entries and continue to the next entry. It still can return an
error due to memory allocation problems, but those do not necessarily
reflect a corrupt environ array.

I want to test a few more things first.

Sean
1. http://people.freebsd.org/~scf/getenv-1.patch

[Sean C. Farley]

Post by Brian Feldman
I also fixed another inconsistency in getenv(3) where updating the
global environ pointer would not be reflected in the return values.
It would have taken an intermediary setenv(3)/putenv(3)/unsetenv(3)
in order to see the change.

In the current code, if environ is replaced or none of the set/put/unset
calls have been made, getenv() will use __findenv_environ() (searches
environ directly) to find the entry. This is necessary since malloc()
depends upon getenv() creating a cross-dependency.

Post by Brian Feldman
@@ -426,22 +439,18 @@ getenv(const char *name)
}
/*
- * An empty environment (environ or its first value) regardless if
- * environ has been copied before will return a NULL.
- *
- * If the environment is not empty, find an environment variable via
- * environ if environ has not been copied via an *env() call or been
- * replaced by a running program, otherwise, use the rebuilt
- * environment.
+ * If we have not already allocated memory by performing
+ * write operations on the environment, avoid doing so now.
*/
- if (environ == NULL || environ[0] == NULL)
- return (NULL);
- else if (envVars == NULL || environ != intEnviron)
+ if (envVars == NULL)
return (__findenv_environ(name, nameLen));
- else {
- envNdx = envVarsTotal - 1;
- return (__findenv(name, nameLen, &envNdx, true));
- }
+
+ /* Synchronize environment. */
+ if (__merge_environ() == -1)
+ return (NULL);
+
+ envNdx = envVarsTotal - 1;
+ return (__findenv(name, nameLen, &envNdx, true));
}

Sean

[Brian Fundakowski Feldman]

Post by Sean C. Farley

Post by Brian Feldman
I also fixed another inconsistency in getenv(3) where updating the
global environ pointer would not be reflected in the return values.
It would have taken an intermediary setenv(3)/putenv(3)/unsetenv(3)
in order to see the change.

In the current code, if environ is replaced or none of the set/put/unset
calls have been made, getenv() will use __findenv_environ() (searches
environ directly) to find the entry. This is necessary since malloc()
depends upon getenv() creating a cross-dependency.

Could you replace the (quoted) comment wholesale with something to that effect?

Post by Sean C. Farley

Post by Brian Feldman
@@ -426,22 +439,18 @@ getenv(const char *name)
}
/*
- * An empty environment (environ or its first value) regardless if
- * environ has been copied before will return a NULL.
- *
- * If the environment is not empty, find an environment variable via
- * environ if environ has not been copied via an *env() call or been
- * replaced by a running program, otherwise, use the rebuilt
- * environment.
+ * If we have not already allocated memory by performing
+ * write operations on the environment, avoid doing so now.
*/
- if (environ == NULL || environ[0] == NULL)
- return (NULL);
- else if (envVars == NULL || environ != intEnviron)
+ if (envVars == NULL)
return (__findenv_environ(name, nameLen));
- else {
- envNdx = envVarsTotal - 1;
- return (__findenv(name, nameLen, &envNdx, true));
- }
+
+ /* Synchronize environment. */
+ if (__merge_environ() == -1)
+ return (NULL);
+
+ envNdx = envVarsTotal - 1;
+ return (__findenv(name, nameLen, &envNdx, true));
}