| Message ID | 20240521131311.604831-1-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] posix: Fix pidfd_spawn/pidfd_spawnp leak if execve fails (BZ 31695) | expand |
* 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?
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); + } } else ec = errno;