| Message ID | xnsg71wvbz.fsf@greed.delorie.com |
|---|---|
| State | New |
| Headers | show |
| Series | nsswitch: do not reload if "/" changes | expand |
* 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
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. */