On Sun, 8 Mar 2026 03:57:45 +0100, Solar Designer wrote: others have. It may well be that allowing those other env vars by 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 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 command."
The daemon now clears the inherited environment (preserving PATH and TERM, respectively, if present) before calling telnetdsetup(). LDPRELOAD=/lib64/libhardenedmalloc.so (although /etc/ld.so.preload is a more reliable way to do this when practical to do it globally).
exorciseenv() 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 NEWENVVAR: case ENVUSERVAR: ... } / end of case TELOPTNEWENVIRON /
Some code duplication here. Not new with these changes, but could be worth moving to a new function e.g. setenvvarifallowed(). Agreed. I'll implement setenvvarifallowed() instead. +/ A default whitelist for environment variables. / +static const char allowedenvvars[] = { + "USER", + "LOGNAME", + "TERM", + "LANG", + "LC", + NULL +};
Can make not only the strings but also the pointers const:
static const char const allowedenvvars[] = {
so that both may end up in a read-only section. OK, .rodata it is. +int +isenvvarallowed (const char var, const char val) +{ + const char p; + int allowed = 0; + + for (p = allowedenvvars; p; p++) + { + if (fnmatch (p, var, FNMNOESCAPE) == 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.