[PATCH] nis: nis_local_group may read from __nisgroup[-1] (bug 28075)

[PATCH] nis: nis_local_group may read from __nisgroup[-1] (bug 28075)

[Florian Weimer via Libc-alpha]

Tested on i686-linux-gnu and x86_64-linux-gnu.

---
 nis/nis_local_names.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/nis/nis_local_names.c b/nis/nis_local_names.c
index 080dcf88bd..218764f32e 100644
--- a/nis/nis_local_names.c
+++ b/nis/nis_local_names.c
@@ -31,6 +31,7 @@ nis_local_group (void)
   char *cptr;
   if (__nisgroup[0] == '\0'
       && (cptr = getenv ("NIS_GROUP")) != NULL
+      && cptr[0] != '\0'
       && strlen (cptr) < NIS_MAXNAMELEN)
     {
       char *cp = stpcpy (__nisgroup, cptr);

Re: [PATCH] nis: nis_local_group may read from __nisgroup[-1] (bug 28075)

[Carlos O'Donell via Libc-alpha]

On 7/11/21 10:35 AM, Florian Weimer via Libc-alpha wrote:
> Tested on i686-linux-gnu and x86_64-linux-gnu.

In nis/* there are 6 uses of getenv.

The other users: __nis_default_owner, __nis_default_group,
__nis_default_ttl, __nis_default_access, all use strstr to
determine the position of parameters and don't suffer the
same problem (empty env var), likewise nis_getnames but with
__srtok_r. The code you are fixing doesn't appear to consider
an empty env var. I also looked up the call chain to see if
nis_clone_object inspected the group to see if it was empty
and it does, and so it appears safe.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
 
> ---
>  nis/nis_local_names.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/nis/nis_local_names.c b/nis/nis_local_names.c
> index 080dcf88bd..218764f32e 100644
> --- a/nis/nis_local_names.c
> +++ b/nis/nis_local_names.c
> @@ -31,6 +31,7 @@ nis_local_group (void)
>    char *cptr;
>    if (__nisgroup[0] == '\0'
>        && (cptr = getenv ("NIS_GROUP")) != NULL
> +      && cptr[0] != '\0'
>        && strlen (cptr) < NIS_MAXNAMELEN)
>      {
>        char *cp = stpcpy (__nisgroup, cptr);
> 


-- 
Cheers,
Carlos.