[v2] nsswitch: do not reload if "/" changes
Commit Message
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
* 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
@@ -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))
@@ -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
@@ -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)
{
@@ -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;
new file mode 100644
@@ -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>
new file mode 100644
@@ -0,0 +1,2 @@
+passwd: test1
+group: test2
new file mode 100644
@@ -0,0 +1 @@
+test3:x:123:
new file mode 100644
@@ -0,0 +1,2 @@
+passwd: test2
+group: files
new file mode 100644
@@ -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