From 718dbdb2ca4458cf91711cd9a7de3a972e46658e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 6 May 2025 11:14:10 +0200 Subject: [PATCH 1/3] man: reword the description of "secure pager" handling The existing description was not *wrong*, but it was a bit muddled. Let's reorder the text to give a short intro and then describe what the options actually do and the clear "true" and "false" cases first, and then describe autodetection. Related to https://yeswehack.com/vulnerability-center/reports/346802. --- man/common-variables.xml | 58 ++++++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/man/common-variables.xml b/man/common-variables.xml index 2d26bf7242..9f322dbf23 100644 --- a/man/common-variables.xml +++ b/man/common-variables.xml @@ -167,28 +167,46 @@ $SYSTEMD_PAGERSECURE - Takes a boolean argument. When true, the "secure" mode of the pager is enabled; if - false, disabled. If $SYSTEMD_PAGERSECURE is not set at all, secure mode is enabled - if the effective UID is not the same as the owner of the login session, see - geteuid2 - and sd_pid_get_owner_uid3. - In secure mode, will be set when invoking the pager, and the pager shall - disable commands that open or create new files or start new subprocesses. When - $SYSTEMD_PAGERSECURE is not set at all, pagers which are not known to implement - secure mode will not be used. (Currently only - less1 - implements secure mode.) - - Note: when commands are invoked with elevated privileges, for example under + Common pager commands like less1, in + addition to "paging", i.e. scrolling through the output, support opening of or writing to other files + and running arbitrary shell commands. When commands are invoked with elevated privileges, for example + under sudo8 or pkexec1, care - must be taken to ensure that unintended interactive features are not enabled. "Secure" mode for the - pager may be enabled automatically as describe above. Setting SYSTEMD_PAGERSECURE=0 - or not removing it from the inherited environment allows the user to invoke arbitrary commands. Note - that if the $SYSTEMD_PAGER or $PAGER variables are to be - honoured, $SYSTEMD_PAGERSECURE must be set too. It might be reasonable to completely - disable the pager using instead. + project='die-net'>pkexec1, the + pager becomes a security boundary. Care must be taken that only programs with strictly limited + functionality are used as pagers, and unintended interactive features like opening or creation of new + files or starting of subprocesses are not allowed. "Secure mode" for the pager may be enabled as + described below, if the pager supports that (most pagers are not written in a way + that takes this into consideration). It is recommended to either explicitly enable "secure mode" or to + completely disable the pager using or PAGER=cat when + allowing untrusted users to execute commands with elevated privileges. + + This option takes a boolean argument. When set to true, the "secure mode" of the pager is + enabled. In "secure mode", will be set when invoking the pager, which + instructs the pager to disable commands that open or create new files or start new subprocesses. + Currently only less1 is known + to understand this variable and implement "secure mode". + + When set to false, no limitation is placed on the pager. Setting + SYSTEMD_PAGERSECURE=0 or not removing it from the inherited environment may allow + the user to invoke arbitrary commands. + + When $SYSTEMD_PAGERSECURE is not set, systemd tools attempt to automatically + figure out if "secure mode" should be enabled and whether the pager supports it. "Secure mode" is + enabled if the effective UID is not the same as the owner of the login session, see + geteuid2 + and + sd_pid_get_owner_uid3. + In this case, SYSTEMD_PAGERSECURE=1 will be set and pagers which are not known to + implement "secure mode" will not be used at all. + + Note that if the $SYSTEMD_PAGER or $PAGER variables are to + be honoured, $SYSTEMD_PAGERSECURE must be set too. + From b6b78170e17727ca0ec320d5703b36d0597d0cc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 6 May 2025 11:37:26 +0200 Subject: [PATCH 2/3] man: rework the description of $SYSTEMD_PAGER and $PAGER $PAGER wasn't documented, but actually we treat it same as $SYSTEMD_PAGER, except for lower priority. And the two variables can be used to disable the pager, even if $SYSTEMD_PAGERSECURE is not set. Behaviour is (obviously) not changed by this patch, it intentionally just updates the docs to match the code. --- man/common-variables.xml | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/man/common-variables.xml b/man/common-variables.xml index 9f322dbf23..6a6e98d334 100644 --- a/man/common-variables.xml +++ b/man/common-variables.xml @@ -96,17 +96,22 @@ $SYSTEMD_PAGER + $PAGER - Pager to use when is not given; overrides - $PAGER. If neither $SYSTEMD_PAGER nor $PAGER are set, a - set of well-known pager implementations are tried in turn, including - less1 and - more1, until one is found. If - no pager implementation is discovered no pager is invoked. Setting this environment variable to an empty string - or the value cat is equivalent to passing . + Pager to use when is not given. + $SYSTEMD_PAGER is used if set; otherwise $PAGER is used. + If neither $SYSTEMD_PAGER nor $PAGER are set, a set of well-known + pager implementations is tried in turn, including + less1 + and + more1, + until one is found. If no pager implementation is discovered, no pager is invoked. Setting those + environment variables to an empty string or the value cat is equivalent to passing + . Note: if $SYSTEMD_PAGERSECURE is not set, $SYSTEMD_PAGER - (as well as $PAGER) will be silently ignored. + and $PAGER can only be used to disable the pager (with cat or + ), and are otherwise ignored. @@ -205,8 +210,8 @@ implement "secure mode" will not be used at all. Note that if the $SYSTEMD_PAGER or $PAGER variables are to - be honoured, $SYSTEMD_PAGERSECURE must be set too. - + be honoured, other than to disable the pager, $SYSTEMD_PAGERSECURE must be set + too. From cd93478af8b9dc69478d5667f113b67d175090fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 6 May 2025 14:29:02 +0200 Subject: [PATCH 3/3] pager: also check for $SUDO_UID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This returns to the original approach proposed in https://github.com/systemd/systemd/pull/17270. After review, the approach was changed to use sd_pid_get_owner_uid() instead. Back then, when running in a typical graphical session, sd_pid_get_owner_uid() would usually return the user UID, and when running under sudo, geteuid() would return 0, so we'd trigger the secure path. sudo may allocate a new session if is invoked outside of a session (depending on the PAM config). Since nowadays desktop environments usually start the user shell through user units, the typical shell in a terminal emulator is not part of a session, and when sudo is invoked, a new session is allocated, and sd_pid_get_owner_uid() returns 0 too. Technically, the code still works as documented in the man page, but in the common case, it doesn't do the expected thing. $ build/test-sd-login |& rg 'get_(owner_uid|cgroup|session)' sd_pid_get_session(0) → No data available sd_pid_get_owner_uid(0) → 1000 sd_pid_get_cgroup(0) → /user.slice/user-1000.slice/user@1000.service/app.slice/app-ghostty-transient-5088.scope/surfaces/556FAF50BA40.scope $ sudo build/test-sd-login |& rg 'get_(owner_uid|cgroup|session)' sd_pid_get_session(0) → c289 sd_pid_get_owner_uid(0) → 0 sd_pid_get_cgroup(0) → /user.slice/user-0.slice/session-c289.scope I think it's worth checking for sudo because it is a common case used by users. There obviously are other mechanims, so the man page is extended to say that only some common mechanisms are supported, and to (again) recommend setting SYSTEMD_LESSSECURE explicitly. The other option would be to set "secure mode" by default. But this would create an inconvenience for users doing the right thing, running systemctl and other tools directly, because then they can't run privileged commands from the pager, e.g. to save the output to a file. (Or the user would need to explicitly set SYSTEMD_LESSSECURE. One option would be to set it always in the environment and to rely on sudo and other tools stripping it from the environment before running privileged code. But that is also fairly fragile and it obviously relies on the user doing a complicated setup to support a fairly common use case. I think this decreases usability of the system quite a bit. I don't think we should build solutions that work in priniciple, but are painfully inconvenient in common cases.) Fixes https://yeswehack.com/vulnerability-center/reports/346802. Also see https://github.com/polkit-org/polkit/pull/562, which adds support for $SUDO_UID/$SUDO_GID to pkexec. --- man/common-variables.xml | 13 ++++++++++--- src/shared/pager.c | 29 +++++++++++++++++++---------- 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/man/common-variables.xml b/man/common-variables.xml index 6a6e98d334..991946f60d 100644 --- a/man/common-variables.xml +++ b/man/common-variables.xml @@ -205,9 +205,16 @@ enabled if the effective UID is not the same as the owner of the login session, see geteuid2 and - sd_pid_get_owner_uid3. - In this case, SYSTEMD_PAGERSECURE=1 will be set and pagers which are not known to - implement "secure mode" will not be used at all. + sd_pid_get_owner_uid3, + or when running under + sudo8 or similar + tools ($SUDO_UID is set + It is recommended for other tools to set and check $SUDO_UID as appropriate, + treating it is a common interface.). In those cases, + SYSTEMD_PAGERSECURE=1 will be set and pagers which are not known to implement + "secure mode" will not be used at all. Note that this autodetection only covers the most common + mechanisms to elevate privileges and is intended as convenience. It is recommended to explicitly set + $SYSTEMD_PAGERSECURE or disable the pager. Note that if the $SYSTEMD_PAGER or $PAGER variables are to be honoured, other than to disable the pager, $SYSTEMD_PAGERSECURE must be set diff --git a/src/shared/pager.c b/src/shared/pager.c index 9b8ae76700..f1043ec132 100644 --- a/src/shared/pager.c +++ b/src/shared/pager.c @@ -82,6 +82,22 @@ static int no_quit_on_interrupt(int exe_name_fd, const char *less_opts) { return r; } +static bool running_with_escalated_privileges(void) { + int r; + + if (getenv("SUDO_UID")) + return true; + + uid_t uid; + r = sd_pid_get_owner_uid(0, &uid); + if (r < 0) { + log_debug_errno(r, "sd_pid_get_owner_uid() failed, enabling pager secure mode: %m"); + return true; + } + + return uid != geteuid(); +} + void pager_open(PagerFlags flags) { _cleanup_close_pair_ int fd[2] = EBADF_PAIR, exe_name_pipe[2] = EBADF_PAIR; _cleanup_strv_free_ char **pager_args = NULL; @@ -177,16 +193,9 @@ void pager_open(PagerFlags flags) { * know to be good. */ int use_secure_mode = secure_getenv_bool("SYSTEMD_PAGERSECURE"); bool trust_pager = use_secure_mode >= 0; - if (use_secure_mode == -ENXIO) { - uid_t uid; - - r = sd_pid_get_owner_uid(0, &uid); - if (r < 0) - log_debug_errno(r, "sd_pid_get_owner_uid() failed, enabling pager secure mode: %m"); - - use_secure_mode = r < 0 || uid != geteuid(); - - } else if (use_secure_mode < 0) { + if (use_secure_mode == -ENXIO) + use_secure_mode = running_with_escalated_privileges(); + else if (use_secure_mode < 0) { log_warning_errno(use_secure_mode, "Unable to parse $SYSTEMD_PAGERSECURE, assuming true: %m"); use_secure_mode = true; }