diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 4f6240e5465..934428e7f52 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -948,6 +948,8 @@ int replace_env_argv( } k += q; + /* reallocarray() does not zero-initialize the grown tail, add NUL termination */ + n[k] = NULL; continue; } diff --git a/src/shared/libfido2-util.c b/src/shared/libfido2-util.c index 4a0d7e6d647..07bea6658ec 100644 --- a/src/shared/libfido2-util.c +++ b/src/shared/libfido2-util.c @@ -622,6 +622,8 @@ static int fido2_use_hmac_hash_specific_token( return log_error_errno(SYNTHETIC_ERRNO(EIO), "Failed to retrieve HMAC secret."); hmac_size = sym_fido_assert_hmac_secret_len(a, 0); + /* Should never be zero, sanity check */ + assert(hmac_size > 0); hmac_copy = memdup(hmac, hmac_size); if (!hmac_copy) @@ -1160,6 +1162,8 @@ int fido2_generate_hmac_hash( return log_error_errno(SYNTHETIC_ERRNO(EIO), "Failed to retrieve HMAC secret."); secret_size = sym_fido_assert_hmac_secret_len(a, 0); + /* Should never be zero, sanity check */ + assert(secret_size > 0); secret_copy = memdup(secret, secret_size); if (!secret_copy) diff --git a/src/shared/ptyfwd.c b/src/shared/ptyfwd.c index fcc1500dc5c..5445da2a1b6 100644 --- a/src/shared/ptyfwd.c +++ b/src/shared/ptyfwd.c @@ -889,19 +889,16 @@ static int on_sigwinch_event(sd_event_source *e, const struct signalfd_siginfo * static int on_exit_event(sd_event_source *e, void *userdata) { PTYForward *f = ASSERT_PTR(userdata); - int r; assert(e); assert(e == f->exit_event_source); - if (!pty_forward_drain(f)) { - /* If not drained, try to drain the buffer. */ - r = shovel_force(f); - if (r < 0) - return r; - } - - return pty_forward_done(f, 0); + /* The event loop is exiting while the forwarder is still active. Force a final synchronous drain of + * whatever is still buffered. shovel() may complete the drain and call pty_forward_done(), which + * can free f via the hangup handler, so we must not touch f afterwards, only propagate the return + * value, exactly like on_master_event() does. */ + f->drain = true; + return shovel_force(f); } static int on_defer_event(sd_event_source *s, void *userdata) { diff --git a/src/test/test-env-util.c b/src/test/test-env-util.c index 281ea7951cb..7c55bcc3d86 100644 --- a/src/test/test-env-util.c +++ b/src/test/test-env-util.c @@ -590,4 +590,28 @@ TEST(strv_env_get_merged) { ASSERT_TRUE(strv_equal(m, expected)); } +TEST(replace_env_argv_unterminated) { + /* A bare unset "$VAR" token expands to nothing and advances neither the write index nor writes a + * terminator, so the output strv must still be NUL-terminated when such a token leads or is the + * only word. */ + + _cleanup_strv_free_ char **a = NULL, **b = NULL, **c = NULL, **d = NULL; + + /* Single bare unset variable: result must be a properly terminated empty strv. */ + ASSERT_OK(replace_env_argv(STRV_MAKE("$THIS_IS_UNSET"), STRV_MAKE("FOO=BAR"), &a, NULL, NULL)); + ASSERT_TRUE(strv_isempty(a)); + + /* Only unset variables. */ + ASSERT_OK(replace_env_argv(STRV_MAKE("$THIS_IS_UNSET", "$ALSO_UNSET"), STRV_MAKE("FOO=BAR"), &b, NULL, NULL)); + ASSERT_TRUE(strv_isempty(b)); + + /* Trailing bare unset variable after an expanded one. */ + ASSERT_OK(replace_env_argv(STRV_MAKE("$FOO", "$THIS_IS_UNSET"), STRV_MAKE("FOO=BAR"), &c, NULL, NULL)); + ASSERT_TRUE(strv_equal(c, STRV_MAKE("BAR"))); + + /* Trailing bare unset variable after a literal word. */ + ASSERT_OK(replace_env_argv(STRV_MAKE("hello", "$THIS_IS_UNSET"), STRV_MAKE("FOO=BAR"), &d, NULL, NULL)); + ASSERT_TRUE(strv_equal(d, STRV_MAKE("hello"))); +} + DEFINE_TEST_MAIN(LOG_DEBUG); diff --git a/test/units/TEST-74-AUX-UTILS.run.sh b/test/units/TEST-74-AUX-UTILS.run.sh index 2f120638b50..bdb0fd1a7eb 100755 --- a/test/units/TEST-74-AUX-UTILS.run.sh +++ b/test/units/TEST-74-AUX-UTILS.run.sh @@ -314,3 +314,19 @@ fi # Tests whether intermediate disconnects corrupt us (modified testcase from https://github.com/systemd/systemd/issues/27204) assert_rc "37" timeout 300 systemd-run --unit=disconnecttest --wait --pipe --user -M testuser@.host bash -ec 'systemctl --user daemon-reexec; sleep 3; exit 37' + +# Trigger on_exit_event(): SIGINT makes systemd-run leave the event loop +# while PTY forwarding is active. coproc gives us a pollable stdout and exec +# makes $PTY_FWD_PID point at systemd-run +coproc PTY_FWD { exec systemd-run --quiet --pty -- bash -c 'echo PTY_FORWARD_READY; exec sleep 60'; } +PTY_FWD_PID_SAVED="$PTY_FWD_PID" +read -r -t 30 -u "${PTY_FWD[0]}" PTY_FWD_LINE || true +[[ "$PTY_FWD_LINE" == *PTY_FORWARD_READY* ]] +kill -INT "$PTY_FWD_PID_SAVED" +PTY_FWD_RC=0 +wait "$PTY_FWD_PID_SAVED" || PTY_FWD_RC=$? +# ASan reports the UAF as systemd-run exiting non-zero. +if [[ "$PTY_FWD_RC" != 0 ]]; then + echo "systemd-run --pty failed" + exit 1 +fi