su(1) --whitelist-environment

su(1) --whitelist-environment

[Karel Zak]

 Hi all,

I'd like to add a new feature to su(1). Look forward to see your
objections and comments.

Example:

         # export FOO="this is foo"

         # su - kzak
         $ echo $FOO

         # su ---whitelist-environment FOO - kzak
         $ echo $FOO
         this is foo

Note that the currently supported --preserve-environment may be
overkill or dangerous in some use-cases as it does not reset
environment at all. The whitelist provides better control (for 
example for scripts) over environment in the new session.

 Karel


>From 384c16960cf279caf1d8e4d721a81f2da66c6a51 Mon Sep 17 00:00:00 2001
From: Karel Zak <kzak@redhat.com>
Date: Fri, 10 Aug 2018 10:49:15 +0200
Subject: [PATCH] su: add --whitelist-environment

* usable with --login to whitelist specified environment variables

* the list is ignored for the core variables like HOME, SHELL, USER,
  LOGNAME and PATH (su --login always resets these variables)

Note that su(1) requires password and after successful authentication
user has full control over the session, so he can set arbitrary
environment variables. The whitelist makes things more user friendly
only.

The patch removes unnecessary optimization when allocate environ[]. It
seems better to keep all in glibc hands and just reset the environment
array only.

Addresses: https://github.com/karelzak/util-linux/issues/221
Signed-off-by: Karel Zak <kzak@redhat.com>
---
 configure.ac            |   1 +
 login-utils/su-common.c | 106 ++++++++++++++++++++++++++++++++++++++++--------
 login-utils/su.1        |  10 +++++
 3 files changed, 101 insertions(+), 16 deletions(-)

diff --git a/configure.ac b/configure.ac
index a41ca7689..995d1d70e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -454,6 +454,7 @@ AC_CHECK_DECL([SO_PASSCRED],
                #include <sys/socket.h>])
 
 AC_CHECK_FUNCS([ \
+	clearenv \
 	__fpurge \
 	fpurge \
 	__fpending \
diff --git a/login-utils/su-common.c b/login-utils/su-common.c
index c1b1a04e4..c730df95f 100644
--- a/login-utils/su-common.c
+++ b/login-utils/su-common.c
@@ -55,6 +55,7 @@
 #include "pathnames.h"
 #include "env.h"
 #include "closestream.h"
+#include "strv.h"
 #include "strutils.h"
 #include "ttyutils.h"
 #include "pwdutils.h"
@@ -130,6 +131,9 @@ struct su_context {
 	pid_t		child;			/* fork() baby */
 	int		childstatus;		/* wait() status */
 
+	char		**env_whitelist_names;	/* environment whitelist */
+	char		**env_whitelist_vals;
+
 	struct sigaction oldact[SIGNALS_IDX_COUNT];	/* original sigactions indexed by SIG*_IDX */
 
 #ifdef USE_PTY
@@ -925,6 +929,56 @@ static void create_watching_parent(struct su_context *su)
 	exit(status);
 }
 
+/* Adds @name from the current environment to the whitelist. If @name is not
+ * set then nothing is added to the whitelist and returns 1.
+ */
+static int env_whitelist_add(struct su_context *su, const char *name)
+{
+	const char *env = getenv(name);
+
+	if (!env)
+		return 1;
+	if (strv_extend(&su->env_whitelist_names, name))
+                err_oom();
+	if (strv_extend(&su->env_whitelist_vals, env))
+                err_oom();
+	return 0;
+}
+
+static int env_whitelist_setenv(struct su_context *su, int overwrite)
+{
+	char **one;
+	size_t i = 0;
+	int rc;
+
+	STRV_FOREACH(one, su->env_whitelist_names) {
+		rc = setenv(*one, su->env_whitelist_vals[i], overwrite);
+		if (rc)
+			return rc;
+		i++;
+	}
+
+	return 0;
+}
+
+/* Creates (add to) whitelist from comma delimited string */
+static int env_whitelist_from_string(struct su_context *su, const char *str)
+{
+	char **all = strv_split(str, ",");
+	char **one;
+
+	if (!all) {
+		if (errno == ENOMEM)
+			err_oom();
+		return -EINVAL;
+	}
+
+	STRV_FOREACH(one, all)
+		env_whitelist_add(su, *one);
+	strv_free(all);
+	return 0;
+}
+
 static void setenv_path(const struct passwd *pw)
 {
 	int rc;
@@ -949,26 +1003,38 @@ static void modify_environment(struct su_context *su, const char *shell)
 	DBG(MISC, ul_debug("modify environ[]"));
 
 	/* Leave TERM unchanged.  Set HOME, SHELL, USER, LOGNAME, PATH.
-	 * Unset all other environment variables.
+	 *
+	 * Unset all other environment variables, but follow
+	 * --whitelist-environment if specified.
 	 */
 	if (su->simulate_login) {
-		char *term = getenv("TERM");
-		if (term)
-			term = xstrdup(term);
-
-		environ = xmalloc((6 + ! !term) * sizeof(char *));
-		environ[0] = NULL;
-		if (term) {
-			xsetenv("TERM", term, 1);
-			free(term);
-		}
-
-		xsetenv("HOME", pw->pw_dir, 1);
+		/* leave TERM unchanged */
+		env_whitelist_add(su, "TERM");
+
+		/* Note that original su(1) has allocated environ[] by malloc
+		 * to the number of expected variables. This seems unnecessary
+		 * optimization as libc later realloc(current_size+2) and for
+		 * empty environ[] the curren_size is zero. It seems better to
+		 * keep all logic around environment in glibc's hands.
+		 *                                           --kzak [Aug 2018]
+		 */
+#ifdef HAVE_CLEARENV
+		clearenv();
+#else
+		environ = NULL;
+#endif
+		/* always reset */
 		if (shell)
 			xsetenv("SHELL", shell, 1);
+
+		setenv_path(pw);
+
+		xsetenv("HOME", pw->pw_dir, 1);
 		xsetenv("USER", pw->pw_name, 1);
 		xsetenv("LOGNAME", pw->pw_name, 1);
-		setenv_path(pw);
+
+		/* apply all from whitelist, but no overwrite */
+		env_whitelist_setenv(su, 0);
 
 	/* Set HOME, SHELL, and (if not becoming a superuser) USER and LOGNAME.
 	 */
@@ -1089,7 +1155,10 @@ static bool is_restricted_shell(const char *shell)
 
 static void usage_common(void)
 {
-	fputs(_(" -m, -p, --preserve-environment  do not reset environment variables\n"), stdout);
+	fputs(_(" -m, -p, --preserve-environment      do not reset environment variables\n"), stdout);
+	fputs(_(" -w, --whitelist-environment <list>  don't reset specified variables\n"), stdout);
+	fputs(USAGE_SEPARATOR, stdout);
+
 	fputs(_(" -g, --group <group>             specify the primary group\n"), stdout);
 	fputs(_(" -G, --supp-group <group>        specify a supplemental group\n"), stdout);
 	fputs(USAGE_SEPARATOR, stdout);
@@ -1234,6 +1303,7 @@ int su_main(int argc, char **argv, int mode)
 		{"group", required_argument, NULL, 'g'},
 		{"supp-group", required_argument, NULL, 'G'},
 		{"user", required_argument, NULL, 'u'},	/* runuser only */
+		{"whitelist-environment", required_argument, NULL, 'w'},
 		{"help", no_argument, 0, 'h'},
 		{"version", no_argument, 0, 'V'},
 		{NULL, 0, NULL, 0}
@@ -1248,7 +1318,7 @@ int su_main(int argc, char **argv, int mode)
 	su->conv.appdata_ptr = (void *) su;
 
 	while ((optc =
-		getopt_long(argc, argv, "c:fg:G:lmpPs:u:hV", longopts,
+		getopt_long(argc, argv, "c:fg:G:lmpPs:u:hVw:", longopts,
 			    NULL)) != -1) {
 		switch (optc) {
 		case 'c':
@@ -1283,6 +1353,10 @@ int su_main(int argc, char **argv, int mode)
 			su->change_environment = false;
 			break;
 
+		case 'w':
+			env_whitelist_from_string(su, optarg);
+			break;
+
 		case 'P':
 #ifdef USE_PTY
 			su->pty = 1;
diff --git a/login-utils/su.1 b/login-utils/su.1
index 84ca104ef..be3ccc9b0 100644
--- a/login-utils/su.1
+++ b/login-utils/su.1
@@ -79,6 +79,7 @@ login:
 o
 clears all the environment variables except
 .B TERM
+or variables specified by \fB\-\-whitelist\-environment\fR
 .TP
 o
 initializes the environment variables
@@ -153,6 +154,15 @@ Same as
 .B \-c
 but do not create a new session.  (Discouraged.)
 .TP
+.BR \-w , " \-\-whitelist\-environment" = \fIlist
+Don't reset environment variables specified in comma separated \fIlist\fR when clears
+environment for \fB\-\-login\fR. The whitelist is ignored for the environment variables
+.BR HOME ,
+.BR SHELL ,
+.BR USER ,
+.BR LOGNAME ", and"
+.BR PATH "."
+.TP
 .BR \-V , " \-\-version"
 Display version information and exit.
 .TP
-- 
2.14.4



    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: su(1) --whitelist-environment

[Bruce Dubbs]

On 08/10/2018 04:24 AM, Karel Zak wrote:
> 
>   Hi all,
> 
> I'd like to add a new feature to su(1). Look forward to see your
> objections and comments.
> 
> Example:
> 
>           # export FOO="this is foo"
> 
>           # su - kzak
>           $ echo $FOO
> 
>           # su ---whitelist-environment FOO - kzak
>           $ echo $FOO
>           this is foo
> 
> Note that the currently supported --preserve-environment may be
> overkill or dangerous in some use-cases as it does not reset
> environment at all. The whitelist provides better control (for
> example for scripts) over environment in the new session.

I think it is unnecessary.  su itself does not need the environment 
variable.  If a user needs a specific setting, then set it in a script:

su --command myscript

   -- Bruce Dubbs, LFS

Re: su(1) --whitelist-environment

[Bernhard Voelker]

On 08/10/2018 11:06 PM, Bruce Dubbs wrote:
> I think it is unnecessary.  su itself does not need the environment 
> variable.  If a user needs a specific setting, then set it in a script:
> 
> su --command myscript

I'm 50:50.  The point was to pass in variables values per environment
to a process inside 'su' (or 'sudo'), and one can achieve that with e.g.

  $ su -c 'env VAR="val" myscript' user

Well, this might become slightly trickier with real shell or environment
variables wrt/ correct shell quoting:

  $ VAR='some value'
  $ su -c 'env VAR="'"$VAR"'" myscript' user

It might be worth adding such an example to the documentation.

Have a nice day,
Berny

Re: su(1) --whitelist-environment

[Karel Zak]

On Mon, Aug 13, 2018 at 10:57:01PM +0200, Bernhard Voelker wrote:
> On 08/10/2018 11:06 PM, Bruce Dubbs wrote:
> > I think it is unnecessary.  su itself does not need the environment 
> > variable.  If a user needs a specific setting, then set it in a script:
> > 
> > su --command myscript

The important is to say that reset environment is good, expected and
wanted thing. You want to have full control on environment in many
cases.

> I'm 50:50.  The point was to pass in variables values per environment
> to a process inside 'su' (or 'sudo'), and one can achieve that with e.g.

BTW, sudo has env_check, env_delete, or env_keep to control environment
in the sudoers.

>   $ su -c 'env VAR="val" myscript' user
> 
> Well, this might become slightly trickier with real shell or environment
> variables wrt/ correct shell quoting:
> 
>   $ VAR='some value'
>   $ su -c 'env VAR="'"$VAR"'" myscript' user

Well, probably usable way for scripts, but ugly for users on command line.

All the idea behind the patch is make things more user-friendly

    su -w GREP_COLOR,COLORFGBG - kzak

seems better than assume -c 'env VAR ..."

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: su(1) --whitelist-environment

[Martin Steigerwald]

Karel, thank you for considering to add an option to whitelist certain=20
environment variables. I bet my feedback my have given some inspiration=20
to this.

Karel Zak - 14.08.18, 11:32:
> On Mon, Aug 13, 2018 at 10:57:01PM +0200, Bernhard Voelker wrote:
> > On 08/10/2018 11:06 PM, Bruce Dubbs wrote:
[=E2=80=A6]
> > I'm 50:50.  The point was to pass in variables values per
> > environment
> > to a process inside 'su' (or 'sudo'), and one can achieve that with
> > e.g.
> BTW, sudo has env_check, env_delete, or env_keep to control
> environment in the sudoers.

I meanwhile use sudo for taking over SSH_AUTH_SOCK exactly for this=20
reason. Initializing SSH_AUTH_SOCK manually in a script that runs as=20
root seems much more error prone to me. I chose to have exactly the=20
SSH_AUTH_SOCK of the user who did sudo in the root environment and not=20
the SSH_AUTH_SOCK of another user.

It may be better to use sudo anyway, but I am not going to lock my root=20
account on my laptop like its done with Ubuntu by default. I even=20
switched sudo to require the root password on my laptop meanwhile. As=20
sudo can do all this, I don=C2=B4t really need su, but I think it would not=
=20
hurt to add an option like it and recommend it over just using su=20
without "-".

> >   $ su -c 'env VAR=3D"val" myscript' user
> >=20
> > Well, this might become slightly trickier with real shell or
> > environment>=20
> > variables wrt/ correct shell quoting:
> >   $ VAR=3D'some value'
> >   $ su -c 'env VAR=3D"'"$VAR"'" myscript' user
>=20
> Well, probably usable way for scripts, but ugly for users on command
> line.
>=20
> All the idea behind the patch is make things more user-friendly
>=20
>     su -w GREP_COLOR,COLORFGBG - kzak
>=20
> seems better than assume -c 'env VAR ..."

I quite like this approach after I got why just taking over the complete=20
environment of the user can cause security and other issues in script=20
behavior at least on machines used by mutiple users. And can cause=20
issues if someone else manages to manipulate the environment of a user=20
only I should have access to.

Thanks,
=2D-=20
Martin

Re: su(1) --whitelist-environment

[Bernhard Voelker]

On 08/14/2018 11:32 AM, Karel Zak wrote:
> On Mon, Aug 13, 2018 at 10:57:01PM +0200, Bernhard Voelker wrote:
>> Well, this might become slightly trickier with real shell or environment
>> variables wrt/ correct shell quoting:
>>
>>   $ VAR='some value'
>>   $ su -c 'env VAR="'"$VAR"'" myscript' user
> 
> Well, probably usable way for scripts, but ugly for users on command line.
> 
> All the idea behind the patch is make things more user-friendly
> 
>     su -w GREP_COLOR,COLORFGBG - kzak
> 
> seems better than assume -c 'env VAR ..."

+1 your point. ;-)

A difference is that  -c 'env VAR ..." works also with shell variables
while -w filters exported variables only.  I think this not a big deal.

Regarding your implementation: I suggest to warn when -w is used together
with -m, -p, --preserve-environment.  WDYT?

Have a nice day,
Berny

Re: su(1) --whitelist-environment

[Karel Zak]

On Tue, Aug 14, 2018 at 02:50:12PM +0200, Bernhard Voelker wrote:
> On 08/14/2018 11:32 AM, Karel Zak wrote:
> > On Mon, Aug 13, 2018 at 10:57:01PM +0200, Bernhard Voelker wrote:
> >> Well, this might become slightly trickier with real shell or environment
> >> variables wrt/ correct shell quoting:
> >>
> >>   $ VAR='some value'
> >>   $ su -c 'env VAR="'"$VAR"'" myscript' user
> > 
> > Well, probably usable way for scripts, but ugly for users on command line.
> > 
> > All the idea behind the patch is make things more user-friendly
> > 
> >     su -w GREP_COLOR,COLORFGBG - kzak
> > 
> > seems better than assume -c 'env VAR ..."
> 
> +1 your point. ;-)
> 
> A difference is that  -c 'env VAR ..." works also with shell variables
> while -w filters exported variables only.  I think this not a big deal.
> 
> Regarding your implementation: I suggest to warn when -w is used together
> with -m, -p, --preserve-environment.  WDYT?

Fixed and pushed. Thanks all for review and opinions.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com