[PATCH v2 0/1] nss: return early in DB reload-and-get if newfstatat fails (BZ #28752)

[PATCH v2 0/1] nss: return early in DB reload-and-get if newfstatat fails (BZ #28752)

[Sam James]

Changes to v1:
Fix silly copy/paste typo.

Sam James (1):
  nss: return early in DB reload-and-get if newfstatat fails (BZ #28752)

 nss/nss_database.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

-- 
2.35.1

[PATCH v2 1/1] nss: return early in DB reload-and-get if newfstatat fails (BZ #28752)

[Sam James]

In some circumstances, the __stat64_time64() call in
nss_database_check_reload_and_get() might fail (via e.g. newfstatat
being filtered by seccomp in parent).

We have to check its return value to avoid an out of bounds access later
on if the call failed.

This manifests as Firefox crashing at runtime when e.g. glib is
compiled with FAM support, which ends up taking this NSS path.

Bug: https://sourceware.org/pipermail/libc-help/2021-December/006061.html
Bug: https://bugs.gentoo.org/828070
Suggested-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Signed-off-by: Sam James <sam@gentoo.org>
---
 nss/nss_database.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/nss/nss_database.c b/nss/nss_database.c
index d56c5b798d..a0522ea7d2 100644
--- a/nss/nss_database.c
+++ b/nss/nss_database.c
@@ -424,17 +424,21 @@ nss_database_check_reload_and_get (struct nss_database_state *local,
      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_time64 ("/", &str) != 0
-      || (local->root_ino != 0
-	  && (str.st_ino != local->root_ino
-	      ||  str.st_dev != local->root_dev)))
-    {
+  if (__stat64_time64 ("/", &str) != 0) {
+    __libc_lock_unlock (local->lock);
+    return false;
+  }
+
+  if (local->root_ino != 0 && (str.st_ino != local->root_ino
+                              || str.st_dev != local->root_dev))
+   {
       /* Change detected; disable reloading and return current state.  */
       atomic_store_release (&local->data.reload_disabled, 1);
       *result = local->data.services[database_index];
       __libc_lock_unlock (local->lock);
       return true;
     }
+
   local->root_ino = str.st_ino;
   local->root_dev = str.st_dev;
   __libc_lock_unlock (local->lock);
-- 
2.35.1

Re: [PATCH v2 1/1] nss: return early in DB reload-and-get if newfstatat fails (BZ #28752)

[DJ Delorie]

Sam James via Libc-alpha <libc-alpha@sourceware.org> writes:

> +  if (__stat64_time64 ("/", &str) != 0) {
> +    __libc_lock_unlock (local->lock);
> +    return false;
> +  }

The new code no longer considers "stat failed" as a reason to prevent
loading new modules?

So we have two cases:

1. "stat failed" might mean we've entered a container, and stat is
   blocked.  We want to stop loading new modules, since they might not
   match ABI-wise with our "outside container" environment, and since we
   might not be able to load them anyway.

2. "stat failed" might mean we're in a firefox sandbox, and stat is
   blocked [incorrectly?].  We likely can't load anything.

(well, and 3. some other really rare error occurred, which, as per the
comment, we ignore ;)

I think the purpose of your patch is to allow one type of bad behavior
(allowing module reload inside a seccomp'd container) to cover up a
second type of bad behavior (preventing reloads leads to NULL
dereferences instead of EPERM).  I.e. it's intentionally bypassing the
results we want, to cover up a bug elsewhere.

I think the real bug is here, in nss/XXX-lookup.c:

  int
  DB_LOOKUP_FCT (nss_action_list *ni, const char *fct_name, const char *fct2_name,
  	       void **fctp)
  {
    if (! __nss_database_get (DATABASE_NAME_ID, &DATABASE_NAME_SYMBOL))
      return -1;
  
    *ni = DATABASE_NAME_SYMBOL;
  
    return __nss_lookup (ni, fct_name, fct2_name, fctp);
  }


It looks like__nss_database_get is returning TRUE but leaves
DATABASE_NAME_SYMBOL set to NULL, so *ni is NULL (we could use an assert
there).  Then in nss_lookup_function we eventually do this:

  if (ni->module == NULL)

Since we're supposed to have defaults for EVERYTHING in nss, a NULL
action list entry means we're still starting up, I think.  The problem
case we're trying to solve happens AFTER we're started up.

How about this?  Check for local->data.services[database_index] being
non-NULL *first*, and if it's NULL, skip the chroot test?  At least then
we can't return NULL, and it shouldn't be NULL in the case we're trying
to prevent anyway.