nsswitch: do not reload if "/" changes
Commit Message
[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
* 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
How does that interact with pivot_root?
Andreas.
* 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
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.
* 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
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.
* 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
@@ -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
@@ -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. */