[v3] posix: Fix pidfd_spawn/pidfd_spawnp leak if execve fails (BZ 31695)

  If the pidfd_spawn/pidfd_spawnp helper process succeeds, but evecve
fails for some reason (either with an invalid/non-existent, memory
allocation, etc.) the resulting pidfd is never closed, nor returned
to caller (so it can call close).

Since the process creation failed, it should be up to posix_spawn to
also, close the file descriptor in this case (similar to what it
does to reap the process).

This patch also changes the waitpid with waitid (P_PIDFD) for pidfd
case, to avoid a possible pid re-use.

Checked on x86_64-linux-gnu.

--
Changes from v2:
* Use waitid (P_PIDFD) for the pidfd case.
Changes from v1:
* Use __close_nocancel_nostatus instead of __close.
---
 posix/tst-spawn2.c               | 80 +++++++++++++++++++-------------
 sysdeps/unix/sysv/linux/spawni.c | 23 ++++++---
 2 files changed, 64 insertions(+), 39 deletions(-)
  

* Adhemerval Zanella:

> If the pidfd_spawn/pidfd_spawnp helper process succeeds, but evecve
> fails for some reason (either with an invalid/non-existent, memory
> allocation, etc.) the resulting pidfd is never closed, nor returned
> to caller (so it can call close).
>
> Since the process creation failed, it should be up to posix_spawn to
> also, close the file descriptor in this case (similar to what it
> does to reap the process).
>
> This patch also changes the waitpid with waitid (P_PIDFD) for pidfd
> case, to avoid a possible pid re-use.
>
> Checked on x86_64-linux-gnu.

Would you please note that the commit reference that is fixed by this?

Is this a denial of service vulnerability?

Thanks,
Florian
  

On 27/05/24 10:47, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> If the pidfd_spawn/pidfd_spawnp helper process succeeds, but evecve
>> fails for some reason (either with an invalid/non-existent, memory
>> allocation, etc.) the resulting pidfd is never closed, nor returned
>> to caller (so it can call close).
>>
>> Since the process creation failed, it should be up to posix_spawn to
>> also, close the file descriptor in this case (similar to what it
>> does to reap the process).
>>
>> This patch also changes the waitpid with waitid (P_PIDFD) for pidfd
>> case, to avoid a possible pid re-use.
>>
>> Checked on x86_64-linux-gnu.
> 
> Would you please note that the commit reference that is fixed by this?

It fixes the initial commit that added the symbol (0d6f9f626521678f330),
I can resend the patch with this reference.

> 
> Is this a denial of service vulnerability?

It could depending of how the service provide the spawn process primitive,
if the caller can define any string as the executable along with a retry
on failure even if the executable does not exist I think you can create a
DoS attack (not sure if such scenario is feasible though).

> 
> Thanks,
> Florian
>
  

* Adhemerval Zanella Netto:

>> Is this a denial of service vulnerability?
>
> It could depending of how the service provide the spawn process primitive,
> if the caller can define any string as the executable along with a retry
> on failure even if the executable does not exist I think you can create a
> DoS attack (not sure if such scenario is feasible though).

I think it can also be triggered if execve fails for other reasons, such
as exceeding the (still fairly low, I believe) command line size limit.

Thanks,
Florian
  

On 28/05/24 03:25, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>>> Is this a denial of service vulnerability?
>>
>> It could depending of how the service provide the spawn process primitive,
>> if the caller can define any string as the executable along with a retry
>> on failure even if the executable does not exist I think you can create a
>> DoS attack (not sure if such scenario is feasible though).
> 
> I think it can also be triggered if execve fails for other reasons, such
> as exceeding the (still fairly low, I believe) command line size limit.

And memory pressure, if the executable is not accessible or user does not
have permission, etc.  Are you ok with this version or should I update the
commt message?
  

On 5/21/24 9:12 AM, Adhemerval Zanella wrote:
> If the pidfd_spawn/pidfd_spawnp helper process succeeds, but evecve
> fails for some reason (either with an invalid/non-existent, memory
> allocation, etc.) the resulting pidfd is never closed, nor returned
> to caller (so it can call close).
> 
> Since the process creation failed, it should be up to posix_spawn to
> also, close the file descriptor in this case (similar to what it
> does to reap the process).

Agreed.
 
> This patch also changes the waitpid with waitid (P_PIDFD) for pidfd
> case, to avoid a possible pid re-use.

OK.

> Checked on x86_64-linux-gnu.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> --
> Changes from v2:
> * Use waitid (P_PIDFD) for the pidfd case.
> Changes from v1:
> * Use __close_nocancel_nostatus instead of __close.
> ---
>  posix/tst-spawn2.c               | 80 +++++++++++++++++++-------------
>  sysdeps/unix/sysv/linux/spawni.c | 23 ++++++---
>  2 files changed, 64 insertions(+), 39 deletions(-)
> 
> diff --git a/posix/tst-spawn2.c b/posix/tst-spawn2.c
> index bb507204a2..b2bad3f1f7 100644
> --- a/posix/tst-spawn2.c
> +++ b/posix/tst-spawn2.c
> @@ -26,6 +26,7 @@
>  #include <stdio.h>
>  
>  #include <support/check.h>
> +#include <support/descriptors.h>
>  #include <tst-spawn.h>
>  
>  int
> @@ -38,38 +39,53 @@ do_test (void)
>    char * const args[] = { 0 };
>    PID_T_TYPE pid = -1;
>  
> -  int ret = POSIX_SPAWN (&pid, program, 0, 0, args, environ);
> -  if (ret != ENOENT)
> -    {
> -      errno = ret;
> -      FAIL_EXIT1 ("posix_spawn: %m");
> -    }
> -
> -  /* POSIX states the value returned on pid variable in case of an error
> -     is not specified.  GLIBC will update the value iff the child
> -     execution is successful.  */
> -  if (pid != -1)
> -    FAIL_EXIT1 ("posix_spawn returned pid != -1 (%i)", (int) pid);
> -
> -  /* Check if no child is actually created.  */
> -  TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1);
> -  TEST_COMPARE (errno, ECHILD);
> -
> -  /* Same as before, but with posix_spawnp.  */
> -  char *args2[] = { (char*) program, 0 };
> -
> -  ret = POSIX_SPAWNP (&pid, args2[0], 0, 0, args2, environ);
> -  if (ret != ENOENT)
> -    {
> -      errno = ret;
> -      FAIL_EXIT1 ("posix_spawnp: %m");
> -    }
> -
> -  if (pid != -1)
> -    FAIL_EXIT1 ("posix_spawnp returned pid != -1 (%i)", (int) pid);
> -
> -  TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1);
> -  TEST_COMPARE (errno, ECHILD);
> +  {
> +    struct support_descriptors *descrs = support_descriptors_list ();
> +
> +    int ret = POSIX_SPAWN (&pid, program, 0, 0, args, environ);
> +    if (ret != ENOENT)
> +      {
> +	errno = ret;
> +	FAIL_EXIT1 ("posix_spawn: %m");
> +      }
> +
> +    /* POSIX states the value returned on pid variable in case of an error
> +       is not specified.  GLIBC will update the value iff the child
> +       execution is successful.  */
> +    if (pid != -1)
> +      FAIL_EXIT1 ("posix_spawn returned pid != -1 (%i)", (int) pid);
> +
> +    /* Check if no child is actually created.  */
> +    TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1);
> +    TEST_COMPARE (errno, ECHILD);
> +
> +    /* Also check if there is no leak descriptors.  */
> +    support_descriptors_check (descrs);
> +    support_descriptors_free (descrs);
> +  }
> +
> +  {
> +    /* Same as before, but with posix_spawnp.  */
> +    char *args2[] = { (char*) program, 0 };
> +
> +    struct support_descriptors *descrs = support_descriptors_list ();
> +
> +    int ret = POSIX_SPAWNP (&pid, args2[0], 0, 0, args2, environ);
> +    if (ret != ENOENT)
> +      {
> +	errno = ret;
> +	FAIL_EXIT1 ("posix_spawnp: %m");
> +      }
> +
> +    if (pid != -1)
> +      FAIL_EXIT1 ("posix_spawnp returned pid != -1 (%i)", (int) pid);
> +
> +    TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1);
> +    TEST_COMPARE (errno, ECHILD);
> +
> +    support_descriptors_check (descrs);
> +    support_descriptors_free (descrs);
> +  }
>  
>    return 0;
>  }
> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index e8ed2babb9..f57e92815e 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -449,13 +449,22 @@ __spawnix (int *pid, const char *file,
>  	 caller to actually collect it.  */
>        ec = args.err;
>        if (ec > 0)
> -	/* There still an unlikely case where the child is cancelled after
> -	   setting args.err, due to a positive error value.  Also there is
> -	   possible pid reuse race (where the kernel allocated the same pid
> -	   to an unrelated process).  Unfortunately due synchronization
> -	   issues where the kernel might not have the process collected
> -	   the waitpid below can not use WNOHANG.  */
> -	__waitpid (new_pid, NULL, 0);
> +	{
> +	  /* There still an unlikely case where the child is cancelled after
> +	     setting args.err, due to a positive error value.  Also there is
> +	     possible pid reuse race (where the kernel allocated the same pid
> +	     to an unrelated process).  Unfortunately due synchronization
> +	     issues where the kernel might not have the process collected
> +	     the waitpid below can not use WNOHANG.  */
> +	  __waitid (use_pidfd ? P_PIDFD : P_PID,
> +		    use_pidfd ? args.pidfd : new_pid,
> +		    NULL,
> +		    WEXITED);
> +	  /* For pidfd we need to also close the file descriptor for the case
> +	     where execve fails.  */
> +	  if (use_pidfd)
> +	    __close_nocancel_nostatus (args.pidfd);

OK. Close descriptor to stop the leak.

> +	}
>      }
>    else
>      ec = errno;