diff mbox series

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

Message ID xnft2svlcc.fsf@greed.delorie.com
State New
Headers show
Series [v2] nsswitch: do not reload if "/" changes | expand

Commit Message

DJ Delorie Jan. 22, 2021, 7:10 p.m. UTC
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.

Comments

Florian Weimer Jan. 26, 2021, 9:58 a.m. UTC | #1
* 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
diff mbox series

Patch

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