nsswitch: do not reload if "/" changes

[DJ Delorie]

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