nss_db: protect against empty mappings

classic Classic list List threaded Threaded
44 messages Options
123
Reply | Threaded
Open this post in threaded view
|

nss_db: protect against empty mappings

DJ Delorie-2

Resolves: #24696

(note to reviewers: Sergi contributed the one-line fix, I did the
rest, for the purposes of copyright-line-counts)

2019-06-17  DJ Delorie  <[hidden email]>
            Sergei Trofimovich <[hidden email]>

        * 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/Makefile: Add new test.

diff --git a/nss/Makefile b/nss/Makefile
index 15fc410cf1..a15c3b7d90 100644
--- a/nss/Makefile
+++ b/nss/Makefile
@@ -62,7 +62,8 @@ xtests = bug-erange
 tests-container = \
   tst-nss-test3 \
   tst-nss-files-hosts-long \
-  tst-nss-db-endpwent
+  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 f7c53b4486..3fa11e9ab0 100644
--- a/nss/nss_db/db-open.c
+++ b/nss/nss_db/db-open.c
@@ -63,6 +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);
-  mapping->header = NULL;
+  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..f4c1e14d60
--- /dev/null
+++ b/nss/tst-nss-db-endgrent.c
@@ -0,0 +1,46 @@
+/* Test for endgrent changing errno.
+   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>
+
+
+static int
+do_test (void)
+{
+  /* Just make sure it's not there, although usually it won't be.  */
+  unlink ("/var/db/group.db");
+
+  getgrent ();
+
+  int saved_errno = errno;
+
+  endgrent ();
+
+  if (errno != saved_errno)
+    FAIL_EXIT1 ("endgrent changed errno from %d o %d\n", saved_errno, 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
Reply | Threaded
Open this post in threaded view
|

Re: nss_db: protect against empty mappings

Carlos O'Donell-6
On 6/17/19 8:28 PM, DJ Delorie wrote:
> Resolves: #24696
>
> (note to reviewers: Sergi contributed the one-line fix, I did the
> rest, for the purposes of copyright-line-counts)
>
> 2019-06-17  DJ Delorie  <[hidden email]>
>    Sergei Trofimovich <[hidden email]>
>

Requires bug # reference.

> * 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/Makefile: Add new test.
>

Please post v2.

> diff --git a/nss/Makefile b/nss/Makefile
> index 15fc410cf1..a15c3b7d90 100644
> --- a/nss/Makefile
> +++ b/nss/Makefile
> @@ -62,7 +62,8 @@ xtests = bug-erange
>  tests-container = \
>    tst-nss-test3 \
>    tst-nss-files-hosts-long \
> -  tst-nss-db-endpwent
> +  tst-nss-db-endpwent \
> +  tst-nss-db-endgrent

OK, new container test.

>  
>  # 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 f7c53b4486..3fa11e9ab0 100644
> --- a/nss/nss_db/db-open.c
> +++ b/nss/nss_db/db-open.c
> @@ -63,6 +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);
> -  mapping->header = NULL;
> +  if (mapping->header != NULL)

OK, if we never loaded a mapping don't attempt to unmap, otherwise we'll clobber
errno via munmap that fails.

> +    {
> +      munmap (mapping->header, mapping->len);
> +      mapping->header = NULL;

No. This merges the fix for 24695 also. We should be careful here to fix just
the thing that's broken and avoid merging both fixes which are coincidentally
in the same place. I would suggest removing the mapping->header = NULL; change
and leave it for your other patch to change.

> +    }
>  }
> diff --git a/nss/tst-nss-db-endgrent.c b/nss/tst-nss-db-endgrent.c
> new file mode 100644
> index 0000000000..f4c1e14d60
> --- /dev/null
> +++ b/nss/tst-nss-db-endgrent.c
> @@ -0,0 +1,46 @@
> +/* Test for endgrent changing errno.

Reference bug # in first line please.

> +   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>
> +
> +

Needs a paragraph explaining what this test is doing.

e.g.

/* The expectation is that a simple call to endgrent
   with only the db and files databases should never set
   errno because there is nothing that can fail. The
   database was never opened and so endgrent is no-op.  */

> +static int
> +do_test (void)
> +{
> +  /* Just make sure it's not there, although usually it won't be.  */
> +  unlink ("/var/db/group.db");
> +
> +  getgrent ();

Do you need this getgrent() or can you call endgrent() right away?

A call to endgrent() should be idempotent, it should do nothing,
and we should be able to call it many times without errno being
set.

It would be a simpler test if we just called endgrent().

> +
> +  int saved_errno = errno;

This isn't correct because errno is not defined, or at least you
haven't verified getgrent might have set it.

Suggest:

errno = 0;

> +
> +  endgrent ();
> +
> +  if (errno != saved_errno)

then:

if (errno != 0)

> +    FAIL_EXIT1 ("endgrent changed errno from %d o %d\n", saved_errno, errno);

FAIL_EXIT1 ("endgrent failed no-op call with errno of %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
>


--
Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: nss_db: protect against empty mappings

DJ Delorie-2

"Carlos O'Donell" <[hidden email]> writes:
>> +    {
>> +      munmap (mapping->header, mapping->len);
>> +      mapping->header = NULL;
>
> No. This merges the fix for 24695 also. We should be careful here to fix just
> the thing that's broken and avoid merging both fixes which are coincidentally
> in the same place. I would suggest removing the mapping->header = NULL; change
> and leave it for your other patch to change.

I wrote the patches in the other order; if I make this one independent
the other one will be dependent.  Plus the Makefile is order-dependent
too.

Unless I do

  if (mapping == NULL)
    return;

But I don't like using that idiom in case other things need to be done
in endgrent, independent of mapping.

Unless you mean to write the patches in isolation and merge them after
review?  I don't think either of these is critical enough to not just
wait until they're both reviewed.

>> +  unlink ("/var/db/group.db");
>> +
>> +  getgrent ();
>
> Do you need this getgrent() or can you call endgrent() right away?

Without it, we don't parse nsswitch, and don't connect to the nss-db
module, and don't get into the "we don't have a database" case.

> A call to endgrent() should be idempotent, it should do nothing,
> and we should be able to call it many times without errno being
> set.

Without getgrent() it really does nothing ;-)
Reply | Threaded
Open this post in threaded view
|

Re: nss_db: protect against empty mappings

Carlos O'Donell-6
On 6/17/19 11:15 PM, DJ Delorie wrote:

>
> "Carlos O'Donell" <[hidden email]> writes:
>>> +    {
>>> +      munmap (mapping->header, mapping->len);
>>> +      mapping->header = NULL;
>>
>> No. This merges the fix for 24695 also. We should be careful here to fix just
>> the thing that's broken and avoid merging both fixes which are coincidentally
>> in the same place. I would suggest removing the mapping->header = NULL; change
>> and leave it for your other patch to change.
>
> I wrote the patches in the other order; if I make this one independent
> the other one will be dependent.  Plus the Makefile is order-dependent
> too.
>
> Unless I do
>
>   if (mapping == NULL)
>     return;
>
> But I don't like using that idiom in case other things need to be done
> in endgrent, independent of mapping.
>
> Unless you mean to write the patches in isolation and merge them after
> review?  I don't think either of these is critical enough to not just
> wait until they're both reviewed.

Merge the fixes then.

Submit one '[PATCH] nss_db: Protect against empty mappys (BZ #xxx, BZ #yyy)'

And list both bugs in the ChangeLog.

>>> +  unlink ("/var/db/group.db");
>>> +
>>> +  getgrent ();
>>
>> Do you need this getgrent() or can you call endgrent() right away?
>
> Without it, we don't parse nsswitch, and don't connect to the nss-db
> module, and don't get into the "we don't have a database" case.

OK, then leave it there.

>> A call to endgrent() should be idempotent, it should do nothing,
>> and we should be able to call it many times without errno being
>> set.
>
> Without getgrent() it really does nothing ;-)

You're right because we don't parse and load db.

--
Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: nss_db: protect against empty mappings

DJ Delorie-2

"Carlos O'Donell" <[hidden email]> writes:
> Merge the fixes then.

Subject: nss_db: fix endent wrt NULL mappings

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.

Resolves: #24695
Resolves: #24696

2019-06-17  DJ Delorie  <[hidden email]>
            Sergei Trofimovich <[hidden email]>

        [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.

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..1d4545f916
--- /dev/null
+++ b/nss/tst-nss-db-endgrent.c
@@ -0,0 +1,50 @@
+/* Test for endgrent changing errno.  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>
+
+
+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..7a2c7ff677
--- /dev/null
+++ b/nss/tst-nss-db-endpwent.c
@@ -0,0 +1,50 @@
+/* Test for endpwent->getpwent crash.  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 <sys/types.h>
+#include <pwd.h>
+
+#include <support/check.h>
+
+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)
+    ;
+
+  endpwent ();
+}
+
+static int
+do_test (void)
+{
+  system ("/usr/bin/makedb -o /var/db/passwd.db /var/db/passwd.in");
+  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..6043b8652b 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>
+int sel;
+#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
+  sel = is_selinux_enabled ();
+#endif
   return 0;
 }
diff --git a/support/links-dso-program.cc b/support/links-dso-program.cc
index dba6976c06..abb04f219a 100644
--- a/support/links-dso-program.cc
+++ b/support/links-dso-program.cc
@@ -1,5 +1,11 @@
 #include <iostream>
 
+/* makedb needs selinux dso's.  */
+#ifdef HAVE_SELINUX
+# include <selinux/selinux.h>
+int sel;
+#endif
+
 using namespace std;
 
 int
@@ -7,5 +13,8 @@ main (int argc, char **argv)
 {
   /* Complexity to keep gcc from optimizing this away.  */
   cout << (argc > 1 ? argv[1] : "null");
+#ifdef HAVE_SELINUX
+  sel = is_selinux_enabled ();
+#endif
   return 0;
 }
Reply | Threaded
Open this post in threaded view
|

Re: nss_db: protect against empty mappings

Florian Weimer-5
* DJ Delorie:

> "Carlos O'Donell" <[hidden email]> writes:
>> Merge the fixes then.
>
> Subject: nss_db: fix endent wrt NULL mappings
>
> 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.
>
> Resolves: #24695
> Resolves: #24696

You need to add “[BZ #24695]” or “bug 24695” to the commit message, the
above will not work.  If you can squeeze both numbers into the first
line, that's best.

> +  /* Before the fix, this would call munmap(NULL) and set errno.  */

Missing space before parenthesis.

> +  /* 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.  */

GNU style is not to write () after function names.

> +  while ((pw = getpwent ()) != NULL)
> +    ;
> +
> +  endpwent ();

Would it be possible to add error checking here?

> +  system ("/usr/bin/makedb -o /var/db/passwd.db /var/db/passwd.in");

I think you need to use the actual installation path, not /usr/bin.

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: nss_db: protect against empty mappings

Carlos O'Donell-6
On 6/18/19 2:12 AM, Florian Weimer wrote:>> +  system ("/usr/bin/makedb -o /var/db/passwd.db /var/db/passwd.in")
> I think you need to use the actual installation path, not /usr/bin.

Correct, you need support_bindir_prefix[].

The /var/db is not configurable, so it can remain static.

--
Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: nss_db: protect against empty mappings

DJ Delorie-2
In reply to this post by Florian Weimer-5

Florian Weimer <[hidden email]> writes:
> You need to add “[BZ #24695]” or “bug 24695” to the commit message, the

Done.

>> +  /* Before the fix, this would call munmap(NULL) and set errno.  */
>
> Missing space before parenthesis.

Fixed.

> GNU style is not to write () after function names.

Fixed.

>> +  while ((pw = getpwent ()) != NULL)
>> +    ;
>> +
>> +  endpwent ();
>
> Would it be possible to add error checking here?

Possible, but pointless.  We're in a testroot with a given database,
and all we care about is segfault-or-not (I added a comment thereof).

>> +  system ("/usr/bin/makedb -o /var/db/passwd.db /var/db/passwd.in");
>
> I think you need to use the actual installation path, not /usr/bin.

Fixed.

Andreas Schwab <[hidden email]> writes:
>> +int sel;
>
> Where is this variable used?

It's not, it's just there to make sure the call to is_selinux_enabled
really happens.



From 0fdf6c5a0f99aca5c943a4a768a3f5deb6befc12 Mon Sep 17 00:00:00 2001
From: DJ Delorie <[hidden email]>
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.

2019-06-17  DJ Delorie  <[hidden email]>
            Sergei Trofimovich <[hidden email]>

        [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.

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..034bcf6bc4
--- /dev/null
+++ b/nss/tst-nss-db-endgrent.c
@@ -0,0 +1,50 @@
+/* Test for endgrent changing errno.  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>
+
+
+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..7e73ce7b5e
--- /dev/null
+++ b/nss/tst-nss-db-endpwent.c
@@ -0,0 +1,61 @@
+/* Test for endpwent->getpwent crash.  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 <sys/types.h>
+#include <pwd.h>
+
+#include <support/check.h>
+
+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;
+  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..e8b64c12ab 100644
--- a/support/links-dso-program-c.c
+++ b/support/links-dso-program-c.c
@@ -1,9 +1,19 @@
 #include <stdio.h>
 
+/* makedb needs selinux dso's.  */
+#ifdef HAVE_SELINUX
+# include <selinux/selinux.h>
+int sel;
+#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.  */
+  sel = is_selinux_enabled ();
+#endif
   return 0;
 }
diff --git a/support/links-dso-program.cc b/support/links-dso-program.cc
index dba6976c06..87bbc2f46b 100644
--- a/support/links-dso-program.cc
+++ b/support/links-dso-program.cc
@@ -1,5 +1,11 @@
 #include <iostream>
 
+/* makedb needs selinux dso's.  */
+#ifdef HAVE_SELINUX
+# include <selinux/selinux.h>
+int sel;
+#endif
+
 using namespace std;
 
 int
@@ -7,5 +13,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.  */
+  sel = is_selinux_enabled ();
+#endif
   return 0;
 }
Reply | Threaded
Open this post in threaded view
|

Re: nss_db: protect against empty mappings

Carlos O'Donell-6
On 6/18/19 1:47 PM, DJ Delorie wrote:

>
> Florian Weimer <[hidden email]> writes:
>> You need to add “[BZ #24695]” or “bug 24695” to the commit message, the
>
> Done.
>
>>> +  /* Before the fix, this would call munmap(NULL) and set errno.  */
>>
>> Missing space before parenthesis.
>
> Fixed.
>
>> GNU style is not to write () after function names.
>
> Fixed.
>
>>> +  while ((pw = getpwent ()) != NULL)
>>> +    ;
>>> +
>>> +  endpwent ();
>>
>> Would it be possible to add error checking here?
>
> Possible, but pointless.  We're in a testroot with a given database,
> and all we care about is segfault-or-not (I added a comment thereof).
>
>>> +  system ("/usr/bin/makedb -o /var/db/passwd.db /var/db/passwd.in");
>>
>> I think you need to use the actual installation path, not /usr/bin.
>
> Fixed.
>
> Andreas Schwab <[hidden email]> writes:
>>> +int sel;
>>
>> Where is this variable used?
>
> It's not, it's just there to make sure the call to is_selinux_enabled
> really happens.
>
>
>
> From 0fdf6c5a0f99aca5c943a4a768a3f5deb6befc12 Mon Sep 17 00:00:00 2001
> From: DJ Delorie <[hidden email]>
> 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.

OK for master if you:

- Use expected subject convention "[PATCH v3] ..."
- Fix test comment.
- Add block comments to the tests.

Reviewed-by: Carlos O'Donell  <[hidden email]>

> 2019-06-17  DJ Delorie  <[hidden email]>
>    Sergei Trofimovich <[hidden email]>
>
> [BZ #24696]
> [BZ #24695]

OK. What a coincidence :}

> * 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.
>
> 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

OK.

>  
>  # 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;
> +    }

OK.

>  }
> diff --git a/nss/tst-nss-db-endgrent.c b/nss/tst-nss-db-endgrent.c
> new file mode 100644
> index 0000000000..034bcf6bc4
> --- /dev/null
> +++ b/nss/tst-nss-db-endgrent.c
> @@ -0,0 +1,50 @@
> +/* Test for endgrent changing errno.  BZ #24696

Suggest:

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>

Needs a block comment explaining what the test is trying to achieve.

/* 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);
> +

OK.

> +  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..7e73ce7b5e
> --- /dev/null
> +++ b/nss/tst-nss-db-endpwent.c
> @@ -0,0 +1,61 @@
> +/* Test for endpwent->getpwent crash.  BZ #24695

Suggest:

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 <sys/types.h>
> +#include <pwd.h>
> +
> +#include <support/check.h>
> +

Suggest:

/* 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;
> +  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>

Ok.

> 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

OK.

> 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

OK.

> 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
> +

OK.

> +
>  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..e8b64c12ab 100644
> --- a/support/links-dso-program-c.c
> +++ b/support/links-dso-program-c.c
> @@ -1,9 +1,19 @@
>  #include <stdio.h>
>  
> +/* makedb needs selinux dso's.  */
> +#ifdef HAVE_SELINUX
> +# include <selinux/selinux.h>
> +int sel;
> +#endif

OK.

> +
>  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.  */
> +  sel = is_selinux_enabled ();
> +#endif

OK.

>    return 0;
>  }
> diff --git a/support/links-dso-program.cc b/support/links-dso-program.cc
> index dba6976c06..87bbc2f46b 100644
> --- a/support/links-dso-program.cc
> +++ b/support/links-dso-program.cc
> @@ -1,5 +1,11 @@
>  #include <iostream>
>  
> +/* makedb needs selinux dso's.  */
> +#ifdef HAVE_SELINUX
> +# include <selinux/selinux.h>
> +int sel;
> +#endif
> +

OK.

>  using namespace std;
>  
>  int
> @@ -7,5 +13,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.  */
> +  sel = is_selinux_enabled ();
> +#endif
>    return 0

OK.

;
>  }
>


--
Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

[PATCH v4] Re: nss_db: protect against empty mappings

DJ Delorie-2
"Carlos O'Donell" <[hidden email]> writes:
> OK for master if you:
>
> - Use expected subject convention "[PATCH v3] ..."
> - Fix test comment.
> - Add block comments to the tests.
>
> Reviewed-by: Carlos O'Donell  <[hidden email]>

Like this?

From dd899abe5b25e00530f6fcf9fc0abda738cfc6a5 Mon Sep 17 00:00:00 2001
From: DJ Delorie <[hidden email]>
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  <[hidden email]>

diff --git a/ChangeLog b/ChangeLog
index 2f5dac5190..ae49367cdb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+2019-06-17  DJ Delorie  <[hidden email]>
+    Sergei Trofimovich <[hidden email]>
+
+ [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  <[hidden email]>
 
  * 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..cab9166fce
--- /dev/null
+++ b/nss/tst-nss-db-endpwent.c
@@ -0,0 +1,68 @@
+/* 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 <sys/types.h>
+#include <pwd.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;
+  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..e8b64c12ab 100644
--- a/support/links-dso-program-c.c
+++ b/support/links-dso-program-c.c
@@ -1,9 +1,19 @@
 #include <stdio.h>
 
+/* makedb needs selinux dso's.  */
+#ifdef HAVE_SELINUX
+# include <selinux/selinux.h>
+int sel;
+#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.  */
+  sel = is_selinux_enabled ();
+#endif
   return 0;
 }
diff --git a/support/links-dso-program.cc b/support/links-dso-program.cc
index dba6976c06..87bbc2f46b 100644
--- a/support/links-dso-program.cc
+++ b/support/links-dso-program.cc
@@ -1,5 +1,11 @@
 #include <iostream>
 
+/* makedb needs selinux dso's.  */
+#ifdef HAVE_SELINUX
+# include <selinux/selinux.h>
+int sel;
+#endif
+
 using namespace std;
 
 int
@@ -7,5 +13,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.  */
+  sel = is_selinux_enabled ();
+#endif
   return 0;
 }
Reply | Threaded
Open this post in threaded view
|

[PATCH v5] Re: nss_db: protect against empty mappings

DJ Delorie-2
In reply to this post by Carlos O'Donell-6
"Carlos O'Donell" <[hidden email]> writes:
> OK for master if you:
>
> - Use expected subject convention "[PATCH v3] ..."
> - Fix test comment.
> - Add block comments to the tests.
>
> Reviewed-by: Carlos O'Donell  <[hidden email]>

And a v5 to fix some build warnings...

From 864952d782df68771590ff85a697504bb3174e8d Mon Sep 17 00:00:00 2001
From: DJ Delorie <[hidden email]>
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  <[hidden email]>

diff --git a/ChangeLog b/ChangeLog
index 2f5dac5190..ae49367cdb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+2019-06-17  DJ Delorie  <[hidden email]>
+    Sergei Trofimovich <[hidden email]>
+
+ [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  <[hidden email]>
 
  * 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..e8b64c12ab 100644
--- a/support/links-dso-program-c.c
+++ b/support/links-dso-program-c.c
@@ -1,9 +1,19 @@
 #include <stdio.h>
 
+/* makedb needs selinux dso's.  */
+#ifdef HAVE_SELINUX
+# include <selinux/selinux.h>
+int sel;
+#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.  */
+  sel = is_selinux_enabled ();
+#endif
   return 0;
 }
diff --git a/support/links-dso-program.cc b/support/links-dso-program.cc
index dba6976c06..87bbc2f46b 100644
--- a/support/links-dso-program.cc
+++ b/support/links-dso-program.cc
@@ -1,5 +1,11 @@
 #include <iostream>
 
+/* makedb needs selinux dso's.  */
+#ifdef HAVE_SELINUX
+# include <selinux/selinux.h>
+int sel;
+#endif
+
 using namespace std;
 
 int
@@ -7,5 +13,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.  */
+  sel = is_selinux_enabled ();
+#endif
   return 0;
 }
Reply | Threaded
Open this post in threaded view
|

Re: nss_db: protect against empty mappings

Andreas Schwab
In reply to this post by DJ Delorie-2
On Jun 18 2019, DJ Delorie <[hidden email]> wrote:

>> Where is this variable used?
>
> It's not, it's just there to make sure the call to is_selinux_enabled
> really happens.

If calling is_selinux_enabled has side effects then it cannot be elided.
If it doesn't, then it is pointless.  Assignment to a write-only
variable doesn't change that.

Andreas.

--
Andreas Schwab, SUSE Labs, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: nss_db: protect against empty mappings

DJ Delorie-2

Andreas Schwab <[hidden email]> writes:
> If calling is_selinux_enabled has side effects then it cannot be elided.
> If it doesn't, then it is pointless.  Assignment to a write-only
> variable doesn't change that.

Sorry, by "side effects" I meant "the program has a runtime dependency
on selinux.so".  The program is never run so it doesn't matter what the
function *does*.  The code is intentionally overcomplicated to ensure
gcc doesn't optimize away the call, either now or in the future.

The only thing we ever do with that program is call ldd on it to
enumerate the DSOs that need to be installed into the testroot.
Reply | Threaded
Open this post in threaded view
|

Re: nss_db: protect against empty mappings

Florian Weimer-5
* DJ Delorie:

> Andreas Schwab <[hidden email]> writes:
>> If calling is_selinux_enabled has side effects then it cannot be elided.
>> If it doesn't, then it is pointless.  Assignment to a write-only
>> variable doesn't change that.
>
> Sorry, by "side effects" I meant "the program has a runtime dependency
> on selinux.so".  The program is never run so it doesn't matter what the
> function *does*.  The code is intentionally overcomplicated to ensure
> gcc doesn't optimize away the call, either now or in the future.

Just print the return value, then?

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

[PATCH V6] nss_db: protect against empty mappings

DJ Delorie-2
Florian Weimer <[hidden email]> writes:
> Just print the return value, then?

Like this?

From 4d5ff68bf777ec0ba4594ec7b46403bff34086b2 Mon Sep 17 00:00:00 2001
From: DJ Delorie <[hidden email]>
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  <[hidden email]>

diff --git a/ChangeLog b/ChangeLog
index 2f5dac5190..ae49367cdb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+2019-06-17  DJ Delorie  <[hidden email]>
+    Sergei Trofimovich <[hidden email]>
+
+ [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  <[hidden email]>
 
  * 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;
 }
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH V6] nss_db: protect against empty mappings

Carlos O'Donell-6
On 6/19/19 12:56 PM, DJ Delorie wrote:

> Florian Weimer <[hidden email]> writes:
>> Just print the return value, then?
>
> Like this?
>
> From 4d5ff68bf777ec0ba4594ec7b46403bff34086b2 Mon Sep 17 00:00:00 2001
> From: DJ Delorie <[hidden email]>
> 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  <[hidden email]>

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 <[hidden email]>

> diff --git a/ChangeLog b/ChangeLog
> index 2f5dac5190..ae49367cdb 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,19 @@
> +2019-06-17  DJ Delorie  <[hidden email]>
> +    Sergei Trofimovich <[hidden email]>
> +
> + [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  <[hidden email]>
>  
>   * 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;
>  }
>


--
Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: nss_db: protect against empty mappings

Andreas Schwab
In reply to this post by DJ Delorie-2
On Jun 19 2019, DJ Delorie <[hidden email]> wrote:

> Andreas Schwab <[hidden email]> writes:
>> If calling is_selinux_enabled has side effects then it cannot be elided.
>> If it doesn't, then it is pointless.  Assignment to a write-only
>> variable doesn't change that.
>
> Sorry, by "side effects" I meant "the program has a runtime dependency
> on selinux.so".  The program is never run so it doesn't matter what the
> function *does*.  The code is intentionally overcomplicated to ensure
> gcc doesn't optimize away the call, either now or in the future.

Then please add a comment that this call is only there to add a
dependency on libselinux.

Andreas.

--
Andreas Schwab, SUSE Labs, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: nss_db: protect against empty mappings

DJ Delorie-2

Andreas Schwab <[hidden email]> writes:
> Then please add a comment that this call is only there to add a
> dependency on libselinux.

I did, in the v6 patch:
https://www.sourceware.org/ml/libc-alpha/2019-06/msg00457.html
Reply | Threaded
Open this post in threaded view
|

Re: nss_db: protect against empty mappings

Andreas Schwab
On Jun 24 2019, DJ Delorie <[hidden email]> wrote:

> Andreas Schwab <[hidden email]> writes:
>> Then please add a comment that this call is only there to add a
>> dependency on libselinux.
>
> I did, in the v6 patch:
> https://www.sourceware.org/ml/libc-alpha/2019-06/msg00457.html

That comment does not quite say that.  It should say that it's a symbol
reference to the library (not a functional dependency).

Andreas.

--
Andreas Schwab, SUSE Labs, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

[PATCHv7] nss_db: protect against empty mappings

DJ Delorie-2
Andreas Schwab <[hidden email]> writes:
> That comment does not quite say that.  It should say that it's a symbol
> reference to the library (not a functional dependency).

I added a big comment to both of those explaining what the files were
for.

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.

2019-06-25  DJ Delorie  <[hidden email]>
            Sergei Trofimovich <[hidden email]>

        [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.

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..5fcbab2c17 100644
--- a/support/links-dso-program-c.c
+++ b/support/links-dso-program-c.c
@@ -1,9 +1,26 @@
 #include <stdio.h>
 
+/* makedb needs selinux dso's.  */
+#ifdef HAVE_SELINUX
+# include <selinux/selinux.h>
+#endif
+
+/* The purpose of this file is to indicate to the build system which
+   shared objects need to be copied into the testroot, such as gcc or
+   selinux support libraries.  This program is never executed, only
+   scanned for dependencies on shared objects, so the code below may
+   seem weird - it's written to survive gcc optimization and force
+   such dependencies.
+*/
+
 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
+  /* This exists to force libselinux.so to be required.  */
+  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..4bc2411086 100644
--- a/support/links-dso-program.cc
+++ b/support/links-dso-program.cc
@@ -1,11 +1,28 @@
 #include <iostream>
 
+/* makedb needs selinux dso's.  */
+#ifdef HAVE_SELINUX
+# include <selinux/selinux.h>
+#endif
+
 using namespace std;
 
+/* The purpose of this file is to indicate to the build system which
+   shared objects need to be copied into the testroot, such as gcc or
+   selinux support libraries.  This program is never executed, only
+   scanned for dependencies on shared objects, so the code below may
+   seem weird - it's written to survive gcc optimization and force
+   such dependencies.
+*/
+
 int
 main (int argc, char **argv)
 {
   /* Complexity to keep gcc from optimizing this away.  */
   cout << (argc > 1 ? argv[1] : "null");
+#ifdef HAVE_SELINUX
+  /* This exists to force libselinux.so to be required.  */
+  cout << "selinux " << is_selinux_enabled ();
+#endif
   return 0;
 }
123