Re: [PATCH v2 2/2] telnetd: add optional --accept-env feature to extend the whitelist.

Hi Justin,

Thank you very much for your work investigating this issue and the
proposed patches.

Justin Swartz <[email protected]> writes:

> +#ifdef ENABLE_ACCEPT_ENV
> +  {"accept-env", 'W', "PATTERN", 0,
> +   "specify an additional environment variable pattern to accept", GRID},
> +#endif

I dislike adding short options that are not supported by other
implementations. In coreutils, we have a few short options that conflict
with other implementations. I.e., they have the same option but with
different behavior. Those are always a bit annoying since they are
another portability headache people have to deal with when writing
scripts.

> +#ifdef ENABLE_ACCEPT_ENV
> +    case 'W':
> +      add_allowed_env_pattern (arg);
> +      break;
> +#endif
> +

Usually the '--enable-*' options are moreso for enabling additional
features. The GNU Coding standards says [1]:

    No =E2=80=98--enable=E2=80=99 option should ever cause one feature to r=
eplace
    another.

So, I don't think this should be gated behind a './configure --option'.

> @@ -90,15 +96,30 @@ is_env_var_allowed (const char *var, const char *val)
>  {
>    const char **p;
>    int allowed =3D 0;
> +#ifdef ENABLE_ACCEPT_ENV
> +  size_t i;
>=20=20
> -  for (p =3D allowed_env_vars; *p; p++)
> +  for (i =3D 0; i < user_env_var_count; i++)
>      {
> -      if (fnmatch (*p, var, FNM_NOESCAPE) =3D=3D 0)
> +      if (fnmatch (user_env_vars[i], var, FNM_NOESCAPE) =3D=3D 0)
>          {
>            allowed =3D 1;
>            break;
>          }
>      }

This would need the addition of the 'fnmatch' Gnulib module to work
around some platform bugs [2].

However, I would prefer avoiding wildcards, etc. and force invocations
to be explicit about what they would like to accept.

> +#ifdef ENABLE_ACCEPT_ENV
> +void
> +add_allowed_env_pattern (const char *pattern)
> +{
> +  if (!pattern || *pattern =3D=3D 0)
> +    return;
> +
> +  if (user_env_var_count >=3D MAX_USER_ENV_VARS)
> +    {
> +      syslog (LOG_NOTICE, "Ignoring --accept-env option: limit reached");
> +      return;
> +    }
> +
> +  user_env_vars[user_env_var_count++] =3D pattern;
> +}
> +#endif
> +

Most GNU programs try very hard to avoid arbitrary limits [3]. Some
users may want many more than 16 environment variables accepted.

I wrote a seperate patch that avoids that issue [4], and also avoids
clearenv which is missing on many platforms [5].

We can just do:

    static char *dummy_environ[] =3D { NULL };
    environ =3D dummy_environ;

It is technically undefined behavior. But GNU coreutils has used it
without problems for decades in 'env' without issue. :)

Collin

[1] https://www.gnu.org/prep/standards/standards.html#How-Configuration-Sho=
uld-Work
[2] https://www.gnu.org/software/gnulib/manual/html_node/fnmatch.html
[3] https://www.gnu.org/prep/standards/standards.html#Writing-Robust-Progra=
ms
[4] https://codeberg.org/inetutils/inetutils/pulls/15
[5] https://www.gnu.org/software/gnulib/manual/html_node/clearenv.html