On Sat, Mar 07, 2026 at 02:20:11AM +0200, Justin Swartz wrote: WHITELISTING

The obsolete blacklist, implemented by scrubenv(), has been removed. The daemon now clears the inherited environment and enforces a default whitelist (USER, LOGNAME, TERM, LANG, and LC) for all NEWENVIRON values. Makes sense to me.

Note that this list is different from Linux NetKit's, which is:

/ Allow only these variables. / if (!strcmp(varp, "TERM")) return 1; if (!strcmp(varp, "DISPLAY")) return 1; if (!strcmp(varp, "USER")) return 1; if (!strcmp(varp, "LOGNAME")) return 1; if (!strcmp(varp, "POSIXLYCORRECT")) return 1;

I also checked the major BSDs. telnetd was removed from OpenBSD in 2005, so I didn't look further. It was removed from FreeBSD in 2022:

https://cgit.freebsd.org/src/commit/?id=d701f45aba19f232ce7817085935f33dd609ed8b

where it used this at time of removal:

static const char acc[] = { "XAUTH=", "XAUTHORITY=", "DISPLAY=", "TERM=", "EDITOR=", "PAGER=", "LOGNAME=", "POSIXLYCORRECT=", "PRINTER=", NULL };

Curiously, there was CVE-2009-0641 where FreeBSD 7.x would accept even LDPRELOAD, but I couldn't quickly find where the bug was exactly:

https://lists.openwall.net/full-disclosure/2009/02/14/1

Message-ID: <72f8221d0902131846h6c77a8d1t90c3352f978b8732 () mail gmail com> Date: Sat, 14 Feb 2009 03:46:07 +0100 From: Kingcope Kingcope <kcope2 () glemail com> To: full-disclosure () ts grok org uk Subject: FreeBSD zeroday

https://www.freebsd.org/security/advisories/FreeBSD-SA-09:05.telnetd.asc

"recent changes in FreeBSD's environment-handling code rendered telnetd's scrubbing inoperative"

We could want to find the detail in order to avoid the same pitfall.

NetBSD still has telnetd:

https://cvsweb.netbsd.org/bsdweb.cgi/src/libexec/telnetd/

and in systerm.c it has:

/ scrubenv() We only accept the environment variables listed below. /

void scrubenv(void) { static const char reject[] = { "TERMCAP=/", NULL };

static const char acceptstr[] = { "XAUTH=", "XAUTHORITY=", "DISPLAY=", "TERM=", "EDITOR=", "PAGER=", "LOGNAME=", "POSIXLYCORRECT=", "TERMCAP=", "PRINTER=", NULL };

char cpp, cpp2; const char p;

for (cpp2 = cpp = environ; cpp; cpp++) { int rejectit = 0;

for(p = reject; p; p++) if(strncmp(cpp, p, strlen(p)) == 0) { rejectit = 1; break; } if (rejectit) continue;

for(p = acceptstr; p; p++) if(strncmp(cpp, p, strlen(p)) == 0) break; if(p != NULL) cpp2++ = cpp; } cpp2 = NULL; }

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.

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 ENVUSERVAR command." TELOPTTTYPE INTERCEPTION

The whitelist validation has been extended, in the second version of the patch set, to intercept raw terminal type negotiations (aka TELOPTTTYPE), to prevent questionable TERM payloads from bypassing the NEWENVIRON filter. Good idea. The daemon now clears the inherited environment (preserving PATH and TERM, respectively, if present) before calling telnetdsetup(). 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 LDPRELOAD=/lib64/libhardenedmalloc.so (although /etc/ld.so.preload is a more reliable way to do this when practical to do it globally). +++ b/telnetd/state.c @@ -1495,10 +1495,18 @@ suboption (void) case NEWENVVAR: case ENVUSERVAR: cp = '\0'; - if (valp) - setenv (varp, valp, 1); - else - unsetenv (varp); + if (isenvvarallowed (varp, valp)) + { + if (valp) + { + if (valp && valp != 0) + setenv (varp, valp, 1); + } + else + { + unsetenv (varp); + } + } cp = varp = (char ) subpointer; valp = 0; break; @@ -1514,10 +1522,18 @@ suboption (void) } } cp = '\0'; - if (valp) - setenv (varp, valp, 1); - else - unsetenv (varp); + if (isenvvarallowed (varp, valp)) + { + if (valp) + { + if (valp && valp != 0) + setenv (varp, valp, 1); + } + else + { + unsetenv (varp); + } + } break; } / 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(). +/ 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. +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.

My review above isn't in full context - I only looked at the patches.

Alexander