diff mbox series

nsswitch: do not reload if "/" changes

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

Commit Message

DJ Delorie Jan. 16, 2021, 12:59 a.m. UTC
[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.

Comments

Florian Weimer Jan. 16, 2021, 10:52 a.m. UTC | #1
* 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
Andreas Schwab Jan. 18, 2021, 12:42 p.m. UTC | #2
How does that interact with pivot_root?

Andreas.
Florian Weimer Jan. 18, 2021, 12:53 p.m. UTC | #3
* 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
Carlos O'Donell Jan. 18, 2021, 3:59 p.m. UTC | #4
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.
Florian Weimer Jan. 18, 2021, 4:53 p.m. UTC | #5
* 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
Carlos O'Donell Jan. 19, 2021, 2:30 p.m. UTC | #6
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.
Florian Weimer Jan. 19, 2021, 2:40 p.m. UTC | #7
* 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
diff mbox series

Patch

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.  */