On Sun, 8 Mar 2026 03:57:45 +0100, Solar Designer wrote:
I'm not saying you should revise the list in any way - just sharing
what
others have. It may well be that allowing those other env vars by
default is obsolete since use cases for telnet are now more
specialized,
and maybe allowing LANG and LC_* is desirable for current use cases.
LANG and LC_* were cargo-culted in as they are honoured by OpenSSHd.
Separately note that I didn't check the *BSDs telnet _client_ (which I
think is still present in all *BSDs) for being (hopefully not) willing
to export arbitrary env vars. The maintainers could want to check
this.
And you could want to check the telnet client in InetUtils, now that we
know this package missed telnet[d] security fixes in general. This was
CVE-2005-0488 (and CVE-2005-1205 on Windows).
"Certain BSD-based Telnet clients, including those used on Solaris and
SuSE Linux, allow remote malicious Telnet servers to read sensitive
environment variables via the NEW-ENVIRON option with a SEND
ENV_USERVAR
command."
I don't mind poking at the InetUtils telnet client once I've done my
best
to leave the telnetd implementation in a better state than I found it
in.
> The daemon now clears the inherited environment (preserving PATH
> and TERM, respectively, if present) before calling telnetd_setup().
Inherited from inetd or the like? It's supposed to be trusted input
and
env vars in there may be set on purpose, so dropping them is
unexpected.
I think e.g. sshd doesn't do that, why would telnetd? Think things
like
LD_PRELOAD=/lib64/libhardened_malloc.so (although /etc/ld.so.preload is
a more reliable way to do this when practical to do it globally).
Yes, inherited from inetd. Your reasoning makes sense, so I'll get rid
of
exorcise_env() and leave the inetd/tcpd supplied environment intact for
telnetd (or some site-specific wrapper) to inherit.
> +++ b/telnetd/state.c
> @@ -1495,10 +1495,18 @@ suboption (void)
> case NEW_ENV_VAR:
> case ENV_USERVAR:
...
> } /* end of case TELOPT_NEW_ENVIRON */
Some code duplication here. Not new with these changes, but could be
worth moving to a new function e.g. set_env_var_if_allowed().
Agreed. I'll implement set_env_var_if_allowed() instead.
> +/* A default whitelist for environment variables. */
> +static const char *allowed_env_vars[] = {
> + "USER",
> + "LOGNAME",
> + "TERM",
> + "LANG",
> + "LC_*",
> + NULL
> +};
Can make not only the strings but also the pointers const:
static const char * const allowed_env_vars[] = {
so that both may end up in a read-only section.
OK, .rodata it is.
> +int
> +is_env_var_allowed (const char *var, const char *val)
> +{
> + const char **p;
> + int allowed = 0;
> +
> + for (p = allowed_env_vars; *p; p++)
> + {
> + if (fnmatch (*p, var, FNM_NOESCAPE) == 0)
> + {
> + allowed = 1;
> + break;
> + }
> + }
> +
> + if (!allowed)
> + return 0;
You didn't strictly need the "allowed" variable, you could check *p
after the loop. But maybe it's more readable the way you wrote it.
I'll keep "allowed" for the time being, but I don't mind changing it if
there's a clearer way to express this logic.
My review above isn't in full context - I only looked at the patches.
Thanks for the review, I'll submit a third version of this patch set
later.
