[V6] nss_db: protect against empty mappings

Florian Weimer <fweimer@redhat.com> writes:
> Just print the return value, then?

Like this?

From 4d5ff68bf777ec0ba4594ec7b46403bff34086b2 Mon Sep 17 00:00:00 2001
From: DJ Delorie <dj@redhat.com>
Date: Mon, 17 Jun 2019 15:33:27 -0400
Subject: nss_db: fix endent wrt NULL mappings [BZ #24695] [BZ #24696]

nss_db allows for getpwent et al to be called without a set*ent,
but it only works once.  After the last get*ent a set*ent is
required to restart, because the end*ent did not properly reset
the module.  Resetting it to NULL allows for a proper restart.

If the database doesn't exist, however, end*ent erroniously called
munmap which set errno.

The test case runs "makedb" inside the testroot, so needs selinux
DSOs installed.

Reviewed-by: Carlos O'Donell  <carlos@redhat.com>

On 6/19/19 12:56 PM, DJ Delorie wrote:
> Florian Weimer <fweimer@redhat.com> writes:
>> Just print the return value, then?
> 
> Like this?
> 
> From 4d5ff68bf777ec0ba4594ec7b46403bff34086b2 Mon Sep 17 00:00:00 2001
> From: DJ Delorie <dj@redhat.com>
> Date: Mon, 17 Jun 2019 15:33:27 -0400
> Subject: nss_db: fix endent wrt NULL mappings [BZ #24695] [BZ #24696]
> 
> nss_db allows for getpwent et al to be called without a set*ent,
> but it only works once.  After the last get*ent a set*ent is
> required to restart, because the end*ent did not properly reset
> the module.  Resetting it to NULL allows for a proper restart.
> 
> If the database doesn't exist, however, end*ent erroniously called
> munmap which set errno.
> 
> The test case runs "makedb" inside the testroot, so needs selinux
> DSOs installed.
> 
> Reviewed-by: Carlos O'Donell  <carlos@redhat.com>

FYI, if you change the patch you have to drop the Reviewed-by: lines
until they are given again. I suggest Florian and Andreas give those
lines so we can thank them for their review when generating the release
note.

This patch also looks good to me. I intentionally didn't bike shed too
much about this because I know you'll get the right DT_NEEDED, and if
you don't the test will fail.

OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/ChangeLog b/ChangeLog
> index 2f5dac5190..ae49367cdb 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,19 @@
> +2019-06-17  DJ Delorie  <dj@redhat.com>
> +	    Sergei Trofimovich <slyfox@inbox.ru>
> +
> +	[BZ #24696]
> +	[BZ #24695]
> +	* nss/nss_db/db-open.c (internal_endent): Protect against NULL
> +	mappings.
> +	* nss/tst-nss-db-endgrent.c: New.
> +	* nss/tst-nss-db-endgrent.root: New.
> +	* nss/tst-nss-db-endpwent.c: New.
> +	* nss/tst-nss-db-endpwent.root: New.
> +	* nss/Makefile: Add new tests.
> +	* support/links-dso-program-c.c: Add selinux dependency.
> +	* support/links-dso-program.cc: Add selinux dependency.
> +	* support/Makefile: Build those with -lselinux if enabled.
> +
>  2019-06-17  Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>  
>  	* sysdeps/unix/sysv/linux/m68k/Makefile (sysdep_routines,
> diff --git a/nss/Makefile b/nss/Makefile
> index 95081bddc5..a15c3b7d90 100644
> --- a/nss/Makefile
> +++ b/nss/Makefile
> @@ -61,7 +61,9 @@ xtests			= bug-erange
>  
>  tests-container = \
>  			  tst-nss-test3 \
> -			  tst-nss-files-hosts-long
> +			  tst-nss-files-hosts-long \
> +			  tst-nss-db-endpwent \
> +			  tst-nss-db-endgrent
>  
>  # Tests which need libdl
>  ifeq (yes,$(build-shared))
> diff --git a/nss/nss_db/db-open.c b/nss/nss_db/db-open.c
> index 8a83d6b930..3fa11e9ab0 100644
> --- a/nss/nss_db/db-open.c
> +++ b/nss/nss_db/db-open.c
> @@ -63,5 +63,9 @@ internal_setent (const char *file, struct nss_db_map *mapping)
>  void
>  internal_endent (struct nss_db_map *mapping)
>  {
> -  munmap (mapping->header, mapping->len);
> +  if (mapping->header != NULL)
> +    {
> +      munmap (mapping->header, mapping->len);
> +      mapping->header = NULL;
> +    }
>  }
> diff --git a/nss/tst-nss-db-endgrent.c b/nss/tst-nss-db-endgrent.c
> new file mode 100644
> index 0000000000..367cc6c901
> --- /dev/null
> +++ b/nss/tst-nss-db-endgrent.c
> @@ -0,0 +1,54 @@
> +/* Test for endgrent changing errno for BZ #24696
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdlib.h>
> +#include <sys/types.h>
> +#include <grp.h>
> +#include <unistd.h>
> +#include <errno.h>
> +
> +#include <support/check.h>
> +#include <support/support.h>
> +
> +/* The following test verifies that if the db NSS Service is initialized
> +   with no database (getgrent), that a subsequent closure (endgrent) does
> +   not set errno. In the case of the db service it is not an error to close
> +   the service and so it should not set errno.  */
> +
> +static int
> +do_test (void)
> +{
> +  /* Just make sure it's not there, although usually it won't be.  */
> +  unlink ("/var/db/group.db");
> +
> +  /* This, in conjunction with the testroot's nsswitch.conf, causes
> +     the nss_db module to be "connected" and initialized - but the
> +     testroot has no group.db, so no mapping will be created.  */
> +  getgrent ();
> +
> +  errno = 0;
> +
> +  /* Before the fix, this would call munmap (NULL) and set errno.  */
> +  endgrent ();
> +
> +  if (errno != 0)
> +    FAIL_EXIT1 ("endgrent set errno to %d\n", errno);
> +
> +  return 0;
> +}
> +#include <support/test-driver.c>
> diff --git a/nss/tst-nss-db-endgrent.root/etc/nsswitch.conf b/nss/tst-nss-db-endgrent.root/etc/nsswitch.conf
> new file mode 100644
> index 0000000000..21471df94f
> --- /dev/null
> +++ b/nss/tst-nss-db-endgrent.root/etc/nsswitch.conf
> @@ -0,0 +1 @@
> +group : db files
> diff --git a/nss/tst-nss-db-endpwent.c b/nss/tst-nss-db-endpwent.c
> new file mode 100644
> index 0000000000..0a8b3184b0
> --- /dev/null
> +++ b/nss/tst-nss-db-endpwent.c
> @@ -0,0 +1,70 @@
> +/* Test for endpwent->getpwent crash for BZ #24695
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <pwd.h>
> +
> +#include <support/support.h>
> +#include <support/check.h>
> +
> +/* It is entirely allowed to start with a getpwent call without
> +   resetting the state of the service via a call to setpwent.
> +   You can also call getpwent more times than you have entries in
> +   the service, and it should not fail.  This test iteratates the
> +   database once, gets to the end, and then attempts a second
> +   iteration to look for crashes.  */
> +
> +static void
> +try_it (void)
> +{
> +  struct passwd *pw;
> +
> +  /* setpwent is intentionally omitted here.  The first call to
> +     getpwent detects that it's first and initializes.  The second
> +     time try_it is called, this "first call" was not detected before
> +     the fix, and getpwent would crash.  */
> +
> +  while ((pw = getpwent ()) != NULL)
> +    ;
> +
> +  /* We only care if this segfaults or not.  */
> +  endpwent ();
> +}
> +
> +static int
> +do_test (void)
> +{
> +  char *cmd;
> +  const char *rest;
> +
> +  rest = "/makedb -o /var/db/passwd.db /var/db/passwd.in";
> +  cmd = (char *) xmalloc (strlen (support_bindir_prefix)
> +				+ strlen (rest) + 1);
> +  strcpy (cmd, support_bindir_prefix);
> +  strcat (cmd, rest);
> +
> +  system (cmd);
> +
> +  try_it ();
> +  try_it ();
> +
> +  return 0;
> +}
> +#include <support/test-driver.c>
> diff --git a/nss/tst-nss-db-endpwent.root/etc/nsswitch.conf b/nss/tst-nss-db-endpwent.root/etc/nsswitch.conf
> new file mode 100644
> index 0000000000..593ffc564a
> --- /dev/null
> +++ b/nss/tst-nss-db-endpwent.root/etc/nsswitch.conf
> @@ -0,0 +1 @@
> +passwd: db
> diff --git a/nss/tst-nss-db-endpwent.root/var/db/passwd.in b/nss/tst-nss-db-endpwent.root/var/db/passwd.in
> new file mode 100644
> index 0000000000..98f39126ef
> --- /dev/null
> +++ b/nss/tst-nss-db-endpwent.root/var/db/passwd.in
> @@ -0,0 +1,4 @@
> +.root root:x:0:0:root:/root:/bin/bash
> +=0 root:x:0:0:root:/root:/bin/bash
> +.bin bin:x:1:1:bin:/bin:/sbin/nologin
> +=1 bin:x:1:1:bin:/bin:/sbin/nologin
> diff --git a/support/Makefile b/support/Makefile
> index 56c1ed43bb..ab66913a02 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -191,6 +191,11 @@ LINKS_DSO_PROGRAM = links-dso-program
>  LDLIBS-links-dso-program = -lstdc++ -lgcc -lgcc_s $(libunwind)
>  endif
>  
> +ifeq (yes,$(have-selinux))
> +LDLIBS-$(LINKS_DSO_PROGRAM) += -lselinux
> +endif
> +
> +
>  LDLIBS-test-container = $(libsupport)
>  
>  others += test-container
> diff --git a/support/links-dso-program-c.c b/support/links-dso-program-c.c
> index d28a28a0d0..9cf3e54981 100644
> --- a/support/links-dso-program-c.c
> +++ b/support/links-dso-program-c.c
> @@ -1,9 +1,18 @@
>  #include <stdio.h>
>  
> +/* makedb needs selinux dso's.  */
> +#ifdef HAVE_SELINUX
> +# include <selinux/selinux.h>
> +#endif
> +
>  int
>  main (int argc, char **argv)
>  {
>    /* Complexity to keep gcc from optimizing this away.  */
>    printf ("This is a test %s.\n", argc > 1 ? argv[1] : "null");
> +#ifdef HAVE_SELINUX
> +  /* We only care about the dependency on selinux, not the result.  */
> +  printf ("selinux %d\n", is_selinux_enabled ());
> +#endif
>    return 0;
>  }
> diff --git a/support/links-dso-program.cc b/support/links-dso-program.cc
> index dba6976c06..87907dd81f 100644
> --- a/support/links-dso-program.cc
> +++ b/support/links-dso-program.cc
> @@ -1,5 +1,10 @@
>  #include <iostream>
>  
> +/* makedb needs selinux dso's.  */
> +#ifdef HAVE_SELINUX
> +# include <selinux/selinux.h>
> +#endif
> +
>  using namespace std;
>  
>  int
> @@ -7,5 +12,9 @@ main (int argc, char **argv)
>  {
>    /* Complexity to keep gcc from optimizing this away.  */
>    cout << (argc > 1 ? argv[1] : "null");
> +#ifdef HAVE_SELINUX
> +  /* We only care about the dependency on selinux, not the result.  */
> +  cout << "selinux " << is_selinux_enabled ();
> +#endif
>    return 0;
>  }
>