1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
|
From d8510d35c857547ddb601a988545ccb9a1e43fcc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Wed, 7 Oct 2020 11:15:05 +0200
Subject: pager: make pager secure when under euid is changed or explicitly
requested
The variable is renamed to SYSTEMD_PAGERSECURE (because it's not just about
less now), and we automatically enable secure mode in certain cases, but not
otherwise.
This approach is more nuanced, but should provide a better experience for
users:
- Previusly we would set LESSSECURE=1 and trust the pager to make use of
it. But this has an effect only on less. We need to not start pagers which
are insecure when in secure mode. In particular more is like that and is a
very popular pager.
- We don't enable secure mode always, which means that those other pagers can
reasonably used.
- We do the right thing by default, but the user has ultimate control by
setting SYSTEMD_PAGERSECURE.
Fixes #5666.
v2:
- also check $PKEXEC_UID
v3:
- use 'sd_pid_get_owner_uid() != geteuid()' as the condition
---
man/less-variables.xml | 30 ++++++++++++++++----
src/shared/pager.c | 63 ++++++++++++++++++++++++++++--------------
2 files changed, 66 insertions(+), 27 deletions(-)
diff --git a/man/less-variables.xml b/man/less-variables.xml
index fed4178b01..b892854509 100644
--- a/man/less-variables.xml
+++ b/man/less-variables.xml
@@ -61,12 +61,30 @@
</varlistentry>
<varlistentry id='lesssecure'>
- <term><varname>$SYSTEMD_LESSSECURE</varname></term>
-
- <listitem><para>Takes a boolean argument. Overrides the <varname>$LESSSECURE</varname> environment
- variable when invoking the pager, which controls the "secure" mode of less (which disables commands
- such as <literal>|</literal> which allow to easily shell out to external command lines). By default
- less secure mode is enabled, with this setting it may be disabled.</para></listitem>
+ <term><varname>$SYSTEMD_PAGERSECURE</varname></term>
+
+ <listitem><para>Takes a boolean argument. When true, the "secure" mode of the pager is enabled; if
+ false, disabled. If <varname>$SYSTEMD_PAGERSECURE</varname> 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 <citerefentry
+ project='man-pages'><refentrytitle>geteuid</refentrytitle><manvolnum>2</manvolnum></citerefentry> and
+ <citerefentry><refentrytitle>sd_pid_get_owner_uid</refentrytitle><manvolnum>3</manvolnum></citerefentry>.
+ In secure mode, <option>LESSSECURE=1</option> will be set when invoking the pager, and the pager shall
+ disable commands that open or create new files or start new subprocesses. When
+ <varname>$SYSTEMD_PAGERSECURE</varname> is not set at all, pagers which are not known to implement
+ secure mode will not be used. (Currently only
+ <citerefentry><refentrytitle>less</refentrytitle><manvolnum>1</manvolnum></citerefentry> implements
+ secure mode.)</para>
+
+ <para>Note: when commands are invoked with elevated privileges, for example under <citerefentry
+ project='man-pages'><refentrytitle>sudo</refentrytitle><manvolnum>8</manvolnum></citerefentry> or
+ <citerefentry
+ project='die-net'><refentrytitle>pkexec</refentrytitle><manvolnum>1</manvolnum></citerefentry>, 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 <varname>SYSTEMD_PAGERSECURE=0</varname>
+ or not removing it from the inherited environment allows the user to invoke arbitrary commands. Note
+ that if the <varname>$SYSTEMD_PAGER</varname> or <varname>$PAGER</varname> variables are to be
+ honoured, <varname>$SYSTEMD_PAGERSECURE</varname> must be set too. It might be reasonable to completly
+ disable the pager using <option>--no-pager</option> instead.</para></listitem>
</varlistentry>
</variablelist>
diff --git a/src/shared/pager.c b/src/shared/pager.c
index 6f1d57a096..0f53cad429 100644
--- a/src/shared/pager.c
+++ b/src/shared/pager.c
@@ -10,6 +10,8 @@
#include <sys/prctl.h>
#include <unistd.h>
+#include "sd-login.h"
+
#include "copy.h"
#include "env-util.h"
#include "fd-util.h"
@@ -165,25 +167,42 @@ int pager_open(PagerFlags flags) {
}
/* People might invoke us from sudo, don't needlessly allow less to be a way to shell out
- * privileged stuff. */
- r = getenv_bool("SYSTEMD_LESSSECURE");
- if (r == 0) { /* Remove env var if off */
- if (unsetenv("LESSSECURE") < 0) {
- log_error_errno(errno, "Failed to uset environment variable LESSSECURE: %m");
- _exit(EXIT_FAILURE);
- }
- } else {
- /* Set env var otherwise */
+ * privileged stuff. If the user set $SYSTEMD_PAGERSECURE, trust their configuration of the
+ * pager. If they didn't, use secure mode when under euid is changed. If $SYSTEMD_PAGERSECURE
+ * wasn't explicitly set, and we autodetect the need for secure mode, only use the pager we
+ * know to be good. */
+ int use_secure_mode = 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_warning_errno(r, "Unable to parse $SYSTEMD_LESSSECURE, ignoring: %m");
+ log_debug_errno(r, "sd_pid_get_owner_uid() failed, enabling pager secure mode: %m");
- if (setenv("LESSSECURE", "1", 1) < 0) {
- log_error_errno(errno, "Failed to set environment variable LESSSECURE: %m");
- _exit(EXIT_FAILURE);
- }
+ use_secure_mode = r < 0 || uid != geteuid();
+
+ } else if (use_secure_mode < 0) {
+ log_warning_errno(use_secure_mode, "Unable to parse $SYSTEMD_PAGERSECURE, assuming true: %m");
+ use_secure_mode = true;
}
- if (pager_args) {
+ /* We generally always set variables used by less, even if we end up using a different pager.
+ * They shouldn't hurt in any case, and ideally other pagers would look at them too. */
+ if (use_secure_mode)
+ r = setenv("LESSSECURE", "1", 1);
+ else
+ r = unsetenv("LESSSECURE");
+ if (r < 0) {
+ log_error_errno(errno, "Failed to adjust environment variable LESSSECURE: %m");
+ _exit(EXIT_FAILURE);
+ }
+
+ if (trust_pager && pager_args) { /* The pager config might be set globally, and we cannot
+ * know if the user adjusted it to be appropriate for the
+ * secure mode. Thus, start the pager specified through
+ * envvars only when $SYSTEMD_PAGERSECURE was explicitly set
+ * as well. */
r = loop_write(exe_name_pipe[1], pager_args[0], strlen(pager_args[0]) + 1, false);
if (r < 0) {
log_error_errno(r, "Failed to write pager name to socket: %m");
@@ -195,13 +214,14 @@ int pager_open(PagerFlags flags) {
"Failed to execute '%s', using fallback pagers: %m", pager_args[0]);
}
- /* Debian's alternatives command for pagers is
- * called 'pager'. Note that we do not call
- * sensible-pagers here, since that is just a
- * shell script that implements a logic that
- * is similar to this one anyway, but is
- * Debian-specific. */
+ /* Debian's alternatives command for pagers is called 'pager'. Note that we do not call
+ * sensible-pagers here, since that is just a shell script that implements a logic that is
+ * similar to this one anyway, but is Debian-specific. */
FOREACH_STRING(exe, "pager", "less", "more") {
+ /* Only less implements secure mode right now. */
+ if (use_secure_mode && !streq(exe, "less"))
+ continue;
+
r = loop_write(exe_name_pipe[1], exe, strlen(exe) + 1, false);
if (r < 0) {
log_error_errno(r, "Failed to write pager name to socket: %m");
@@ -212,6 +232,7 @@ int pager_open(PagerFlags flags) {
"Failed to execute '%s', using next fallback pager: %m", exe);
}
+ /* Our builtin is also very secure. */
r = loop_write(exe_name_pipe[1], "(built-in)", strlen("(built-in)") + 1, false);
if (r < 0) {
log_error_errno(r, "Failed to write pager name to socket: %m");
--
2.30.2
|
