Re: new env defaults for doas
On Mon, 17 Jun 2019 13:17:15 -0400, "Ted Unangst" wrote: > Not sure what you mean. This diff does put it before the options. > > same place as su, although su has other problems. fortunately, we don't have > the same combination of possibilities as su. Sorry, I misread the diff. The placement in that diff is fine. - todd
Re: new env defaults for doas
Todd C. Miller wrote: > On Mon, 17 Jun 2019 12:58:00 -0400, "Ted Unangst" wrote: > > > Committed this. I'm not entirely happy with the wording. I think hiding them > > under an option in the config man page is the wrong place. The default > > behavior should be documented in a more default place. > > I would just place that bit either before the options are described > or after the options in an "Execution environment" sub-section. Not sure what you mean. This diff does put it before the options. same place as su, although su has other problems. fortunately, we don't have the same combination of possibilities as su.
Re: new env defaults for doas
On Mon, 17 Jun 2019 12:58:00 -0400, "Ted Unangst" wrote: > Committed this. I'm not entirely happy with the wording. I think hiding them > under an option in the config man page is the wrong place. The default > behavior should be documented in a more default place. I would just place that bit either before the options are described or after the options in an "Execution environment" sub-section. - todd
Re: new env defaults for doas
Ted Unangst wrote: > Yes, I think it's better to always reset these things. Here's a diff. > > 1. Always set HOME, PATH, SHELL etc to the target. Committed this. I'm not entirely happy with the wording. I think hiding them under an option in the config man page is the wrong place. The default behavior should be documented in a more default place. Also mention working directory is not changed. Index: doas.1 === RCS file: /cvs/src/usr.bin/doas/doas.1,v retrieving revision 1.19 diff -u -p -r1.19 doas.1 --- doas.1 4 Sep 2016 15:20:37 - 1.19 +++ doas.1 17 Jun 2019 16:57:26 - @@ -40,6 +40,23 @@ or .Fl s is specified. .Pp +By default, the environment is reset. +The variables +.Ev HOME , +.Ev LOGNAME , +.Ev PATH , +.Ev SHELL , +and +.Ev USER +are set to values appropriate for the target user. +The variables +.Ev DISPLAY +and +.Ev TERM +are inherited from the current environment. +This behavior may be modified by the config file. +The working directory is not changed. +.Pp The options are as follows: .Bl -tag -width tenletters .It Fl a Ar style
Re: new env defaults for doas
On Sun, 16 Jun 2019 14:23:24 -0400, "Ted Unangst" wrote: > Yes, I think it's better to always reset these things. Here's a diff. > > 1. Always set HOME, PATH, SHELL etc to the target. > > 2. Without keepenv, other environment variables are discarded. > > 3. With keepenv, other variables are retained, but the above are still reset. > > 4. Possible to override this with setenv. > > This is much more consistent and predictable. That does seem better. The user can always preserve some or all of those variables via setenv. OK millert@ - todd
Re: new env defaults for doas
Martijn van Duren wrote:
> > 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 ... }.
Yes, I think it's better to always reset these things. Here's a diff.
1. Always set HOME, PATH, SHELL etc to the target.
2. Without keepenv, other environment variables are discarded.
3. With keepenv, other variables are retained, but the above are still reset.
4. Possible to override this with setenv.
This is much more consistent and predictable.
Index: doas.conf.5
===
RCS file: /cvs/src/usr.bin/doas/doas.conf.5,v
retrieving revision 1.36
diff -u -p -r1.36 doas.conf.5
--- doas.conf.5 16 Jun 2019 18:16:34 - 1.36
+++ doas.conf.5 16 Jun 2019 18:20:20 -
@@ -54,6 +54,14 @@ The default is to reset the environment,
.Ev DISPLAY
and
.Ev TERM .
+The variables
+.Ev HOME ,
+.Ev LOGNAME ,
+.Ev PATH ,
+.Ev SHELL ,
+and
+.Ev USER
+are always reset.
.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: env.c
===
RCS file: /cvs/src/usr.bin/doas/env.c,v
retrieving revision 1.7
diff -u -p -r1.7 env.c
--- env.c 16 Jun 2019 18:16:34 - 1.7
+++ env.c 16 Jun 2019 18:20:20 -
@@ -85,6 +85,10 @@ static struct env *
createenv(const struct rule *rule, const struct passwd *mypw,
const struct passwd *targpw)
{
+ static const char *copyset[] = {
+ "DISPLAY", "TERM",
+ NULL
+ };
struct env *env;
u_int i;
@@ -95,6 +99,13 @@ createenv(const struct rule *rule, const
env->count = 0;
addnode(env, "DOAS_USER", mypw->pw_name);
+ 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);
if (rule->options & KEEPENV) {
extern const char **environ;
@@ -124,19 +135,6 @@ createenv(const struct rule *rule, const
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;
Re: new env defaults for doas
Martijn van Duren wrote: > 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. Short answer is this is what su - does. We can set a default safe path, but there's no reason that's better than what's in .profile either. With login.conf, we get a safe default and a way for the admin to fix it if it's not what they want.
Re: new env defaults for doas
Martijn van Duren wrote: > 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. Why do all this? How many usage cases are there? I can't think of any. Will a sub-process ever be in a situation where it knows more than it should? That feels more likely.
Re: new env defaults for doas
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.c12 Jun 2019 02:50:29 - 1.76
> +++ doas.c13 Jun 2019 02:11:07 -
> @@ -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 - 1.35
> +++ doas.conf.5 13 Jun 2019 01:41:18 -
> @@ -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
new env defaults for doas
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.
2. When doing keepenv, nothing changes, except addition of above.
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.
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.)
5. As before, anything can be overridden with setenv in the config.
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 - 1.76
+++ doas.c 13 Jun 2019 02:11:07 -
@@ -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)
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 - 1.35
+++ doas.conf.5 13 Jun 2019 01:41:18 -
@@ -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: /home/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 - 1.13
+++ doas.h 13 Jun 2019 01:10:29 -
@@ -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: /home/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 - 1.6
+++ env.c 13 Jun 2019 02:12:57 -
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#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,8 +71,19 @@ 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, >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;
u_int i;
@@ -80,6 +94,8 @@ createenv(const struct rule *rule)
