chroot vs su vs doas

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

chroot vs su vs doas

Marc Espie-2
So, in dpb, I've been forking a lot of 'chroot -u user /build'
to build various things, and it works just great.

I was wondering about the benefits of
su ${BUILDUSER} -c 'some quoted command'
vs
chroot -u ${BUILDUSER} / some unquoted command

Superficially, it looks mostly similar.  

The very nice thing about chroot (IMO) being that you can pass the
command verbatim without having to re-quote anything.  The other
difference being that chroot doesn't fork an extra shell, which
might make things more transparent wrt running commands.

I'm also wondering about doas.
By default, it's not configured at all.

But what would it hurt to allow root usage ?
Specifically,

doas -u ${BUILDUSER} some unquoted command

as run by root.  This would not open any security hole, would it ?

Finally, I'm wondering if people would have any use for a chroot'd
option in doas, and whether it's a security issue (again).

Like, people have some hardened doas.conf which only allows running
some commands as root.

Some of these commands are basically game over, as they allow anything
to be run, actually. Specifically, /usr/bin/env, or chroot...

Would
doas -c /rootdir somecmd
be of any use ?

Reply | Threaded
Open this post in threaded view
|

Re: chroot vs su vs doas

Martijn van Duren-5
On 5/13/19 9:13 AM, Marc Espie wrote:

> So, in dpb, I've been forking a lot of 'chroot -u user /build'
> to build various things, and it works just great.
>
> I was wondering about the benefits of
> su ${BUILDUSER} -c 'some quoted command'
> vs
> chroot -u ${BUILDUSER} / some unquoted command
>
> Superficially, it looks mostly similar.  
>
> The very nice thing about chroot (IMO) being that you can pass the
> command verbatim without having to re-quote anything.  The other
> difference being that chroot doesn't fork an extra shell, which
> might make things more transparent wrt running commands.
>
> I'm also wondering about doas.
> By default, it's not configured at all.
>
> But what would it hurt to allow root usage ?
> Specifically,
>
> doas -u ${BUILDUSER} some unquoted command
>
> as run by root.  This would not open any security hole, would it ?

I don't see any and I've been bitten by having a rootshell open and
typing doas out of habit.

lightly tested diff below.

>
> Finally, I'm wondering if people would have any use for a chroot'd
> option in doas, and whether it's a security issue (again).
>
> Like, people have some hardened doas.conf which only allows running
> some commands as root.
>
> Some of these commands are basically game over, as they allow anything
> to be run, actually. Specifically, /usr/bin/env, or chroot...
>
> Would
> doas -c /rootdir somecmd
> be of any use ?

Not particularly opposed, but the extend of this option should be
examined. E.g. do we want to extend it to the config to be something
similar to -u and limit it's use for certain commands?
>
Index: doas.c
===================================================================
RCS file: /cvs/src/usr.bin/doas/doas.c,v
retrieving revision 1.74
diff -u -p -r1.74 doas.c
--- doas.c 17 Jan 2019 05:35:35 -0000 1.74
+++ doas.c 13 May 2019 07:57:29 -0000
@@ -132,6 +132,15 @@ static int
 permit(uid_t uid, gid_t *groups, int ngroups, const struct rule **lastr,
     uid_t target, const char *cmd, const char **cmdargs)
 {
+ static struct rule allowroot = {
+ .action = PERMIT,
+ .options = NOPASS,
+ .ident = NULL,
+ .target = NULL,
+ .cmd = NULL,
+ .cmdargs = NULL,
+ .envlist = NULL
+ };
  int i;
 
  *lastr = NULL;
@@ -140,8 +149,13 @@ permit(uid_t uid, gid_t *groups, int ngr
     cmdargs, rules[i]))
  *lastr = rules[i];
  }
- if (!*lastr)
+ if (!*lastr) {
+ if (uid == 0) {
+ *lastr = &allowroot;
+ return PERMIT;
+ }
  return 0;
+ }
  return (*lastr)->action == PERMIT;
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: chroot vs su vs doas

Martijn van Duren-5
On 5/13/19 10:00 AM, Martijn van Duren wrote:

> On 5/13/19 9:13 AM, Marc Espie wrote:
>> So, in dpb, I've been forking a lot of 'chroot -u user /build'
>> to build various things, and it works just great.
>>
>> I was wondering about the benefits of
>> su ${BUILDUSER} -c 'some quoted command'
>> vs
>> chroot -u ${BUILDUSER} / some unquoted command
>>
>> Superficially, it looks mostly similar.  
>>
>> The very nice thing about chroot (IMO) being that you can pass the
>> command verbatim without having to re-quote anything.  The other
>> difference being that chroot doesn't fork an extra shell, which
>> might make things more transparent wrt running commands.
>>
>> I'm also wondering about doas.
>> By default, it's not configured at all.
>>
>> But what would it hurt to allow root usage ?
>> Specifically,
>>
>> doas -u ${BUILDUSER} some unquoted command
>>
>> as run by root.  This would not open any security hole, would it ?
>
> I don't see any and I've been bitten by having a rootshell open and
> typing doas out of habit.
>
> lightly tested diff below.
>>
>> Finally, I'm wondering if people would have any use for a chroot'd
>> option in doas, and whether it's a security issue (again).
>>
>> Like, people have some hardened doas.conf which only allows running
>> some commands as root.
>>
>> Some of these commands are basically game over, as they allow anything
>> to be run, actually. Specifically, /usr/bin/env, or chroot...
>>
>> Would
>> doas -c /rootdir somecmd
>> be of any use ?
>
> Not particularly opposed, but the extend of this option should be
> examined. E.g. do we want to extend it to the config to be something
> similar to -u and limit it's use for certain commands?
>
Working this out I'm not particularly a fan of the extra code this would
take, but the diff below seems to do the trick.

martijn@

Index: doas.c
===================================================================
RCS file: /cvs/src/usr.bin/doas/doas.c,v
retrieving revision 1.74
diff -u -p -r1.74 doas.c
--- doas.c 17 Jan 2019 05:35:35 -0000 1.74
+++ doas.c 13 May 2019 09:23:41 -0000
@@ -89,8 +89,9 @@ parsegid(const char *s, gid_t *gid)
 }
 
 static int
-match(uid_t uid, gid_t *groups, int ngroups, uid_t target, const char *cmd,
-    const char **cmdargs, struct rule *r)
+match(uid_t uid, gid_t *groups, int ngroups, uid_t target,
+    const char *chrootpath, const char *cmd, const char **cmdargs,
+    struct rule *r)
 {
  int i;
 
@@ -110,6 +111,12 @@ match(uid_t uid, gid_t *groups, int ngro
  }
  if (r->target && uidcheck(r->target, target) != 0)
  return 0;
+ if (r->chroot != NULL) {
+ if (chrootpath == NULL)
+ return 0;
+ if (strcmp(r->chroot, chrootpath) != 0)
+ return 0;
+ }
  if (r->cmd) {
  if (strcmp(r->cmd, cmd))
  return 0;
@@ -130,13 +137,13 @@ match(uid_t uid, gid_t *groups, int ngro
 
 static int
 permit(uid_t uid, gid_t *groups, int ngroups, const struct rule **lastr,
-    uid_t target, const char *cmd, const char **cmdargs)
+    uid_t target, const char *chrootpath, const char *cmd, const char **cmdargs)
 {
  int i;
 
  *lastr = NULL;
  for (i = 0; i < nrules; i++) {
- if (match(uid, groups, ngroups, target, cmd,
+ if (match(uid, groups, ngroups, target, chrootpath, cmd,
     cmdargs, rules[i]))
  *lastr = rules[i];
  }
@@ -173,7 +180,7 @@ parseconfig(const char *filename, int ch
 }
 
 static void __dead
-checkconfig(const char *confpath, int argc, char **argv,
+checkconfig(const char *confpath, int argc, char **argv, const char *chrootpath,
     uid_t uid, gid_t *groups, int ngroups, uid_t target)
 {
  const struct rule *rule;
@@ -183,7 +190,7 @@ checkconfig(const char *confpath, int ar
  if (!argc)
  exit(0);
 
- if (permit(uid, groups, ngroups, &rule, target, argv[0],
+ if (permit(uid, groups, ngroups, &rule, target, chrootpath, argv[0],
     (const char **)argv + 1)) {
  printf("permit%s\n", (rule->options & NOPASS) ? " nopass" : "");
  exit(0);
@@ -286,6 +293,7 @@ main(int argc, char **argv)
  const char *confpath = NULL;
  char *shargv[] = { NULL, NULL };
  char *sh;
+ const char *chrootpath = NULL;
  const char *cmd;
  char cmdline[LINE_MAX];
  char myname[_PW_NAME_LEN + 1];
@@ -309,11 +317,14 @@ main(int argc, char **argv)
 
  uid = getuid();
 
- while ((ch = getopt(argc, argv, "a:C:Lnsu:")) != -1) {
+ while ((ch = getopt(argc, argv, "a:c:C:Lnsu:")) != -1) {
  switch (ch) {
  case 'a':
  login_style = optarg;
  break;
+ case 'c':
+ chrootpath = optarg;
+ break;
  case 'C':
  confpath = optarg;
  break;
@@ -369,8 +380,8 @@ main(int argc, char **argv)
  }
 
  if (confpath) {
- checkconfig(confpath, argc, argv, uid, groups, ngroups,
-    target);
+ checkconfig(confpath, argc, argv, chrootpath, uid, groups,
+    ngroups, target);
  exit(1); /* fail safe */
  }
 
@@ -389,7 +400,7 @@ main(int argc, char **argv)
  }
 
  cmd = argv[0];
- if (!permit(uid, groups, ngroups, &rule, target, cmd,
+ if (!permit(uid, groups, ngroups, &rule, target, chrootpath, cmd,
     (const char **)argv + 1)) {
  syslog(LOG_AUTHPRIV | LOG_NOTICE,
     "failed command for %s: %s", myname, cmdline);
@@ -403,8 +414,17 @@ main(int argc, char **argv)
  authuser(myname, login_style, rule->options & PERSIST);
  }
 
- if (unveil(_PATH_LOGIN_CONF, "r") == -1)
- err(1, "unveil");
+ pw = getpwuid(target);
+ if (!pw)
+ errx(1, "no passwd entry for target");
+
+ if (chrootpath != NULL) {
+ if (chroot(chrootpath) == -1)
+ err(1, "failed to chroot");
+ if (chdir("/") == -1)
+ err(1, "failed to chdir to chroot");
+ }
+
  if (rule->cmd) {
  if (setenv("PATH", safepath, 1) == -1)
  err(1, "failed to set PATH '%s'", safepath);
@@ -415,9 +435,6 @@ main(int argc, char **argv)
  if (pledge("stdio rpath getpw exec id", NULL) == -1)
  err(1, "pledge");
 
- pw = getpwuid(target);
- if (!pw)
- errx(1, "no passwd entry for target");
 
  if (setusercontext(NULL, pw, target, LOGIN_SETGROUP |
     LOGIN_SETPRIORITY | LOGIN_SETRESOURCES | LOGIN_SETUMASK |
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 13 May 2019 09:23:41 -0000
@@ -20,6 +20,7 @@ struct rule {
  int options;
  const char *ident;
  const char *target;
+ const char *chroot;
  const char *cmd;
  const char **cmdargs;
  const char **envlist;
Index: parse.y
===================================================================
RCS file: /cvs/src/usr.bin/doas/parse.y,v
retrieving revision 1.27
diff -u -p -r1.27 parse.y
--- parse.y 11 Jul 2018 07:39:22 -0000 1.27
+++ parse.y 13 May 2019 09:23:41 -0000
@@ -69,7 +69,7 @@ arraylen(const char **arr)
 
 %}
 
-%token TPERMIT TDENY TAS TCMD TARGS
+%token TPERMIT TDENY TAS TCHROOT TCMD TARGS
 %token TNOPASS TPERSIST TKEEPENV TSETENV
 %token TSTRING
 
@@ -81,7 +81,7 @@ grammar: /* empty */
  | error '\n'
  ;
 
-rule: action ident target cmd {
+rule: action ident target chroot cmd {
  struct rule *r;
  r = calloc(1, sizeof(*r));
  if (!r)
@@ -91,8 +91,9 @@ rule: action ident target cmd {
  r->envlist = $1.envlist;
  r->ident = $2.str;
  r->target = $3.str;
- r->cmd = $4.cmd;
- r->cmdargs = $4.cmdargs;
+ r->chroot = $4.str;
+ r->cmd = $5.cmd;
+ r->cmdargs = $5.cmdargs;
  if (nrules == maxrules) {
  if (maxrules == 0)
  maxrules = 63;
@@ -170,6 +171,12 @@ target: /* optional */ {
  $$.str = $2.str;
  } ;
 
+chroot: /* optional */ {
+ $$.str = NULL;
+ } | TCHROOT TSTRING {
+ $$.str = $2.str;
+ } ;
+
 cmd: /* optional */ {
  $$.cmd = NULL;
  $$.cmdargs = NULL;
@@ -206,6 +213,7 @@ static struct keyword {
  { "deny", TDENY },
  { "permit", TPERMIT },
  { "as", TAS },
+ { "chroot", TCHROOT },
  { "cmd", TCMD },
  { "args", TARGS },
  { "nopass", TNOPASS },

Reply | Threaded
Open this post in threaded view
|

Re: chroot vs su vs doas

Ted Unangst-6
In reply to this post by Martijn van Duren-5
Martijn van Duren wrote:
> > But what would it hurt to allow root usage ?
> > Specifically,
> >
> > doas -u ${BUILDUSER} some unquoted command
> >
> > as run by root.  This would not open any security hole, would it ?
>
> I don't see any and I've been bitten by having a rootshell open and
> typing doas out of habit.

The reason there's no builtin config is to prevent confusion, even if it
sometimes causes mild annoyance. When there are invisible rules, it becomes
harder to know which rule is actually being taken.

For example, your rule below doesn't include keepenv. So next week we're going
to get bug reports that things work when run as a user, but not as root. And
for exactly the same reason, people only half set things up. The fact that the
default appears to work sometimes makes things even more annoying. And I
don't think the default should just be changed to include keepenv, because
maybe that's not what people want and then we'd need to explain how to undo
that.


> + static struct rule allowroot = {
> + .action = PERMIT,
> + .options = NOPASS,
> + .ident = NULL,
> + .target = NULL,
> + .cmd = NULL,
> + .cmdargs = NULL,
> + .envlist = NULL
> + };

Reply | Threaded
Open this post in threaded view
|

Re: chroot vs su vs doas

Ted Unangst-6
In reply to this post by Martijn van Duren-5
Martijn van Duren wrote:

> >> Would
> >> doas -c /rootdir somecmd
> >> be of any use ?
> >
> > Not particularly opposed, but the extend of this option should be
> > examined. E.g. do we want to extend it to the config to be something
> > similar to -u and limit it's use for certain commands?
> >
> Working this out I'm not particularly a fan of the extra code this would
> take, but the diff below seems to do the trick.

This seems like feature creep. We already have a command to chroot and switch
user.