From patchwork Thu May 22 11:04:51 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 112823 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 967F2385781B for ; Thu, 22 May 2025 11:09:24 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 967F2385781B Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=S5meoUYm X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 0D07E385770D for ; Thu, 22 May 2025 11:05:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0D07E385770D Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 0D07E385770D Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1747911904; cv=none; b=h9/C2Ti2GY3QG++vX3ZOshuHO9fDp6GirsiziTKA8uktGIfTkK85fGevyW7PaPX3Hc4gpT05ikQjBwMmBiQPkSRKEuO/7kbZRf0GHoTB5I33QUzjO3AuflMkFVal/qDawXvndf9m4KK7nr8I3rAZ5EWR4VqNIaf4JSQ5DXk8Xok= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1747911904; c=relaxed/simple; bh=wPyULfN4RY0nJxc8mM+roGsS7ahLB73MbHCwDKY/f0Q=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=CyAmGq2Ct/l5b6oGhKI9RiRdNaGKwrEy+EX15J5ZISz3b/wMfsnh2P1Y/6zlMWpw73zDL82E60wprn82Gp2s1A4qoQ70Tgl/3HMwT2TLov4GnVd1e1VQ9s7b6vPYU9t/5LTQZQdP6VaVnBqIh0Tt+JObSL0ODEYmGwBtocM1fbg= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0D07E385770D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1747911901; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type; bh=n0dHWhV8D9fHfezvL7302v2rNzEZ13ZEuASdW1NE0VM=; b=S5meoUYmW8sTjmltF80PAJ4/exo1qvi3H8rZFUFrclCR/VjoGBrZueAHEXkT8lZRbuqrb8 FAk8U6QeRTz5p7qVUMdMiy2/wyNby+IQRX+I0E0sH3hhTE4FnnhMit0JeP+SPaRAn7++er Jxor26TcHgtKGuyV0gqPIhXtUfeMkPM= Received: from mx-prod-mc-06.mail-002.prod.us-west-2.aws.redhat.com (ec2-35-165-154-97.us-west-2.compute.amazonaws.com [35.165.154.97]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-360-_9XqIBQxPSKIhUj7YJ4oqQ-1; Thu, 22 May 2025 07:04:58 -0400 X-MC-Unique: _9XqIBQxPSKIhUj7YJ4oqQ-1 X-Mimecast-MFC-AGG-ID: _9XqIBQxPSKIhUj7YJ4oqQ_1747911897 Received: from mx-prod-int-03.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-03.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.12]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-06.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id D35491800256; Thu, 22 May 2025 11:04:56 +0000 (UTC) Received: from oldenburg.str.redhat.com (unknown [10.45.224.17]) by mx-prod-int-03.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id DBE6B1944DFF; Thu, 22 May 2025 11:04:54 +0000 (UTC) From: Florian Weimer To: libc-alpha@sourceware.org Cc: Carlos O'Donell , Sam James , Siddhesh Poyarekar Subject: [PATCH] Fix error reporting (false negatives) in SGID tests Date: Thu, 22 May 2025 13:04:51 +0200 Message-ID: <87jz6829f0.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.0 on 10.30.177.12 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: qWr8qdsX1Sfss4-y0uduhipTWBpI9u8XnulwyUUeHBE_1747911897 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, RCVD_IN_VALIDITY_RPBL_BLOCKED, RCVD_IN_VALIDITY_SAFE_BLOCKED, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libc-alpha-bounces~patchwork=sourceware.org@sourceware.org And simplify the interface of support_capture_subprogram_self_sgid. Use the existing framework for temporary directories (now with mode 0700) and directory/file deletion. Handle all execution errors within support_capture_subprogram_self_sgid. In particular, this includes test failures because the invoked program did not exit with exit status zero. Existing tests that expect exit status 42 are adjuste to use zero instead. In addition, fix callers not to call exit (0) with test failures pending (which may mask them, especially when running with --direct). Fixes commit 35fc356fa3b4f485bd3ba3114c9f774e5df7d3c2 ("elf: Fix subprocess status handling for tst-dlopen-sgid (bug 32987)"). I tested this as regular users (with and without supplementary groups) and as root. --- elf/tst-dlopen-sgid.c | 8 +-- elf/tst-env-setuid-tunables.c | 18 +----- elf/tst-env-setuid.c | 17 +----- stdlib/tst-secure-getenv.c | 9 +-- support/capture_subprocess.h | 10 ++-- support/support_capture_subprocess.c | 106 ++++++++--------------------------- 6 files changed, 36 insertions(+), 132 deletions(-) base-commit: 4052d99ead880797cf271309fd87ddd2b95bd353 diff --git a/elf/tst-dlopen-sgid.c b/elf/tst-dlopen-sgid.c index 5688b79f2e..8aec52e19f 100644 --- a/elf/tst-dlopen-sgid.c +++ b/elf/tst-dlopen-sgid.c @@ -70,13 +70,7 @@ do_test (void) free (libdir); - int status = support_capture_subprogram_self_sgid (magic_argument); - - if (WEXITSTATUS (status) == EXIT_UNSUPPORTED) - return EXIT_UNSUPPORTED; - - if (!WIFEXITED (status)) - FAIL_EXIT1 ("Unexpected exit status %d from child process\n", status); + support_capture_subprogram_self_sgid (magic_argument); return 0; } diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c index a4233b1727..bfdb30cbd8 100644 --- a/elf/tst-env-setuid-tunables.c +++ b/elf/tst-env-setuid-tunables.c @@ -105,10 +105,7 @@ do_test (int argc, char **argv) if (ret != 0) exit (1); - - /* Special return code to make sure that the child executed all the way - through. */ - exit (42); + return 0; } else { @@ -127,18 +124,7 @@ do_test (int argc, char **argv) continue; } - int status = support_capture_subprogram_self_sgid (buf); - - /* Bail out early if unsupported. */ - if (WEXITSTATUS (status) == EXIT_UNSUPPORTED) - return EXIT_UNSUPPORTED; - - if (WEXITSTATUS (status) != 42) - { - printf (" [%d] child failed with status %d\n", i, - WEXITSTATUS (status)); - support_record_failure (); - } + support_capture_subprogram_self_sgid (buf); } return 0; } diff --git a/elf/tst-env-setuid.c b/elf/tst-env-setuid.c index 2c632ed30c..7209acd616 100644 --- a/elf/tst-env-setuid.c +++ b/elf/tst-env-setuid.c @@ -147,10 +147,7 @@ do_test (int argc, char **argv) if (ret != 0) exit (1); - - /* Special return code to make sure that the child executed all the way - through. */ - exit (42); + return 0; } else { @@ -174,17 +171,7 @@ do_test (int argc, char **argv) free (profilepath); } - int status = support_capture_subprogram_self_sgid (SETGID_CHILD); - - if (WEXITSTATUS (status) == EXIT_UNSUPPORTED) - exit (EXIT_UNSUPPORTED); - - if (WEXITSTATUS (status) != 42) - { - printf (" child failed with status %d\n", - WEXITSTATUS (status)); - support_record_failure (); - } + support_capture_subprogram_self_sgid (SETGID_CHILD); return 0; } diff --git a/stdlib/tst-secure-getenv.c b/stdlib/tst-secure-getenv.c index 3fd1d232be..c12c63aee1 100644 --- a/stdlib/tst-secure-getenv.c +++ b/stdlib/tst-secure-getenv.c @@ -57,13 +57,7 @@ do_test (void) exit (1); } - int status = support_capture_subprogram_self_sgid (MAGIC_ARGUMENT); - - if (WEXITSTATUS (status) == EXIT_UNSUPPORTED) - return EXIT_UNSUPPORTED; - - if (!WIFEXITED (status)) - FAIL_EXIT1 ("Unexpected exit status %d from child process\n", status); + support_capture_subprogram_self_sgid (MAGIC_ARGUMENT); return 0; } @@ -82,6 +76,7 @@ alternative_main (int argc, char **argv) if (secure_getenv ("PATH") != NULL) FAIL_EXIT (4, "PATH variable not filtered out\n"); + support_record_failure_barrier (); exit (EXIT_SUCCESS); } } diff --git a/support/capture_subprocess.h b/support/capture_subprocess.h index 77140430d2..b37462d0d1 100644 --- a/support/capture_subprocess.h +++ b/support/capture_subprocess.h @@ -42,10 +42,12 @@ struct support_capture_subprocess support_capture_subprocess struct support_capture_subprocess support_capture_subprogram (const char *file, char *const argv[], char *const envp[]); -/* Copy the running program into a setgid binary and run it with CHILD_ID - argument. If execution is successful, return the exit status of the child - program, otherwise return a non-zero failure exit code. */ -int support_capture_subprogram_self_sgid (const char *child_id); +/* Copy the running program into a setgid binary and run it with + CHILD_ID argument. If the program exits with a non-zero status, + exit with that exit status (or status 1 if the program did not exit + normally). If the test cannot be performed, exit with + EXIT_UNSUPPORTED. */ +void support_capture_subprogram_self_sgid (const char *child_id); /* Deallocate the subprocess data captured by support_capture_subprocess. */ diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c index 9d88d6291a..b4e4bf9502 100644 --- a/support/support_capture_subprocess.c +++ b/support/support_capture_subprocess.c @@ -31,6 +31,7 @@ #include #include #include +#include #include static void @@ -113,105 +114,44 @@ support_capture_subprogram (const char *file, char *const argv[], /* Copies the executable into a restricted directory, so that we can safely make it SGID with the TARGET group ID. Then runs the executable. */ -static int +static void copy_and_spawn_sgid (const char *child_id, gid_t gid) { - char *dirname = xasprintf ("%s/tst-tunables-setuid.%jd", - test_dir, (intmax_t) getpid ()); + char *dirname = support_create_temp_directory ("tst-glibc-sgid-"); char *execname = xasprintf ("%s/bin", dirname); - int infd = -1; - int outfd = -1; - int ret = 1, status = 1; + add_temp_file (execname); - TEST_VERIFY (mkdir (dirname, 0700) == 0); - if (support_record_failure_is_failed ()) - goto err; - - infd = open ("/proc/self/exe", O_RDONLY); - if (infd < 0) + if (access ("/proc/self/exe", R_OK) != 0) FAIL_UNSUPPORTED ("unsupported: Cannot read binary from procfs\n"); - outfd = open (execname, O_WRONLY | O_CREAT | O_EXCL, 0700); - TEST_VERIFY (outfd >= 0); - if (support_record_failure_is_failed ()) - goto err; + support_copy_file ("/proc/self/exe", execname); - char buf[4096]; - for (;;) - { - ssize_t rdcount = read (infd, buf, sizeof (buf)); - TEST_VERIFY (rdcount >= 0); - if (support_record_failure_is_failed ()) - goto err; - if (rdcount == 0) - break; - char *p = buf; - char *end = buf + rdcount; - while (p != end) - { - ssize_t wrcount = write (outfd, buf, end - p); - if (wrcount == 0) - errno = ENOSPC; - TEST_VERIFY (wrcount > 0); - if (support_record_failure_is_failed ()) - goto err; - p += wrcount; - } - } + if (chown (execname, getuid (), gid) != 0) + FAIL_UNSUPPORTED ("cannot change group of \"%s\" to %jd: %m", + execname, (intmax_t) gid); - bool chowned = false; - TEST_VERIFY ((chowned = fchown (outfd, getuid (), gid) == 0) - || errno == EPERM); - if (support_record_failure_is_failed ()) - goto err; - else if (!chowned) - { - ret = 77; - goto err; - } - - TEST_VERIFY (fchmod (outfd, 02750) == 0); - if (support_record_failure_is_failed ()) - goto err; - TEST_VERIFY (close (outfd) == 0); - if (support_record_failure_is_failed ()) - goto err; - TEST_VERIFY (close (infd) == 0); - if (support_record_failure_is_failed ()) - goto err; + if (chmod (execname, 02750) != 0) + FAIL_UNSUPPORTED ("cannot make \"%s\" SGID: %m ", execname); /* We have the binary, now spawn the subprocess. Avoid using support_subprogram because we only want the program exit status, not the contents. */ - ret = 0; - infd = outfd = -1; char * const args[] = {execname, (char *) child_id, NULL}; + int status = support_subprogram_wait (args[0], args); - status = support_subprogram_wait (args[0], args); + free (execname); + free (dirname); -err: - if (outfd >= 0) - close (outfd); - if (infd >= 0) - close (infd); - if (execname != NULL) + if (WIFEXITED (status)) { - unlink (execname); - free (execname); + if (WEXITSTATUS (status) == 0) + return; + else + exit (WEXITSTATUS (status)); } - if (dirname != NULL) - { - rmdir (dirname); - free (dirname); - } - - if (ret == 77) - FAIL_UNSUPPORTED ("Failed to make sgid executable for test\n"); - if (ret != 0) - FAIL_EXIT1 ("Failed to make sgid executable for test\n"); - - return status; + else + FAIL_EXIT1 ("subprogram failed with status %d", status); } /* Returns true if a group with NAME has been found, and writes its @@ -253,7 +193,7 @@ find_sgid_group (gid_t *target, const char *name) return ok; } -int +void support_capture_subprogram_self_sgid (const char *child_id) { const int count = 64; @@ -288,7 +228,7 @@ support_capture_subprogram_self_sgid (const char *child_id) (intmax_t) getuid ()); } - return copy_and_spawn_sgid (child_id, target); + copy_and_spawn_sgid (child_id, target); } void