Sorry for the delay.
I like the general direction, but I'm not 100% convinced the semantics
are fine-tuned enough.
On 6/13/19 4:16 AM, Ted Unangst wrote:
> This has come up a few times before. For background, the default rule for doas
> is to copy a few environment settings from the user and omit the rest. This is
> to prevent confusion, and also supposedly for security. However, some of the
> alleged safe variables like PATH probably aren't that safe. And things like
> USER are confusing? And why even bother with MAIL? The list is kinda adhoc and
> mostly copied from what I understood sudo to do at the time, but I believe
> sudo has changed defaults as well.
>
> So here's a new model which I think is safer and more consistent.
>
> 1. Always add a DOAS_USER with the invoking user's name.
Why not add a DOAS_UID and DOAS_GID as well. From testing I found that
sudo sets them from the passwd values, but since these values can easily
be derived from DOAS_USER (unless there's overlap between passwd(5) and
ypldap(8)); I propose that we save the original uid and egid. Egid,
because that's the gid that's actually in effect and uid because we
already lost euid to doas (0).
suggested diff below.
I don't know if it's worth to store DOAS_COMMAND (similar to sudo), or
maybe store some additional values that we overwrite by default, but it's
easier to add to the DOAS_* set than remove them, so that can always be
determined later.
>
> 2. When doing keepenv, nothing changes, except addition of above.
It feels inconsistent to make keepenv behave different here.
- It may allow certain applications to behave unexpected compared to
without keepenv (e.g. scripts that use $HOME/.cache).
- The values are hard to retrofit into doas.conf by the end-user, while
it's easy to keep the original with setenv { HOME ... }.
I'm not convinced one way or the other, but for now it just feels weird.
>
> 3. When starting with a new environment, fill in USER and HOME and such with
> the values of the target user, instead of copying from the invoking user. You
> run a command as a user, you should have that user's environment.
This I like.
>
> 4. DISPLAY and TERM are still copied, since they represent a description of
> the actual environment in which we are running. (Not sure how useful display
> is without xauth cookies, but not my problem.)
>
Sure.
> 5. As before, anything can be overridden with setenv in the config.
See point 2.
>
> This is a kinda breaking change, but probably in a good way.
> For simple configurations, I expect nothing changes. For more complicated
> setups, this new handling is probably closer to what the admin expects or
> desires.
>
>
> Index: doas.c
> ===================================================================
> RCS file: /home/cvs/src/usr.bin/doas/doas.c,v
> retrieving revision 1.76
> diff -u -p -r1.76 doas.c
> --- doas.c 12 Jun 2019 02:50:29 -0000 1.76
> +++ doas.c 13 Jun 2019 02:11:07 -0000
> @@ -421,6 +421,7 @@ main(int argc, char **argv)
> errx(1, "no passwd entry for target");
>
> if (setusercontext(NULL, targpw, target, LOGIN_SETGROUP |
> + LOGIN_SETPATH |
> LOGIN_SETPRIORITY | LOGIN_SETRESOURCES | LOGIN_SETUMASK |
> LOGIN_SETUSER) != 0)
> errx(1, "failed to set user context for target");
> @@ -439,8 +440,13 @@ main(int argc, char **argv)
> syslog(LOG_AUTHPRIV | LOG_INFO, "%s ran command %s as %s from %s",
> mypw->pw_name, cmdline, targpw->pw_name, cwd);
>
> - envp = prepenv(rule);
> + envp = prepenv(rule, mypw, targpw);
>
> + if (rule->cmd) {
> + /* do this again after setusercontext reset it */
> + if (setenv("PATH", safepath, 1) == -1)
> + err(1, "failed to set PATH '%s'", safepath);
> + }
> execvpe(cmd, argv, envp);
> fail:
> if (errno == ENOENT)
I'm not convinced that LOGIN_SETPATH is a good idea here. From what I
gathered that sets PATH from login.conf(5), while most environments I
know will use .profile to set it and could cause unexpected behaviour
if the my and targ PATH are reset to unexpected values.
I would vote to safe PATH from the caller instead of getting a likely
unexpected or incomplete PATH environment based on login.conf.
> Index: doas.conf.5
> ===================================================================
> RCS file: /home/cvs/src/usr.bin/doas/doas.conf.5,v
> retrieving revision 1.35
> diff -u -p -r1.35 doas.conf.5
> --- doas.conf.5 7 Feb 2018 05:13:57 -0000 1.35
> +++ doas.conf.5 13 Jun 2019 01:41:18 -0000
> @@ -51,15 +51,9 @@ again for some time.
> .It Ic keepenv
> The user's environment is maintained.
> The default is to reset the environment, except for the variables
> -.Ev DISPLAY ,
> -.Ev HOME ,
> -.Ev LOGNAME ,
> -.Ev MAIL ,
> -.Ev PATH ,
> -.Ev TERM ,
> -.Ev USER
> +.Ev DISPLAY
> and
> -.Ev USERNAME .
> +.Ev TERM .
> .It Ic setenv { Oo Ar variable ... Oc Oo Ar variable=value ... Oc Ic }
> In addition to the variables mentioned above, keep the space-separated
> specified variables.
Maybe elaborate a little bit more on DOAS_USER and the resetting of the
environment.
martijn@
Index: doas.c
===================================================================
RCS file: /cvs/src/usr.bin/doas/doas.c,v
retrieving revision 1.76
diff -u -p -r1.76 doas.c
--- doas.c 12 Jun 2019 02:50:29 -0000 1.76
+++ doas.c 15 Jun 2019 09:42:58 -0000
@@ -352,6 +352,8 @@ main(int argc, char **argv)
err(1, "getpwuid_r failed");
if (mypw == NULL)
errx(1, "no passwd entry for self");
+ mypw->pw_uid = uid;
+ mypw->pw_gid = getegid();
ngroups = getgroups(NGROUPS_MAX, groups);
if (ngroups == -1)
err(1, "can't get groups");
@@ -421,6 +423,7 @@ main(int argc, char **argv)
errx(1, "no passwd entry for target");
if (setusercontext(NULL, targpw, target, LOGIN_SETGROUP |
+ LOGIN_SETPATH |
LOGIN_SETPRIORITY | LOGIN_SETRESOURCES | LOGIN_SETUMASK |
LOGIN_SETUSER) != 0)
errx(1, "failed to set user context for target");
@@ -439,8 +442,13 @@ main(int argc, char **argv)
syslog(LOG_AUTHPRIV | LOG_INFO, "%s ran command %s as %s from %s",
mypw->pw_name, cmdline, targpw->pw_name, cwd);
- envp = prepenv(rule);
+ envp = prepenv(rule, mypw, targpw);
+ if (rule->cmd) {
+ /* do this again after setusercontext reset it */
+ if (setenv("PATH", safepath, 1) == -1)
+ err(1, "failed to set PATH '%s'", safepath);
+ }
execvpe(cmd, argv, envp);
fail:
if (errno == ENOENT)
Index: doas.conf.5
===================================================================
RCS file: /cvs/src/usr.bin/doas/doas.conf.5,v
retrieving revision 1.35
diff -u -p -r1.35 doas.conf.5
--- doas.conf.5 7 Feb 2018 05:13:57 -0000 1.35
+++ doas.conf.5 15 Jun 2019 09:42:58 -0000
@@ -51,15 +51,9 @@ again for some time.
.It Ic keepenv
The user's environment is maintained.
The default is to reset the environment, except for the variables
-.Ev DISPLAY ,
-.Ev HOME ,
-.Ev LOGNAME ,
-.Ev MAIL ,
-.Ev PATH ,
-.Ev TERM ,
-.Ev USER
+.Ev DISPLAY
and
-.Ev USERNAME .
+.Ev TERM .
.It Ic setenv { Oo Ar variable ... Oc Oo Ar variable=value ... Oc Ic }
In addition to the variables mentioned above, keep the space-separated
specified variables.
Index: doas.h
===================================================================
RCS file: /cvs/src/usr.bin/doas/doas.h,v
retrieving revision 1.13
diff -u -p -r1.13 doas.h
--- doas.h 6 Apr 2017 21:12:06 -0000 1.13
+++ doas.h 15 Jun 2019 09:42:58 -0000
@@ -29,7 +29,10 @@ extern struct rule **rules;
extern int nrules;
extern int parse_errors;
-char **prepenv(const struct rule *);
+struct passwd;
+
+char **prepenv(const struct rule *, const struct passwd *,
+ const struct passwd *);
#define PERMIT 1
#define DENY 2
Index: env.c
===================================================================
RCS file: /cvs/src/usr.bin/doas/env.c,v
retrieving revision 1.6
diff -u -p -r1.6 env.c
--- env.c 6 Apr 2017 21:12:06 -0000 1.6
+++ env.c 15 Jun 2019 09:42:58 -0000
@@ -24,6 +24,7 @@
#include <err.h>
#include <unistd.h>
#include <errno.h>
+#include <pwd.h>
#include "doas.h"
@@ -38,6 +39,8 @@ struct env {
u_int count;
};
+static void fillenv(struct env *env, const char **envlist);
+
static int
envcmp(struct envnode *a, struct envnode *b)
{
@@ -68,10 +71,22 @@ freenode(struct envnode *node)
free(node);
}
+static void
+addnode(struct env *env, const char *key, const char *value)
+{
+ struct envnode *node;
+
+ node = createnode(key, value);
+ RB_INSERT(envtree, &env->root, node);
+ env->count++;
+}
+
static struct env *
-createenv(const struct rule *rule)
+createenv(const struct rule *rule, const struct passwd *mypw,
+ const struct passwd *targpw)
{
struct env *env;
+ char id[64];
u_int i;
env = malloc(sizeof(*env));
@@ -80,6 +95,12 @@ createenv(const struct rule *rule)
RB_INIT(&env->root);
env->count = 0;
+ addnode(env, "DOAS_USER", mypw->pw_name);
+ snprintf(id, sizeof(id), "%u", mypw->pw_uid);
+ addnode(env, "DOAS_UID", id);
+ snprintf(id, sizeof(id), "%u", mypw->pw_gid);
+ addnode(env, "DOAS_GID", id);
+
if (rule->options & KEEPENV) {
extern const char **environ;
@@ -108,6 +129,19 @@ createenv(const struct rule *rule)
env->count++;
}
}
+ } else {
+ static const char *copyset[] = {
+ "DISPLAY", "TERM",
+ NULL
+ };
+
+ addnode(env, "HOME", targpw->pw_dir);
+ addnode(env, "LOGNAME", targpw->pw_name);
+ addnode(env, "PATH", getenv("PATH"));
+ addnode(env, "SHELL", targpw->pw_shell);
+ addnode(env, "USER", targpw->pw_name);
+
+ fillenv(env, copyset);
}
return env;
@@ -186,20 +220,12 @@ fillenv(struct env *env, const char **en
}
char **
-prepenv(const struct rule *rule)
+prepenv(const struct rule *rule, const struct passwd *mypw,
+ const struct passwd *targpw)
{
- static const char *safeset[] = {
- "DISPLAY", "HOME", "LOGNAME", "MAIL",
- "PATH", "TERM", "USER", "USERNAME",
- NULL
- };
struct env *env;
- env = createenv(rule);
-
- /* if we started with blank, fill some defaults then apply rules */
- if (!(rule->options & KEEPENV))
- fillenv(env, safeset);
+ env = createenv(rule, mypw, targpw);
if (rule->envlist)
fillenv(env, rule->envlist);
