[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0940bd56c70e5d93bc32024cca2b2ac9@risingedge.co.za>
Date: Sun, 08 Mar 2026 11:41:47 +0200
From: Justin Swartz <justin.swartz@...ingedge.co.za>
To: Solar Designer <solar@...nwall.com>
Cc: oss-security@...ts.openwall.com, bug-inetutils@....org,
collin.funk1@...il.com, simon@...efsson.org, auerswal@...x-ag.uni-kl.de,
ron.benyizhak@...ebreach.com
Subject: Re: Telnetd Vulnerability Report
On 2026-03-08 10:05, Solar Designer wrote:
> On Sun, Mar 08, 2026 at 09:34:22AM +0200, Justin Swartz wrote:
>> Based on the feedback provided, the third version of the patch set
>> [1]:
>
>> - Places the strings of the allowed environment variables array into
>> the .rodata section.
>
> Actually, the strings would be in .rodata (or after linking, in .text)
> with your previous patch version as well. It's the array of pointers
> that you're also moving to there now.
Thanks for clarifying that.
>> - Discards the --accept-env feature [3], as an inetutils maintainer
>> [2]
>> is working on an implementation to extend the allowed environment
>> using Gnulib instead.
>
> It sounds like one of you will have to rebase this on the other's work.
I'm leaving this up to the inetutils maintainers to take of, as I won't
have the spare time to contribute towards this going forward.
>> +extern int is_env_var_allowed (const char *var, const char *val);
>
> You shouldn't need to have this one extern now - you can make it
> static.
>
>> +#ifdef HAVE_PATHS_H
>> +# include <paths.h>
>> +#else
>> +# ifndef _PATH_DEFPATH
>> +# define _PATH_DEFPATH "/usr/bin:/bin"
>> +# endif
>> +#endif
>
> You shouldn't need this anymore.
You're right. I forgot to drop that when I got rid of exorcise_env().
>> +is_env_var_allowed (const char *var, const char *val)
>> +{
>> + const char * const *p;
>
> This second const here looks wrong as you're changing the value of this
> pointer. I suggested this syntax only for the array, where you used it
> correctly.
That pointer isn't constant.
######################################################################
## ##
## A test with the pointer declaration used in the previous patch ##
## ##
######################################################################
$ cat > p1.c << "EOF"
#include <stdio.h>
static const char * const allowed_env_vars[] = {
"USER", "LOGNAME", "TERM", "LANG", "LC_*", NULL
};
int main(void)
{
const char **p;
for (p = allowed_env_vars; *p; p++)
puts(*p);
putchar('\n');
return 0;
}
EOF
$ cc -o p1 p1.c -Wall -Werror -Wextra -pedantic
p.c: In function ‘main’:
p.c:11:16: error: assignment discards ‘const’ qualifier from pointer
target type [-Werror=discarded-qualifiers]
11 | for (p = allowed_env_vars; *p; p++)
| ^
cc1: all warnings being treated as errors
######################################################################
## ##
## A test with the pointer declaration used in the current patch ##
## ##
######################################################################
$ (echo '9s/\*\*p/\* const \*p/g'; echo ',p'; echo 'wq') | ed -s p.c
#include <stdio.h>
static const char * const allowed_env_vars[] = {
"USER", "LOGNAME", "TERM", "LANG", "LC_*", NULL
};
int main(void)
{
const char * const *p;
for (p = allowed_env_vars; *p; p++)
puts(*p);
putchar('\n');
return 0;
}
$ cc -o p p.c -Wall -Werror -Wextra -pedantic && echo $?
0
######################################################################
## ##
## A test with the pointer declaration that would be constant ##
## ##
######################################################################
$ (echo '9s/p;/ const p;/g'; echo ',p'; echo 'wq') | ed -s p.c
#include <stdio.h>
static const char * const allowed_env_vars[] = {
"USER", "LOGNAME", "TERM", "LANG", "LC_*", NULL
};
int main(void)
{
const char * const * const p;
for (p = allowed_env_vars; *p; p++)
puts(*p);
putchar('\n');
return 0;
}
$ cc -o p p.c -Wall -Werror -Wextra -pedantic
p.c: In function ‘main’:
p.c:11:16: error: assignment of read-only variable ‘p’
11 | for (p = allowed_env_vars; *p; p++)
| ^
p.c:11:41: error: increment of read-only variable ‘p’
11 | for (p = allowed_env_vars; *p; p++)
| ^~
>> +void
>> +set_env_var_if_allowed (const char *var, const char *val)
>> +{
>> + if (is_env_var_allowed (var, val))
>> + {
>> + if (val)
>> + {
>> + if (*val != 0)
>> + setenv (var, val, 1);
>> + }
>> + else
>> + {
>> + unsetenv (var);
>> + }
>> + }
>> +}
>
> I doubt it's desired behavior to retain the previous value of the env
> var if the new value is an empty string - or is it? If it is not, then
> I suggest either dropping the "*val != 0" check or moving it into
> "if (val && *val != 0)".
My thinking was: if a whitelisted variable was defined but featured
an empty value, then it might be safer to use whatever value it was
already assigned in the environment instead.
But given that RFC 1572 explicitly supports a client sending a
defined variable with no value, then maybe it's best not to second
guess the client and just let them clear it.
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
