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

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

[Sam James]

First contribution so let me know if need to change anything, thanks!

(Did want to add a test case but found it too hard to find a way to
construct a situation where the call would fail w/o seccomp, and
adding a libseccomp dependency isn't desirable.)

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

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

-- 
2.35.1

[PATCH 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 | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/nss/nss_database.c b/nss/nss_database.c
index d56c5b798d..44c6ad091a 100644
--- a/nss/nss_database.c
+++ b/nss/nss_database.c
@@ -424,11 +424,13 @@ 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];
-- 
2.35.1

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

[Carlos O'Donell]

On 3/14/22 12:54, Sam James via Libc-alpha wrote:
> 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.

Fails CI/CD for i686:
https://patchwork.sourceware.org/project/glibc/patch/20220314165414.3110670-2-sam@gentoo.org/

Please review. If you need help please reach out.
 
> 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 | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/nss/nss_database.c b/nss/nss_database.c
> index d56c5b798d..44c6ad091a 100644
> --- a/nss/nss_database.c
> +++ b/nss/nss_database.c
> @@ -424,11 +424,13 @@ 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];


-- 
Cheers,
Carlos.

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

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]


> On 15 Mar 2022, at 21:48, Carlos O'Donell via Libc-alpha <libc-alpha@sourceware.org> wrote:
> 
> On 3/14/22 12:54, Sam James via Libc-alpha wrote:
>> 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.
> 
> Fails CI/CD for i686:
> https://patchwork.sourceware.org/project/glibc/patch/20220314165414.3110670-2-sam@gentoo.org/
> 
> Please review. If you need help please reach out.
> 

Hi Carlos,

Thanks a lot!

I think v2 should be okay.

Best,
sam

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 618 bytes --]