nsswitch: do not reload if "/" changes

nsswitch: do not reload if "/" changes

[DJ Delorie via Libc-alpha]

[Note: I tried putting this functionality in the file_change_detection
module, but that didn't have enough persistence.]

[Note: tested by instrumenting test-container.c and observing the
instrumentation with test containers on the root fs and on a separate
fs]

https://sourceware.org/bugzilla/show_bug.cgi?id=27077

Before reloading nsswitch.conf, verify that the root directory
hasn't changed - if it has, it's likely that we've entered a
container and should not trust the nsswitch inside the container
nor load any shared objects therein.

diff --git a/nss/nss_database.c b/nss/nss_database.c
index e719ec0865..580ea7b963 100644
--- a/nss/nss_database.c
+++ b/nss/nss_database.c
@@ -33,6 +33,11 @@ struct nss_database_state
 {
   struct nss_database_data data;
   __libc_lock_define (, lock);
+  /* If "/" changes, we switched into a container and do NOT want to
+     reload anything.  This data must be persistent across
+     reloads.  */
+  ino64_t root_ino;
+  ino64_t root_dev;
 };
 
 
@@ -53,6 +58,8 @@ global_state_allocate (void *closure)
       memset (result->data.services, 0, sizeof (result->data.services));
       result->data.initialized = true;
       result->data.reload_disabled = false;
+      result->root_ino = 0;
+      result->root_dev = 0;
       __libc_lock_init (result->lock);
     }
   return result;
@@ -356,6 +363,8 @@ nss_database_check_reload_and_get (struct nss_database_state *local,
                                    nss_action_list *result,
                                    enum nss_database database_index)
 {
+  struct stat64 str;
+
   /* Acquire MO is needed because the thread that sets reload_disabled
      may have loaded the configuration first, so synchronize with the
      Release MO store there.  */
@@ -379,6 +388,25 @@ nss_database_check_reload_and_get (struct nss_database_state *local,
       __libc_lock_unlock (local->lock);
       return true;
     }
+
+  /* Before we reload, verify that "/" hasn't changed.  We assume that
+     errors here are very unlikely, but the chance that we're entering
+     a container is also very unlikely, so we err on the side of both
+     very unlikely things not happening at the same time.  */
+  if (__stat64 ("/", &str) == 0)
+    {
+      if (local->root_ino != 0
+	  && (str.st_ino != local->root_ino
+	      ||  str.st_dev != local->root_dev))
+	{
+	  /* Change detected; disable reloading.  */
+	  atomic_store_release (&local->data.reload_disabled, 1);
+	  __libc_lock_unlock (local->lock);
+	  return true;
+	}
+      local->root_ino = str.st_ino;
+      local->root_dev = str.st_dev;
+    }
   __libc_lock_unlock (local->lock);
 
   /* Avoid overwriting the global configuration until we have loaded
diff --git a/nss/nss_database.h b/nss/nss_database.h
index 1f827e6def..f94c629174 100644
--- a/nss/nss_database.h
+++ b/nss/nss_database.h
@@ -75,6 +75,10 @@ struct nss_database_data
   nss_action_list services[NSS_DATABASE_COUNT];
   int reload_disabled;          /* Actually bool; int for atomic access.  */
   bool initialized;
+  /* If "/" changes, we switched into a container and do NOT want to
+     reload anything.  */
+  ino64_t root_ino;
+  ino64_t root_dev;
 };
 
 /* Called by fork in the parent process, before forking.  */

Re: nsswitch: do not reload if "/" changes

[Florian Weimer via Libc-alpha]

* DJ Delorie via Libc-alpha:

> diff --git a/nss/nss_database.c b/nss/nss_database.c
> index e719ec0865..580ea7b963 100644
> --- a/nss/nss_database.c
> +++ b/nss/nss_database.c
> @@ -33,6 +33,11 @@ struct nss_database_state
>  {
>    struct nss_database_data data;
>    __libc_lock_define (, lock);
> +  /* If "/" changes, we switched into a container and do NOT want to
> +     reload anything.  This data must be persistent across
> +     reloads.  */
> +  ino64_t root_ino;
> +  ino64_t root_dev;
>  };

dev_t for root_dev?

> @@ -53,6 +58,8 @@ global_state_allocate (void *closure)
>        memset (result->data.services, 0, sizeof (result->data.services));
>        result->data.initialized = true;
>        result->data.reload_disabled = false;
> +      result->root_ino = 0;
> +      result->root_dev = 0;
>        __libc_lock_init (result->lock);
>      }

Perhaps you can match the declaration order in the initialization?

> diff --git a/nss/nss_database.h b/nss/nss_database.h
> index 1f827e6def..f94c629174 100644
> --- a/nss/nss_database.h
> +++ b/nss/nss_database.h
> @@ -75,6 +75,10 @@ struct nss_database_data
>    nss_action_list services[NSS_DATABASE_COUNT];
>    int reload_disabled;          /* Actually bool; int for atomic access.  */
>    bool initialized;
> +  /* If "/" changes, we switched into a container and do NOT want to
> +     reload anything.  */
> +  ino64_t root_ino;
> +  ino64_t root_dev;
>  };
>  
>  /* Called by fork in the parent process, before forking.  */

This does not seem to be needed?

Rest looks good.

I have one remaining question: Should we load service modules after /
has changed?  Disabling reloading brings us back to the old behavior in
terms of exposure to untrusted /, but maybe we can do even better and
stop loading service modules altogether?  Assuming that this change is
compatible with init systems.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: nsswitch: do not reload if "/" changes

[DJ Delorie via Libc-alpha]

Florian Weimer <fweimer@redhat.com> writes:
> dev_t for root_dev?

Oops.

> Perhaps you can match the declaration order in the initialization?

Fixed.

> This does not seem to be needed?

Heh, I had removed the code, but forgot to save that buffer.  Fixed.

> I have one remaining question: Should we load service modules after /
> has changed?  Disabling reloading brings us back to the old behavior in
> terms of exposure to untrusted /, but maybe we can do even better and
> stop loading service modules altogether?  Assuming that this change is
> compatible with init systems.

This patch makes it "no worse than before" but I'm not sure we can make
it better than before, because we have no hints that we're entering a
container, and by the time we have, it's too late to load the right
module.  The options become (1) don't load the module and definitely
fail, or (2) maybe load the module in the container and work (and,
depending on your app, open a security hole?) (which is the "old way").

We would either need a new API that says "about to enter container" (or
hack into the namespace syscalls) or at least dlopen all mentioned
modules when we parse nsswitch.conf


From f4e9b7d9f9f6f513a9e1264e69b1e666d441de80 Mon Sep 17 00:00:00 2001
From: DJ Delorie <dj@redhat.com>
Date: Fri, 15 Jan 2021 19:50:00 -0500
Subject: nsswitch: do not reload if "/" changes

https://sourceware.org/bugzilla/show_bug.cgi?id=27077

Before reloading nsswitch.conf, verify that the root directory
hasn't changed - if it has, it's likely that we've entered a
container and should not trust the nsswitch inside the container
nor load any shared objects therein.

diff --git a/nss/nss_database.c b/nss/nss_database.c
index e719ec0865..4b4730b114 100644
--- a/nss/nss_database.c
+++ b/nss/nss_database.c
@@ -33,6 +33,11 @@ struct nss_database_state
 {
   struct nss_database_data data;
   __libc_lock_define (, lock);
+  /* If "/" changes, we switched into a container and do NOT want to
+     reload anything.  This data must be persistent across
+     reloads.  */
+  ino64_t root_ino;
+  dev_t root_dev;
 };
 
 
@@ -54,6 +59,8 @@ global_state_allocate (void *closure)
       result->data.initialized = true;
       result->data.reload_disabled = false;
       __libc_lock_init (result->lock);
+      result->root_ino = 0;
+      result->root_dev = 0;
     }
   return result;
 }
@@ -356,6 +363,8 @@ nss_database_check_reload_and_get (struct nss_database_state *local,
                                    nss_action_list *result,
                                    enum nss_database database_index)
 {
+  struct stat64 str;
+
   /* Acquire MO is needed because the thread that sets reload_disabled
      may have loaded the configuration first, so synchronize with the
      Release MO store there.  */
@@ -379,6 +388,25 @@ nss_database_check_reload_and_get (struct nss_database_state *local,
       __libc_lock_unlock (local->lock);
       return true;
     }
+
+  /* Before we reload, verify that "/" hasn't changed.  We assume that
+     errors here are very unlikely, but the chance that we're entering
+     a container is also very unlikely, so we err on the side of both
+     very unlikely things not happening at the same time.  */
+  if (__stat64 ("/", &str) == 0)
+    {
+      if (local->root_ino != 0
+	  && (str.st_ino != local->root_ino
+	      ||  str.st_dev != local->root_dev))
+	{
+	  /* Change detected; disable reloading.  */
+	  atomic_store_release (&local->data.reload_disabled, 1);
+	  __libc_lock_unlock (local->lock);
+	  return true;
+	}
+      local->root_ino = str.st_ino;
+      local->root_dev = str.st_dev;
+    }
   __libc_lock_unlock (local->lock);
 
   /* Avoid overwriting the global configuration until we have loaded

Re: nsswitch: do not reload if "/" changes

[Florian Weimer via Libc-alpha]

* DJ Delorie:

>> I have one remaining question: Should we load service modules after /
>> has changed?  Disabling reloading brings us back to the old behavior in
>> terms of exposure to untrusted /, but maybe we can do even better and
>> stop loading service modules altogether?  Assuming that this change is
>> compatible with init systems.
>
> This patch makes it "no worse than before" but I'm not sure we can make
> it better than before, because we have no hints that we're entering a
> container, and by the time we have, it's too late to load the right
> module.  The options become (1) don't load the module and definitely
> fail, or (2) maybe load the module in the container and work (and,
> depending on your app, open a security hole?) (which is the "old way").
>
> We would either need a new API that says "about to enter container" (or
> hack into the namespace syscalls) or at least dlopen all mentioned
> modules when we parse nsswitch.conf

Are you concerned with the case that there are no NSS calls before
entering the container, so that we don't initialize anything at all?

> +  /* Before we reload, verify that "/" hasn't changed.  We assume that
> +     errors here are very unlikely, but the chance that we're entering
> +     a container is also very unlikely, so we err on the side of both
> +     very unlikely things not happening at the same time.  */
> +  if (__stat64 ("/", &str) == 0)

Hmm.  Upon second thought, I think this need to be made fail-closed by
disabling reload on stat failure.  The two things aren't as unrelated as
one might think (chroot + truning on some security filter doesn't seem
to be uncommon).  Now of course it's a bit unlikely that anything can be
loaded later if / can't be read, but is there a harm in macking this
explicity?

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: nsswitch: do not reload if "/" changes

[DJ Delorie via Libc-alpha]

Florian Weimer <fweimer@redhat.com> writes:
> Are you concerned with the case that there are no NSS calls before
> entering the container, so that we don't initialize anything at all?

The key use case for this is tools that manage/debug/investigate the
contents of containers.  I would expect with those, they've done many
nss calls in order to authenticate, find the container, etc.

The *common* case for container entry is "enter container, exec some
other program".  That exec resets the nss lookup since it's a new
process.

> Hmm.  Upon second thought, I think this need to be made fail-closed by
> disabling reload on stat failure.  The two things aren't as unrelated as
> one might think (chroot + truning on some security filter doesn't seem
> to be uncommon).  Now of course it's a bit unlikely that anything can be
> loaded later if / can't be read, but is there a harm in macking this
> explicity?

Can we reuse the reloadable flag?  Or do we need a second independent
flag just for dll reloading?  IIRC we also use the reloadable flag for
test cases that override nsswitch.conf, so I would guess no.

Re: nsswitch: do not reload if "/" changes

[Florian Weimer via Libc-alpha]

* DJ Delorie:

>> Hmm.  Upon second thought, I think this need to be made fail-closed by
>> disabling reload on stat failure.  The two things aren't as unrelated as
>> one might think (chroot + truning on some security filter doesn't seem
>> to be uncommon).  Now of course it's a bit unlikely that anything can be
>> loaded later if / can't be read, but is there a harm in macking this
>> explicity?
>
> Can we reuse the reloadable flag?

The above is about a second issue, disabling reloading in case of stat
failure.  It's independent whether we disable DSO loading or not.

> Or do we need a second independent flag just for dll reloading?  IIRC
> we also use the reloadable flag for test cases that override
> nsswitch.conf, so I would guess no.

I think we could reuse the flag.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

[v2] nsswitch: do not reload if "/" changes

[DJ Delorie via Libc-alpha]

Florian Weimer <fweimer@redhat.com> writes:
> I think we could reuse the flag.

It turned out design-wise difficult to use that flag, but I found a
simple way to handle it through the module interface.  Now includes test
case.

From 28d52ffe491e46d8092c9239141c03c7b9d83c0d Mon Sep 17 00:00:00 2001
From: DJ Delorie <dj@redhat.com>
Date: Fri, 15 Jan 2021 19:50:00 -0500
Subject: nsswitch: do not reload if "/" changes

https://sourceware.org/bugzilla/show_bug.cgi?id=27077

Before reloading nsswitch.conf, verify that the root directory
hasn't changed - if it has, it's likely that we've entered a
container and should not trust the nsswitch inside the container
nor load any shared objects therein.

diff --git a/nss/Makefile b/nss/Makefile
index 5f6bf598a1..0906202db9 100644
--- a/nss/Makefile
+++ b/nss/Makefile
@@ -67,7 +67,7 @@ tests-container = \
 			  tst-nss-files-hosts-long \
 			  tst-nss-db-endpwent \
 			  tst-nss-db-endgrent \
-			  tst-reload1
+			  tst-reload1 tst-reload2
 
 # Tests which need libdl
 ifeq (yes,$(build-shared))
diff --git a/nss/nss_database.c b/nss/nss_database.c
index e719ec0865..ec0809d7a6 100644
--- a/nss/nss_database.c
+++ b/nss/nss_database.c
@@ -33,6 +33,11 @@ struct nss_database_state
 {
   struct nss_database_data data;
   __libc_lock_define (, lock);
+  /* If "/" changes, we switched into a container and do NOT want to
+     reload anything.  This data must be persistent across
+     reloads.  */
+  ino64_t root_ino;
+  dev_t root_dev;
 };
 
 
@@ -54,6 +59,8 @@ global_state_allocate (void *closure)
       result->data.initialized = true;
       result->data.reload_disabled = false;
       __libc_lock_init (result->lock);
+      result->root_ino = 0;
+      result->root_dev = 0;
     }
   return result;
 }
@@ -356,6 +363,8 @@ nss_database_check_reload_and_get (struct nss_database_state *local,
                                    nss_action_list *result,
                                    enum nss_database database_index)
 {
+  struct stat64 str;
+
   /* Acquire MO is needed because the thread that sets reload_disabled
      may have loaded the configuration first, so synchronize with the
      Release MO store there.  */
@@ -379,6 +388,26 @@ nss_database_check_reload_and_get (struct nss_database_state *local,
       __libc_lock_unlock (local->lock);
       return true;
     }
+
+  /* Before we reload, verify that "/" hasn't changed.  We assume that
+     errors here are very unlikely, but the chance that we're entering
+     a container is also very unlikely, so we err on the side of both
+     very unlikely things not happening at the same time.  */
+  if (__stat64 ("/", &str) == 0)
+    {
+      if (local->root_ino != 0
+	  && (str.st_ino != local->root_ino
+	      ||  str.st_dev != local->root_dev))
+	{
+	  /* Change detected; disable reloading.  */
+	  atomic_store_release (&local->data.reload_disabled, 1);
+	  __libc_lock_unlock (local->lock);
+	  __nss_module_disable_loading ();
+	  return true;
+	}
+      local->root_ino = str.st_ino;
+      local->root_dev = str.st_dev;
+    }
   __libc_lock_unlock (local->lock);
 
   /* Avoid overwriting the global configuration until we have loaded
diff --git a/nss/nss_module.c b/nss/nss_module.c
index 1a9359930d..6c5f341f3e 100644
--- a/nss/nss_module.c
+++ b/nss/nss_module.c
@@ -349,6 +349,19 @@ __nss_disable_nscd (void (*cb) (size_t, struct traced_file *))
 }
 #endif
 
+/* Block attempts to dlopen any module we haven't already opened.  */
+void
+__nss_module_disable_loading (void)
+{
+  __libc_lock_lock (nss_module_list_lock);
+
+  for (struct nss_module *p = nss_module_list; p != NULL; p = p->next)
+    if (p->state == nss_module_uninitialized)
+      p->state = nss_module_failed;
+
+  __libc_lock_unlock (nss_module_list_lock);
+}
+
 void __libc_freeres_fn_section
 __nss_module_freeres (void)
 {
diff --git a/nss/nss_module.h b/nss/nss_module.h
index 06e8c29040..05c4791d11 100644
--- a/nss/nss_module.h
+++ b/nss/nss_module.h
@@ -87,6 +87,9 @@ bool __nss_module_load (struct nss_module *module) attribute_hidden;
 void *__nss_module_get_function (struct nss_module *module, const char *name)
   attribute_hidden;
 
+/* Block attempts to dlopen any module we haven't already opened.  */
+void __nss_module_disable_loading (void);
+
 /* Called from __libc_freeres.  */
 void __nss_module_freeres (void) attribute_hidden;
 
diff --git a/nss/tst-reload2.c b/nss/tst-reload2.c
new file mode 100644
index 0000000000..afa2cad415
--- /dev/null
+++ b/nss/tst-reload2.c
@@ -0,0 +1,126 @@
+/* Test that reloading is disabled after a chroot.
+   Copyright (C) 2020-2021 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <nss.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <limits.h>
+#include <sys/types.h>
+#include <errno.h>
+#include <pwd.h>
+#include <grp.h>
+#include <unistd.h>
+
+#include <support/support.h>
+#include <support/check.h>
+#include <support/xunistd.h>
+
+#include "nss_test.h"
+
+static struct passwd pwd_table1[] =
+  {
+   PWD_N (1234, "test1"),
+   PWD_N (4321, "test2"),
+   PWD_LAST ()
+  };
+
+static const char *group_4[] = {
+  "alpha", "beta", "gamma", "fred", NULL
+};
+
+static struct group group_table_data[] =
+  {
+   GRP(4), /* match */
+   GRP_LAST ()
+  };
+
+void
+_nss_test1_init_hook (test_tables *t)
+{
+  t->pwd_table = pwd_table1;
+  t->grp_table = group_table_data;
+}
+
+static struct passwd pwd_table2[] =
+  {
+   PWD_N (5, "test1"),
+   PWD_N (2468, "test2"),
+   PWD_LAST ()
+  };
+
+void
+_nss_test2_init_hook (test_tables *t)
+{
+  t->pwd_table = pwd_table2;
+}
+
+static int
+do_test (void)
+{
+  struct passwd *pw;
+  struct group *gr;
+  char buf1[PATH_MAX];
+  char buf2[PATH_MAX];
+
+  sprintf (buf1, "/subdir%s", support_slibdir_prefix);
+  xmkdirp (buf1, 0777);
+
+  /* Copy this DSO into the chroot so it *could* be loaded.  */
+  sprintf (buf1, "%s/libnss_files.so.2", support_slibdir_prefix);
+  sprintf (buf2, "/subdir%s/libnss_files.so.2", support_slibdir_prefix);
+  support_copy_file (buf1, buf2);
+  
+  /* Check we're using the "outer" nsswitch.conf.  */
+
+  /* This uses the test1 DSO.  */
+  pw = getpwnam("test1");
+  TEST_VERIFY (pw != NULL);
+  if (pw)
+    TEST_COMPARE (pw->pw_uid, 1234);
+
+  /* This just loads the test2 DSO.  */
+  gr = getgrnam("name4");
+
+  /* Change the root dir.  */
+
+  TEST_VERIFY (chroot ("/subdir") == 0);
+  chdir("/");
+
+  /* Check we're NOT using the "inner" nsswitch.conf.  */
+
+  /* Both DSOs are loaded, which is used?  */
+  pw = getpwnam("test2");
+  TEST_VERIFY (pw != NULL);
+  if (pw)
+    TEST_VERIFY (pw->pw_uid != 2468);
+
+  /* The "files" DSO should not be loaded.  */
+  gr = getgrnam("test3");
+  TEST_VERIFY (gr == NULL);
+
+  /* We should still be using the old configuration.  */
+  pw = getpwnam("test1");
+  TEST_VERIFY (pw != NULL);
+  if (pw)
+    TEST_COMPARE (pw->pw_uid, 1234);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/nss/tst-reload2.root/etc/nsswitch.conf b/nss/tst-reload2.root/etc/nsswitch.conf
new file mode 100644
index 0000000000..570795ae22
--- /dev/null
+++ b/nss/tst-reload2.root/etc/nsswitch.conf
@@ -0,0 +1,2 @@
+passwd: test1
+group: test2
diff --git a/nss/tst-reload2.root/subdir/etc/group b/nss/tst-reload2.root/subdir/etc/group
new file mode 100644
index 0000000000..e48646bd47
--- /dev/null
+++ b/nss/tst-reload2.root/subdir/etc/group
@@ -0,0 +1 @@
+test3:x:123:
diff --git a/nss/tst-reload2.root/subdir/etc/nsswitch.conf b/nss/tst-reload2.root/subdir/etc/nsswitch.conf
new file mode 100644
index 0000000000..f1d73f8765
--- /dev/null
+++ b/nss/tst-reload2.root/subdir/etc/nsswitch.conf
@@ -0,0 +1,2 @@
+passwd: test2
+group: files
diff --git a/nss/tst-reload2.root/tst-reload2.script b/nss/tst-reload2.root/tst-reload2.script
new file mode 100644
index 0000000000..c6ee4b8e5e
--- /dev/null
+++ b/nss/tst-reload2.root/tst-reload2.script
@@ -0,0 +1,3 @@
+su
+cp $B/nss/libnss_test1.so $L/libnss_test1.so.2
+cp $B/nss/libnss_test2.so $L/libnss_test2.so.2

Re: [v2] nsswitch: do not reload if "/" changes

[Florian Weimer via Libc-alpha]

* DJ Delorie via Libc-alpha:

> +  /* Before we reload, verify that "/" hasn't changed.  We assume that
> +     errors here are very unlikely, but the chance that we're entering
> +     a container is also very unlikely, so we err on the side of both
> +     very unlikely things not happening at the same time.  */
> +  if (__stat64 ("/", &str) == 0)
> +    {
> +      if (local->root_ino != 0
> +	  && (str.st_ino != local->root_ino
> +	      ||  str.st_dev != local->root_dev))
> +	{
> +	  /* Change detected; disable reloading.  */
> +	  atomic_store_release (&local->data.reload_disabled, 1);
> +	  __libc_lock_unlock (local->lock);
> +	  __nss_module_disable_loading ();
> +	  return true;
> +	}
> +      local->root_ino = str.st_ino;
> +      local->root_dev = str.st_dev;
> +    }

I still think you should disable (re)loading if __stat64 fails.

>    __libc_lock_unlock (local->lock);
>  
>    /* Avoid overwriting the global configuration until we have loaded
> diff --git a/nss/nss_module.c b/nss/nss_module.c
> index 1a9359930d..6c5f341f3e 100644
> --- a/nss/nss_module.c
> +++ b/nss/nss_module.c
> @@ -349,6 +349,19 @@ __nss_disable_nscd (void (*cb) (size_t, struct traced_file *))
>  }
>  #endif
>  
> +/* Block attempts to dlopen any module we haven't already opened.  */
> +void
> +__nss_module_disable_loading (void)
> +{
> +  __libc_lock_lock (nss_module_list_lock);
> +
> +  for (struct nss_module *p = nss_module_list; p != NULL; p = p->next)
> +    if (p->state == nss_module_uninitialized)
> +      p->state = nss_module_failed;
> +
> +  __libc_lock_unlock (nss_module_list_lock);
> +}

Maybe call __nss_module_disable_loading after releasing local->lock?
Although there should not be a risk of deadlocks in the current code
because there aren't any relevant function calls while
nss_module_list_lock is locked.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [v2] nsswitch: do not reload if "/" changes

[DJ Delorie via Libc-alpha]

Florian Weimer <fweimer@redhat.com> writes:
> I still think you should disable (re)loading if __stat64 fails.

Ok.

> Maybe call __nss_module_disable_loading after releasing local->lock?

Er, I already do?

+      __libc_lock_unlock (local->lock);
+      __nss_module_disable_loading ();

From c6eda21758158b7bd4861fb8ad3128b0e0660e94 Mon Sep 17 00:00:00 2001
From: DJ Delorie <dj@redhat.com>
Date: Fri, 15 Jan 2021 19:50:00 -0500
Subject: nsswitch: do not reload if "/" changes

https://sourceware.org/bugzilla/show_bug.cgi?id=27077

Before reloading nsswitch.conf, verify that the root directory
hasn't changed - if it has, it's likely that we've entered a
container and should not trust the nsswitch inside the container
nor load any shared objects therein.

diff --git a/nss/Makefile b/nss/Makefile
index 5f6bf598a1..0906202db9 100644
--- a/nss/Makefile
+++ b/nss/Makefile
@@ -67,7 +67,7 @@ tests-container = \
 			  tst-nss-files-hosts-long \
 			  tst-nss-db-endpwent \
 			  tst-nss-db-endgrent \
-			  tst-reload1
+			  tst-reload1 tst-reload2
 
 # Tests which need libdl
 ifeq (yes,$(build-shared))
diff --git a/nss/nss_database.c b/nss/nss_database.c
index e719ec0865..cf0306adc4 100644
--- a/nss/nss_database.c
+++ b/nss/nss_database.c
@@ -33,6 +33,11 @@ struct nss_database_state
 {
   struct nss_database_data data;
   __libc_lock_define (, lock);
+  /* If "/" changes, we switched into a container and do NOT want to
+     reload anything.  This data must be persistent across
+     reloads.  */
+  ino64_t root_ino;
+  dev_t root_dev;
 };
 
 
@@ -54,6 +59,8 @@ global_state_allocate (void *closure)
       result->data.initialized = true;
       result->data.reload_disabled = false;
       __libc_lock_init (result->lock);
+      result->root_ino = 0;
+      result->root_dev = 0;
     }
   return result;
 }
@@ -356,6 +363,8 @@ nss_database_check_reload_and_get (struct nss_database_state *local,
                                    nss_action_list *result,
                                    enum nss_database database_index)
 {
+  struct stat64 str;
+
   /* Acquire MO is needed because the thread that sets reload_disabled
      may have loaded the configuration first, so synchronize with the
      Release MO store there.  */
@@ -379,6 +388,24 @@ nss_database_check_reload_and_get (struct nss_database_state *local,
       __libc_lock_unlock (local->lock);
       return true;
     }
+
+  /* Before we reload, verify that "/" hasn't changed.  We assume that
+     errors here are very unlikely, but the chance that we're entering
+     a container is also very unlikely, so we err on the side of both
+     very unlikely things not happening at the same time.  */
+  if (__stat64 ("/", &str) != 0
+      || (local->root_ino != 0
+	  && (str.st_ino != local->root_ino
+	      ||  str.st_dev != local->root_dev)))
+    {
+      /* Change detected; disable reloading.  */
+      atomic_store_release (&local->data.reload_disabled, 1);
+      __libc_lock_unlock (local->lock);
+      __nss_module_disable_loading ();
+      return true;
+    }
+  local->root_ino = str.st_ino;
+  local->root_dev = str.st_dev;
   __libc_lock_unlock (local->lock);
 
   /* Avoid overwriting the global configuration until we have loaded
diff --git a/nss/nss_module.c b/nss/nss_module.c
index 1a9359930d..6c5f341f3e 100644
--- a/nss/nss_module.c
+++ b/nss/nss_module.c
@@ -349,6 +349,19 @@ __nss_disable_nscd (void (*cb) (size_t, struct traced_file *))
 }
 #endif
 
+/* Block attempts to dlopen any module we haven't already opened.  */
+void
+__nss_module_disable_loading (void)
+{
+  __libc_lock_lock (nss_module_list_lock);
+
+  for (struct nss_module *p = nss_module_list; p != NULL; p = p->next)
+    if (p->state == nss_module_uninitialized)
+      p->state = nss_module_failed;
+
+  __libc_lock_unlock (nss_module_list_lock);
+}
+
 void __libc_freeres_fn_section
 __nss_module_freeres (void)
 {
diff --git a/nss/nss_module.h b/nss/nss_module.h
index 06e8c29040..05c4791d11 100644
--- a/nss/nss_module.h
+++ b/nss/nss_module.h
@@ -87,6 +87,9 @@ bool __nss_module_load (struct nss_module *module) attribute_hidden;
 void *__nss_module_get_function (struct nss_module *module, const char *name)
   attribute_hidden;
 
+/* Block attempts to dlopen any module we haven't already opened.  */
+void __nss_module_disable_loading (void);
+
 /* Called from __libc_freeres.  */
 void __nss_module_freeres (void) attribute_hidden;
 
diff --git a/nss/tst-reload2.c b/nss/tst-reload2.c
new file mode 100644
index 0000000000..afa2cad415
--- /dev/null
+++ b/nss/tst-reload2.c
@@ -0,0 +1,126 @@
+/* Test that reloading is disabled after a chroot.
+   Copyright (C) 2020-2021 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <nss.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <limits.h>
+#include <sys/types.h>
+#include <errno.h>
+#include <pwd.h>
+#include <grp.h>
+#include <unistd.h>
+
+#include <support/support.h>
+#include <support/check.h>
+#include <support/xunistd.h>
+
+#include "nss_test.h"
+
+static struct passwd pwd_table1[] =
+  {
+   PWD_N (1234, "test1"),
+   PWD_N (4321, "test2"),
+   PWD_LAST ()
+  };
+
+static const char *group_4[] = {
+  "alpha", "beta", "gamma", "fred", NULL
+};
+
+static struct group group_table_data[] =
+  {
+   GRP(4), /* match */
+   GRP_LAST ()
+  };
+
+void
+_nss_test1_init_hook (test_tables *t)
+{
+  t->pwd_table = pwd_table1;
+  t->grp_table = group_table_data;
+}
+
+static struct passwd pwd_table2[] =
+  {
+   PWD_N (5, "test1"),
+   PWD_N (2468, "test2"),
+   PWD_LAST ()
+  };
+
+void
+_nss_test2_init_hook (test_tables *t)
+{
+  t->pwd_table = pwd_table2;
+}
+
+static int
+do_test (void)
+{
+  struct passwd *pw;
+  struct group *gr;
+  char buf1[PATH_MAX];
+  char buf2[PATH_MAX];
+
+  sprintf (buf1, "/subdir%s", support_slibdir_prefix);
+  xmkdirp (buf1, 0777);
+
+  /* Copy this DSO into the chroot so it *could* be loaded.  */
+  sprintf (buf1, "%s/libnss_files.so.2", support_slibdir_prefix);
+  sprintf (buf2, "/subdir%s/libnss_files.so.2", support_slibdir_prefix);
+  support_copy_file (buf1, buf2);
+  
+  /* Check we're using the "outer" nsswitch.conf.  */
+
+  /* This uses the test1 DSO.  */
+  pw = getpwnam("test1");
+  TEST_VERIFY (pw != NULL);
+  if (pw)
+    TEST_COMPARE (pw->pw_uid, 1234);
+
+  /* This just loads the test2 DSO.  */
+  gr = getgrnam("name4");
+
+  /* Change the root dir.  */
+
+  TEST_VERIFY (chroot ("/subdir") == 0);
+  chdir("/");
+
+  /* Check we're NOT using the "inner" nsswitch.conf.  */
+
+  /* Both DSOs are loaded, which is used?  */
+  pw = getpwnam("test2");
+  TEST_VERIFY (pw != NULL);
+  if (pw)
+    TEST_VERIFY (pw->pw_uid != 2468);
+
+  /* The "files" DSO should not be loaded.  */
+  gr = getgrnam("test3");
+  TEST_VERIFY (gr == NULL);
+
+  /* We should still be using the old configuration.  */
+  pw = getpwnam("test1");
+  TEST_VERIFY (pw != NULL);
+  if (pw)
+    TEST_COMPARE (pw->pw_uid, 1234);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/nss/tst-reload2.root/etc/nsswitch.conf b/nss/tst-reload2.root/etc/nsswitch.conf
new file mode 100644
index 0000000000..570795ae22
--- /dev/null
+++ b/nss/tst-reload2.root/etc/nsswitch.conf
@@ -0,0 +1,2 @@
+passwd: test1
+group: test2
diff --git a/nss/tst-reload2.root/subdir/etc/group b/nss/tst-reload2.root/subdir/etc/group
new file mode 100644
index 0000000000..e48646bd47
--- /dev/null
+++ b/nss/tst-reload2.root/subdir/etc/group
@@ -0,0 +1 @@
+test3:x:123:
diff --git a/nss/tst-reload2.root/subdir/etc/nsswitch.conf b/nss/tst-reload2.root/subdir/etc/nsswitch.conf
new file mode 100644
index 0000000000..f1d73f8765
--- /dev/null
+++ b/nss/tst-reload2.root/subdir/etc/nsswitch.conf
@@ -0,0 +1,2 @@
+passwd: test2
+group: files
diff --git a/nss/tst-reload2.root/tst-reload2.script b/nss/tst-reload2.root/tst-reload2.script
new file mode 100644
index 0000000000..c6ee4b8e5e
--- /dev/null
+++ b/nss/tst-reload2.root/tst-reload2.script
@@ -0,0 +1,3 @@
+su
+cp $B/nss/libnss_test1.so $L/libnss_test1.so.2
+cp $B/nss/libnss_test2.so $L/libnss_test2.so.2

Re: [v2] nsswitch: do not reload if "/" changes

[Florian Weimer via Libc-alpha]

* DJ Delorie:

> Florian Weimer <fweimer@redhat.com> writes:
>> I still think you should disable (re)loading if __stat64 fails.
>
> Ok.
>
>> Maybe call __nss_module_disable_loading after releasing local->lock?
>
> Er, I already do?
>
> +      __libc_lock_unlock (local->lock);
> +      __nss_module_disable_loading ();

Ah, right.  I was confused.

> +  /* Before we reload, verify that "/" hasn't changed.  We assume that
> +     errors here are very unlikely, but the chance that we're entering
> +     a container is also very unlikely, so we err on the side of both
> +     very unlikely things not happening at the same time.  */
> +  if (__stat64 ("/", &str) != 0
> +      || (local->root_ino != 0
> +	  && (str.st_ino != local->root_ino
> +	      ||  str.st_dev != local->root_dev)))
> +    {
> +      /* Change detected; disable reloading.  */
> +      atomic_store_release (&local->data.reload_disabled, 1);
> +      __libc_lock_unlock (local->lock);
> +      __nss_module_disable_loading ();
> +      return true;
> +    }

0 used to be a valid inode number for XFS, but I suspect a lot will break if /
uses it.

There are some style issues:

+   GRP(4), /* match */
+  
+  pw = getpwnam("test1");
+  gr = getgrnam("name4");
+  chdir("/");
+  pw = getpwnam("test2");
+  gr = getgrnam("test3");
+  pw = getpwnam("test1");

Comment formatting, trailing whitespace, missing space before '('.

Rest looks okay.  I assume the test fails in the expected way without
the patch. 8-)

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [v2] nsswitch: do not reload if "/" changes

[DJ Delorie via Libc-alpha]

Florian Weimer <fweimer@redhat.com> writes:
> There are some style issues:
> Comment formatting, trailing whitespace, missing space before '('.

Fixed.

> I assume the test fails in the expected way without the patch. 8-)

Yup.

From 5a1b7ace008ca4c2f5f1683b624f414e38226482 Mon Sep 17 00:00:00 2001
From: DJ Delorie <dj@redhat.com>
Date: Fri, 15 Jan 2021 19:50:00 -0500
Subject: nsswitch: do not reload if "/" changes

https://sourceware.org/bugzilla/show_bug.cgi?id=27077

Before reloading nsswitch.conf, verify that the root directory
hasn't changed - if it has, it's likely that we've entered a
container and should not trust the nsswitch inside the container
nor load any shared objects therein.

diff --git a/nss/Makefile b/nss/Makefile
index 5f6bf598a1..0906202db9 100644
--- a/nss/Makefile
+++ b/nss/Makefile
@@ -67,7 +67,7 @@ tests-container = \
 			  tst-nss-files-hosts-long \
 			  tst-nss-db-endpwent \
 			  tst-nss-db-endgrent \
-			  tst-reload1
+			  tst-reload1 tst-reload2
 
 # Tests which need libdl
 ifeq (yes,$(build-shared))
diff --git a/nss/nss_database.c b/nss/nss_database.c
index e719ec0865..cf0306adc4 100644
--- a/nss/nss_database.c
+++ b/nss/nss_database.c
@@ -33,6 +33,11 @@ struct nss_database_state
 {
   struct nss_database_data data;
   __libc_lock_define (, lock);
+  /* If "/" changes, we switched into a container and do NOT want to
+     reload anything.  This data must be persistent across
+     reloads.  */
+  ino64_t root_ino;
+  dev_t root_dev;
 };
 
 
@@ -54,6 +59,8 @@ global_state_allocate (void *closure)
       result->data.initialized = true;
       result->data.reload_disabled = false;
       __libc_lock_init (result->lock);
+      result->root_ino = 0;
+      result->root_dev = 0;
     }
   return result;
 }
@@ -356,6 +363,8 @@ nss_database_check_reload_and_get (struct nss_database_state *local,
                                    nss_action_list *result,
                                    enum nss_database database_index)
 {
+  struct stat64 str;
+
   /* Acquire MO is needed because the thread that sets reload_disabled
      may have loaded the configuration first, so synchronize with the
      Release MO store there.  */
@@ -379,6 +388,24 @@ nss_database_check_reload_and_get (struct nss_database_state *local,
       __libc_lock_unlock (local->lock);
       return true;
     }
+
+  /* Before we reload, verify that "/" hasn't changed.  We assume that
+     errors here are very unlikely, but the chance that we're entering
+     a container is also very unlikely, so we err on the side of both
+     very unlikely things not happening at the same time.  */
+  if (__stat64 ("/", &str) != 0
+      || (local->root_ino != 0
+	  && (str.st_ino != local->root_ino
+	      ||  str.st_dev != local->root_dev)))
+    {
+      /* Change detected; disable reloading.  */
+      atomic_store_release (&local->data.reload_disabled, 1);
+      __libc_lock_unlock (local->lock);
+      __nss_module_disable_loading ();
+      return true;
+    }
+  local->root_ino = str.st_ino;
+  local->root_dev = str.st_dev;
   __libc_lock_unlock (local->lock);
 
   /* Avoid overwriting the global configuration until we have loaded
diff --git a/nss/nss_module.c b/nss/nss_module.c
index 1a9359930d..6c5f341f3e 100644
--- a/nss/nss_module.c
+++ b/nss/nss_module.c
@@ -349,6 +349,19 @@ __nss_disable_nscd (void (*cb) (size_t, struct traced_file *))
 }
 #endif
 
+/* Block attempts to dlopen any module we haven't already opened.  */
+void
+__nss_module_disable_loading (void)
+{
+  __libc_lock_lock (nss_module_list_lock);
+
+  for (struct nss_module *p = nss_module_list; p != NULL; p = p->next)
+    if (p->state == nss_module_uninitialized)
+      p->state = nss_module_failed;
+
+  __libc_lock_unlock (nss_module_list_lock);
+}
+
 void __libc_freeres_fn_section
 __nss_module_freeres (void)
 {
diff --git a/nss/nss_module.h b/nss/nss_module.h
index 06e8c29040..05c4791d11 100644
--- a/nss/nss_module.h
+++ b/nss/nss_module.h
@@ -87,6 +87,9 @@ bool __nss_module_load (struct nss_module *module) attribute_hidden;
 void *__nss_module_get_function (struct nss_module *module, const char *name)
   attribute_hidden;
 
+/* Block attempts to dlopen any module we haven't already opened.  */
+void __nss_module_disable_loading (void);
+
 /* Called from __libc_freeres.  */
 void __nss_module_freeres (void) attribute_hidden;
 
diff --git a/nss/tst-reload2.c b/nss/tst-reload2.c
new file mode 100644
index 0000000000..b69d15edd1
--- /dev/null
+++ b/nss/tst-reload2.c
@@ -0,0 +1,126 @@
+/* Test that reloading is disabled after a chroot.
+   Copyright (C) 2020-2021 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <nss.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <limits.h>
+#include <sys/types.h>
+#include <errno.h>
+#include <pwd.h>
+#include <grp.h>
+#include <unistd.h>
+
+#include <support/support.h>
+#include <support/check.h>
+#include <support/xunistd.h>
+
+#include "nss_test.h"
+
+static struct passwd pwd_table1[] =
+  {
+   PWD_N (1234, "test1"),
+   PWD_N (4321, "test2"),
+   PWD_LAST ()
+  };
+
+static const char *group_4[] = {
+  "alpha", "beta", "gamma", "fred", NULL
+};
+
+static struct group group_table_data[] =
+  {
+   GRP(4),
+   GRP_LAST ()
+  };
+
+void
+_nss_test1_init_hook (test_tables *t)
+{
+  t->pwd_table = pwd_table1;
+  t->grp_table = group_table_data;
+}
+
+static struct passwd pwd_table2[] =
+  {
+   PWD_N (5, "test1"),
+   PWD_N (2468, "test2"),
+   PWD_LAST ()
+  };
+
+void
+_nss_test2_init_hook (test_tables *t)
+{
+  t->pwd_table = pwd_table2;
+}
+
+static int
+do_test (void)
+{
+  struct passwd *pw;
+  struct group *gr;
+  char buf1[PATH_MAX];
+  char buf2[PATH_MAX];
+
+  sprintf (buf1, "/subdir%s", support_slibdir_prefix);
+  xmkdirp (buf1, 0777);
+
+  /* Copy this DSO into the chroot so it *could* be loaded.  */
+  sprintf (buf1, "%s/libnss_files.so.2", support_slibdir_prefix);
+  sprintf (buf2, "/subdir%s/libnss_files.so.2", support_slibdir_prefix);
+  support_copy_file (buf1, buf2);
+
+  /* Check we're using the "outer" nsswitch.conf.  */
+
+  /* This uses the test1 DSO.  */
+  pw = getpwnam ("test1");
+  TEST_VERIFY (pw != NULL);
+  if (pw)
+    TEST_COMPARE (pw->pw_uid, 1234);
+
+  /* This just loads the test2 DSO.  */
+  gr = getgrnam ("name4");
+
+  /* Change the root dir.  */
+
+  TEST_VERIFY (chroot ("/subdir") == 0);
+  chdir ("/");
+
+  /* Check we're NOT using the "inner" nsswitch.conf.  */
+
+  /* Both DSOs are loaded, which is used?  */
+  pw = getpwnam ("test2");
+  TEST_VERIFY (pw != NULL);
+  if (pw)
+    TEST_VERIFY (pw->pw_uid != 2468);
+
+  /* The "files" DSO should not be loaded.  */
+  gr = getgrnam ("test3");
+  TEST_VERIFY (gr == NULL);
+
+  /* We should still be using the old configuration.  */
+  pw = getpwnam ("test1");
+  TEST_VERIFY (pw != NULL);
+  if (pw)
+    TEST_COMPARE (pw->pw_uid, 1234);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/nss/tst-reload2.root/etc/nsswitch.conf b/nss/tst-reload2.root/etc/nsswitch.conf
new file mode 100644
index 0000000000..570795ae22
--- /dev/null
+++ b/nss/tst-reload2.root/etc/nsswitch.conf
@@ -0,0 +1,2 @@
+passwd: test1
+group: test2
diff --git a/nss/tst-reload2.root/subdir/etc/group b/nss/tst-reload2.root/subdir/etc/group
new file mode 100644
index 0000000000..e48646bd47
--- /dev/null
+++ b/nss/tst-reload2.root/subdir/etc/group
@@ -0,0 +1 @@
+test3:x:123:
diff --git a/nss/tst-reload2.root/subdir/etc/nsswitch.conf b/nss/tst-reload2.root/subdir/etc/nsswitch.conf
new file mode 100644
index 0000000000..f1d73f8765
--- /dev/null
+++ b/nss/tst-reload2.root/subdir/etc/nsswitch.conf
@@ -0,0 +1,2 @@
+passwd: test2
+group: files
diff --git a/nss/tst-reload2.root/tst-reload2.script b/nss/tst-reload2.root/tst-reload2.script
new file mode 100644
index 0000000000..c6ee4b8e5e
--- /dev/null
+++ b/nss/tst-reload2.root/tst-reload2.script
@@ -0,0 +1,3 @@
+su
+cp $B/nss/libnss_test1.so $L/libnss_test1.so.2
+cp $B/nss/libnss_test2.so $L/libnss_test2.so.2

Re: [v2] nsswitch: do not reload if "/" changes

[Carlos O'Donell via Libc-alpha]

On 1/26/21 11:47 AM, DJ Delorie via Libc-alpha wrote:
> Florian Weimer <fweimer@redhat.com> writes:
>> There are some style issues:
>> Comment formatting, trailing whitespace, missing space before '('.
> 
> Fixed.
> 
>> I assume the test fails in the expected way without the patch. 8-)
> 
> Yup.

This fixes the whitespace issue so this is good to go in.

Caught one final whitespace error, but you're going to fix that (noted in review).

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
 
> From 5a1b7ace008ca4c2f5f1683b624f414e38226482 Mon Sep 17 00:00:00 2001
> From: DJ Delorie <dj@redhat.com>
> Date: Fri, 15 Jan 2021 19:50:00 -0500
> Subject: nsswitch: do not reload if "/" changes
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=27077
> 
> Before reloading nsswitch.conf, verify that the root directory
> hasn't changed - if it has, it's likely that we've entered a
> container and should not trust the nsswitch inside the container
> nor load any shared objects therein.
> 
> diff --git a/nss/Makefile b/nss/Makefile
> index 5f6bf598a1..0906202db9 100644
> --- a/nss/Makefile
> +++ b/nss/Makefile
> @@ -67,7 +67,7 @@ tests-container = \
>  			  tst-nss-files-hosts-long \
>  			  tst-nss-db-endpwent \
>  			  tst-nss-db-endgrent \
> -			  tst-reload1
> +			  tst-reload1 tst-reload2

OK. Add test.

>  
>  # Tests which need libdl
>  ifeq (yes,$(build-shared))
> diff --git a/nss/nss_database.c b/nss/nss_database.c
> index e719ec0865..cf0306adc4 100644
> --- a/nss/nss_database.c
> +++ b/nss/nss_database.c
> @@ -33,6 +33,11 @@ struct nss_database_state
>  {
>    struct nss_database_data data;
>    __libc_lock_define (, lock);
> +  /* If "/" changes, we switched into a container and do NOT want to
> +     reload anything.  This data must be persistent across
> +     reloads.  */
> +  ino64_t root_ino;
> +  dev_t root_dev;

OK.

>  };
>  
>  
> @@ -54,6 +59,8 @@ global_state_allocate (void *closure)
>        result->data.initialized = true;
>        result->data.reload_disabled = false;
>        __libc_lock_init (result->lock);
> +      result->root_ino = 0;
> +      result->root_dev = 0;

OK.

>      }
>    return result;
>  }
> @@ -356,6 +363,8 @@ nss_database_check_reload_and_get (struct nss_database_state *local,
>                                     nss_action_list *result,
>                                     enum nss_database database_index)
>  {
> +  struct stat64 str;
> +
>    /* Acquire MO is needed because the thread that sets reload_disabled
>       may have loaded the configuration first, so synchronize with the
>       Release MO store there.  */
> @@ -379,6 +388,24 @@ nss_database_check_reload_and_get (struct nss_database_state *local,
>        __libc_lock_unlock (local->lock);
>        return true;
>      }
> +
> +  /* Before we reload, verify that "/" hasn't changed.  We assume that
> +     errors here are very unlikely, but the chance that we're entering
> +     a container is also very unlikely, so we err on the side of both
> +     very unlikely things not happening at the same time.  */
> +  if (__stat64 ("/", &str) != 0
> +      || (local->root_ino != 0
> +	  && (str.st_ino != local->root_ino
> +	      ||  str.st_dev != local->root_dev)))
> +    {
> +      /* Change detected; disable reloading.  */
> +      atomic_store_release (&local->data.reload_disabled, 1);
> +      __libc_lock_unlock (local->lock);
> +      __nss_module_disable_loading ();

OK. If stat fails we see it as a root change (good, fails safe).
OK. If the ino or dev change then root change (good).

> +      return true;
> +    }
> +  local->root_ino = str.st_ino;
> +  local->root_dev = str.st_dev;
>    __libc_lock_unlock (local->lock);
>  
>    /* Avoid overwriting the global configuration until we have loaded
> diff --git a/nss/nss_module.c b/nss/nss_module.c
> index 1a9359930d..6c5f341f3e 100644
> --- a/nss/nss_module.c
> +++ b/nss/nss_module.c
> @@ -349,6 +349,19 @@ __nss_disable_nscd (void (*cb) (size_t, struct traced_file *))
>  }
>  #endif
>  
> +/* Block attempts to dlopen any module we haven't already opened.  */
> +void
> +__nss_module_disable_loading (void)
> +{
> +  __libc_lock_lock (nss_module_list_lock);
> +
> +  for (struct nss_module *p = nss_module_list; p != NULL; p = p->next)
> +    if (p->state == nss_module_uninitialized)
> +      p->state = nss_module_failed;

OK. Reusing nss_module_failed for this state.

> +
> +  __libc_lock_unlock (nss_module_list_lock);
> +}
> +
>  void __libc_freeres_fn_section
>  __nss_module_freeres (void)
>  {
> diff --git a/nss/nss_module.h b/nss/nss_module.h
> index 06e8c29040..05c4791d11 100644
> --- a/nss/nss_module.h
> +++ b/nss/nss_module.h
> @@ -87,6 +87,9 @@ bool __nss_module_load (struct nss_module *module) attribute_hidden;
>  void *__nss_module_get_function (struct nss_module *module, const char *name)
>    attribute_hidden;
>  
> +/* Block attempts to dlopen any module we haven't already opened.  */
> +void __nss_module_disable_loading (void);

OK.

> +
>  /* Called from __libc_freeres.  */
>  void __nss_module_freeres (void) attribute_hidden;
>  
> diff --git a/nss/tst-reload2.c b/nss/tst-reload2.c
> new file mode 100644
> index 0000000000..b69d15edd1
> --- /dev/null
> +++ b/nss/tst-reload2.c
> @@ -0,0 +1,126 @@
> +/* Test that reloading is disabled after a chroot.
> +   Copyright (C) 2020-2021 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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <nss.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <limits.h>
> +#include <sys/types.h>
> +#include <errno.h>
> +#include <pwd.h>
> +#include <grp.h>
> +#include <unistd.h>
> +
> +#include <support/support.h>
> +#include <support/check.h>
> +#include <support/xunistd.h>
> +
> +#include "nss_test.h"
> +
> +static struct passwd pwd_table1[] =
> +  {
> +   PWD_N (1234, "test1"),
> +   PWD_N (4321, "test2"),
> +   PWD_LAST ()
> +  };
> +
> +static const char *group_4[] = {
> +  "alpha", "beta", "gamma", "fred", NULL
> +};
> +
> +static struct group group_table_data[] =
> +  {
> +   GRP(4),

Needs a space. Just discussed this with you and you'll fix on commit.

> +   GRP_LAST ()
> +  };
> +
> +void
> +_nss_test1_init_hook (test_tables *t)
> +{
> +  t->pwd_table = pwd_table1;
> +  t->grp_table = group_table_data;
> +}
> +
> +static struct passwd pwd_table2[] =
> +  {
> +   PWD_N (5, "test1"),
> +   PWD_N (2468, "test2"),
> +   PWD_LAST ()
> +  };
> +
> +void
> +_nss_test2_init_hook (test_tables *t)
> +{
> +  t->pwd_table = pwd_table2;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  struct passwd *pw;
> +  struct group *gr;
> +  char buf1[PATH_MAX];
> +  char buf2[PATH_MAX];
> +
> +  sprintf (buf1, "/subdir%s", support_slibdir_prefix);
> +  xmkdirp (buf1, 0777);
> +
> +  /* Copy this DSO into the chroot so it *could* be loaded.  */
> +  sprintf (buf1, "%s/libnss_files.so.2", support_slibdir_prefix);
> +  sprintf (buf2, "/subdir%s/libnss_files.so.2", support_slibdir_prefix);
> +  support_copy_file (buf1, buf2);
> +
> +  /* Check we're using the "outer" nsswitch.conf.  */
> +
> +  /* This uses the test1 DSO.  */
> +  pw = getpwnam ("test1");
> +  TEST_VERIFY (pw != NULL);
> +  if (pw)
> +    TEST_COMPARE (pw->pw_uid, 1234);
> +
> +  /* This just loads the test2 DSO.  */
> +  gr = getgrnam ("name4");
> +
> +  /* Change the root dir.  */
> +
> +  TEST_VERIFY (chroot ("/subdir") == 0);

OK.

> +  chdir ("/");
> +
> +  /* Check we're NOT using the "inner" nsswitch.conf.  */
> +
> +  /* Both DSOs are loaded, which is used?  */
> +  pw = getpwnam ("test2");
> +  TEST_VERIFY (pw != NULL);
> +  if (pw)
> +    TEST_VERIFY (pw->pw_uid != 2468);
> +
> +  /* The "files" DSO should not be loaded.  */
> +  gr = getgrnam ("test3");
> +  TEST_VERIFY (gr == NULL);
> +
> +  /* We should still be using the old configuration.  */
> +  pw = getpwnam ("test1");
> +  TEST_VERIFY (pw != NULL);
> +  if (pw)
> +    TEST_COMPARE (pw->pw_uid, 1234);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/nss/tst-reload2.root/etc/nsswitch.conf b/nss/tst-reload2.root/etc/nsswitch.conf
> new file mode 100644
> index 0000000000..570795ae22
> --- /dev/null
> +++ b/nss/tst-reload2.root/etc/nsswitch.conf
> @@ -0,0 +1,2 @@
> +passwd: test1
> +group: test2
> diff --git a/nss/tst-reload2.root/subdir/etc/group b/nss/tst-reload2.root/subdir/etc/group
> new file mode 100644
> index 0000000000..e48646bd47
> --- /dev/null
> +++ b/nss/tst-reload2.root/subdir/etc/group
> @@ -0,0 +1 @@
> +test3:x:123:
> diff --git a/nss/tst-reload2.root/subdir/etc/nsswitch.conf b/nss/tst-reload2.root/subdir/etc/nsswitch.conf
> new file mode 100644
> index 0000000000..f1d73f8765
> --- /dev/null
> +++ b/nss/tst-reload2.root/subdir/etc/nsswitch.conf
> @@ -0,0 +1,2 @@
> +passwd: test2
> +group: files
> diff --git a/nss/tst-reload2.root/tst-reload2.script b/nss/tst-reload2.root/tst-reload2.script
> new file mode 100644
> index 0000000000..c6ee4b8e5e
> --- /dev/null
> +++ b/nss/tst-reload2.root/tst-reload2.script
> @@ -0,0 +1,3 @@
> +su
> +cp $B/nss/libnss_test1.so $L/libnss_test1.so.2
> +cp $B/nss/libnss_test2.so $L/libnss_test2.so.2
> 

OK.

-- 
Cheers,
Carlos.

Re: [v2] nsswitch: do not reload if "/" changes

[DJ Delorie via Libc-alpha]

"Carlos O'Donell" <carlos@redhat.com> writes:
> This fixes the whitespace issue so this is good to go in.
>
> Caught one final whitespace error, but you're going to fix that (noted in review).
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>

Thanks, pushed!

Re: [v2] nsswitch: do not reload if "/" changes

[Joseph Myers]

This has broken the testsuite build for Hurd.

tst-reload2.c: In function 'do_test':
tst-reload2.c:78:13: error: 'PATH_MAX' undeclared (first use in this function)
   78 |   char buf1[PATH_MAX];
      |             ^~~~~~~~
tst-reload2.c:78:13: note: each undeclared identifier is reported only once for each function it appears in

https://sourceware.org/pipermail/libc-testresults/2021q1/007436.html

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [v2] nsswitch: do not reload if "/" changes

[DJ Delorie via Libc-alpha]

What's the right macro for Hurd then?

Re: [v2] nsswitch: do not reload if "/" changes

[Joseph Myers]

On Thu, 28 Jan 2021, DJ Delorie wrote:

> What's the right macro for Hurd then?

Hurd does not have a limit on path length, but some tests do e.g.

#ifndef PATH_MAX
# define PATH_MAX 4096
#endif

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [v2] nsswitch: do not reload if "/" changes

[DJ Delorie via Libc-alpha]

Joseph Myers <joseph@codesourcery.com> writes:
> Hurd does not have a limit on path length, but some tests do e.g.
>
> #ifndef PATH_MAX
> # define PATH_MAX 4096
> #endif

Ah.  I committed this as obvious, which was copied from one of the other
tests (my test doesn't need that much buffer space anyway)

From 757a14b5ac7c736c759605f4b674cae28d752116 Mon Sep 17 00:00:00 2001
From: DJ Delorie <dj@redhat.com>
Date: Wed, 27 Jan 2021 20:05:26 -0500
Subject: Fix nss/tst-reload2 for systems without PATH_MAX


diff --git a/nss/tst-reload2.c b/nss/tst-reload2.c
index 128db25ae6..5dae16b4f0 100644
--- a/nss/tst-reload2.c
+++ b/nss/tst-reload2.c
@@ -33,6 +33,10 @@
 
 #include "nss_test.h"
 
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
 static struct passwd pwd_table1[] =
   {
    PWD_N (1234, "test1"),

Re: nsswitch: do not reload if "/" changes

[Andreas Schwab]

How does that interact with pivot_root?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: nsswitch: do not reload if "/" changes

[Florian Weimer via Libc-alpha]

* Andreas Schwab:

> How does that interact with pivot_root?

I expect it to disable reloading, as a safety measure.  Behavior will be
as before: If NSS was initialized before pivot_root, the old
/etc/nsswitch.conf will be used for the remaining life-time of the
process.  If not, the new configuration file will be read.

I don't think there is a way around that.  Maybe we could add a special
__nss_configure_lookup call to re-enable reloading for the cases where
this is safe.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: nsswitch: do not reload if "/" changes

[DJ Delorie via Libc-alpha]

Andreas Schwab <schwab@linux-m68k.org> writes:
> How does that interact with pivot_root?

If the initroot has an nsswitch.conf (I can't imagine why, but it
wouldn't surprise me if the initroot had a copy of emacs) then
pivot_root would lock that version, just like before.  If the initroot
and the pivot root have different versions of nss modules, something has
gone horribly wrong building your distro.

Note that the pivot_root(8) man page says you should exec a chroot after
pivoting anyway, which resets nsswitch.conf.  The pivot_root(2) man page
says that changes to the current process's / and pwd may or may not
change, and it's up to the calling process to deal with it.

Either way, I don't think we have to worry about it.

Re: nsswitch: do not reload if "/" changes

[Carlos O'Donell via Libc-alpha]

On 1/15/21 7:59 PM, DJ Delorie via Libc-alpha wrote:
> 
> [Note: I tried putting this functionality in the file_change_detection
> module, but that didn't have enough persistence.]
> 
> [Note: tested by instrumenting test-container.c and observing the
> instrumentation with test containers on the root fs and on a separate
> fs]
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=27077
> 
> Before reloading nsswitch.conf, verify that the root directory
> hasn't changed - if it has, it's likely that we've entered a
> container and should not trust the nsswitch inside the container
> nor load any shared objects therein.

Can we create a non-test-container test for this?

I think you can use support_become_root to unshare and then try
to use support_chroot_create/support_chroot_free and xhcroot to 
change root, and then try to do an NSS call that will fail?

The test can start by calling __nss_lookup_configure to set
a known module to provide the NSS values, and then do an IdM
call, verify you're using a known value, and then try to
become root and chroot.

I'm not sure if this is possible though.

-- 
Cheers,
Carlos.

Re: nsswitch: do not reload if "/" changes

[Florian Weimer via Libc-alpha]

* Carlos O'Donell via Libc-alpha:

> Can we create a non-test-container test for this?
>
> I think you can use support_become_root to unshare and then try
> to use support_chroot_create/support_chroot_free and xhcroot to 
> change root, and then try to do an NSS call that will fail?

You need to chroot twice, first to get a defined /etc/nsswitch.conf, and
another one to make sure things don't ger reloaded after chroot.

You probably also have to copy different service modules into the two
chroots.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: nsswitch: do not reload if "/" changes

[Carlos O'Donell via Libc-alpha]

On 1/18/21 11:53 AM, Florian Weimer wrote:
> * Carlos O'Donell via Libc-alpha:
> 
>> Can we create a non-test-container test for this?
>>
>> I think you can use support_become_root to unshare and then try
>> to use support_chroot_create/support_chroot_free and xhcroot to 
>> change root, and then try to do an NSS call that will fail?
> 
> You need to chroot twice, first to get a defined /etc/nsswitch.conf, and
> another one to make sure things don't ger reloaded after chroot.

That would be a perfect solution.

However, I think you could get away with recording some known uid/gid
from the system that was doing the build and then ensure that value
is not present in the container.

> You probably also have to copy different service modules into the two
> chroots.

Correct.

-- 
Cheers,
Carlos.

Re: nsswitch: do not reload if "/" changes

[Florian Weimer via Libc-alpha]

* Carlos O'Donell:

> On 1/18/21 11:53 AM, Florian Weimer wrote:
>> * Carlos O'Donell via Libc-alpha:
>> 
>>> Can we create a non-test-container test for this?
>>>
>>> I think you can use support_become_root to unshare and then try
>>> to use support_chroot_create/support_chroot_free and xhcroot to 
>>> change root, and then try to do an NSS call that will fail?
>> 
>> You need to chroot twice, first to get a defined /etc/nsswitch.conf, and
>> another one to make sure things don't ger reloaded after chroot.
>
> That would be a perfect solution.
>
> However, I think you could get away with recording some known uid/gid
> from the system that was doing the build and then ensure that value
> is not present in the container.

The issue is with loading NSS modules from the system.  This can lead to
crashes/unpredictable behavior.  We've been through this already, if I
recall correctly.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: nsswitch: do not reload if "/" changes

[DJ Delorie via Libc-alpha]

"Carlos O'Donell" <carlos@redhat.com> writes:
> I'm not sure if this is possible though.

The tricky part is you only get *one* opportunity to change "/" so you
can't, for example, setup a pre- and post- testroot, unless - and this
is a maybe - nothing else invokes nsswitch.conf between exec() and
do_test().

And note that we can only test for the st_ino changing case, as creating
an environment where the testroot is "/" in a mounted filesystem (and
thus has st_ino 2 but a different st_dev) is way outside of glibc's
testsuite's scope.  Not a big deal IMHO.

Note that inside the testroot we should be able to do a simpler form of
chroot, though.  I don't think any test has tried this yet.