Re: Telnetd Vulnerability Report

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.