nss_db: protect against empty mappings
Commit Message
Florian Weimer <fweimer@redhat.com> 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 <schwab@suse.de> 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 <dj@redhat.com>
Date: Mon, 17 Jun 2019 15:33:27 -0400
Subject: nss_db: fix endent wrt NULL mappings [BZ #24695] [BZ #24696]
nss_db allows for getpwent et al to be called without a set*ent,
but it only works once. After the last get*ent a set*ent is
required to restart, because the end*ent did not properly reset
the module. Resetting it to NULL allows for a proper restart.
If the database doesn't exist, however, end*ent erroniously called
munmap which set errno.
The test case runs "makedb" inside the testroot, so needs selinux
DSOs installed.
2019-06-17 DJ Delorie <dj@redhat.com>
Sergei Trofimovich <slyfox@inbox.ru>
[BZ #24696]
[BZ #24695]
* nss/nss_db/db-open.c (internal_endent): Protect against NULL
mappings.
* nss/tst-nss-db-endgrent.c: New.
* nss/tst-nss-db-endgrent.root: New.
* nss/tst-nss-db-endpwent.c: New.
* nss/tst-nss-db-endpwent.root: New.
* nss/Makefile: Add new tests.
* support/links-dso-program-c.c: Add selinux dependency.
* support/links-dso-program.cc: Add selinux dependency.
* support/Makefile: Build those with -lselinux if enabled.
Comments
On 6/18/19 1:47 PM, DJ Delorie wrote:
>
> Florian Weimer <fweimer@redhat.com> 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 <schwab@suse.de> 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 <dj@redhat.com>
> Date: Mon, 17 Jun 2019 15:33:27 -0400
> Subject: nss_db: fix endent wrt NULL mappings [BZ #24695] [BZ #24696]
>
> nss_db allows for getpwent et al to be called without a set*ent,
> but it only works once. After the last get*ent a set*ent is
> required to restart, because the end*ent did not properly reset
> the module. Resetting it to NULL allows for a proper restart.
>
> If the database doesn't exist, however, end*ent erroniously called
> munmap which set errno.
>
> The test case runs "makedb" inside the testroot, so needs selinux
> DSOs installed.
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 <carlos@redhat.com>
> 2019-06-17 DJ Delorie <dj@redhat.com>
> Sergei Trofimovich <slyfox@inbox.ru>
>
> [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.
;
> }
>
On Jun 18 2019, DJ Delorie <dj@redhat.com> 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.
@@ -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))
@@ -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;
+ }
}
new file mode 100644
@@ -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>
new file mode 100644
@@ -0,0 +1 @@
+group : db files
new file mode 100644
@@ -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>
new file mode 100644
@@ -0,0 +1 @@
+passwd: db
new file mode 100644
@@ -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
@@ -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
@@ -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;
}
@@ -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;
}