From: David Gibson <david@gibson.dropbear.id.au>
To: passt-dev@passt.top
Subject: Re: [PATCH v2 03/10] Consolidate determination of UID/GID to run as
Date: Tue, 13 Sep 2022 15:20:49 +1000 [thread overview]
Message-ID: <YyATMbuHZfivWZJE@yekko> (raw)
In-Reply-To: <20220913054952.02818197@elisabeth>
[-- Attachment #1: Type: text/plain, Size: 6046 bytes --]
On Tue, Sep 13, 2022 at 05:49:52AM +0200, Stefano Brivio wrote:
> On Mon, 12 Sep 2022 19:53:38 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
>
> > On Sat, Sep 10, 2022 at 10:43:31PM +0200, Stefano Brivio wrote:
> > > On Sat, 10 Sep 2022 17:15:41 +1000
> > > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> > >
> > > > On Fri, Sep 09, 2022 at 04:33:47PM +0200, Stefano Brivio wrote:
> > > > > Also mere nitpicking on this one:
> > > > >
> > > > > On Thu, 8 Sep 2022 13:59:00 +1000
> > > > > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> > > > >
> > > > > > Currently the logic to work out what UID and GID we will run as is spread
> > > > > > across conf(). If --runas is specified it's handled in conf_runas(),
> > > > > > otherwise it's handled by check_root(), which depends on initialization of
> > > > > > the uid and gid variables by either conf() itself or conf_runas().
> > > > > >
> > > > > > Make this clearer by putting all the UID and GID logic into a single
> > > > > > conf_ugid() function.
> > > > > >
> > > > > > Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
> > > > > > ---
> > > > > > conf.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
> > > > > > util.c | 50 ------------------------------------
> > > > > > util.h | 1 -
> > > > > > 3 files changed, 73 insertions(+), 59 deletions(-)
> > > > > >
> > > > > > diff --git a/conf.c b/conf.c
> > > > > > index 545f61d..5c293b5 100644
> > > > > > --- a/conf.c
> > > > > > +++ b/conf.c
> > > > > > @@ -1021,6 +1021,70 @@ static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid)
> > > > > > #endif /* !GLIBC_NO_STATIC_NSS */
> > > > > > }
> > > > > >
> > > > > > +/**
> > > > > > + * conf_ugid() - Determine UID and GID to run as
> > > > > > + * @runas: --runas option, may be NULL
> > > > > > + * @uid: User ID, set on success
> > > > > > + * @gid: Group ID, set on success
> > > > > > + *
> > > > > > + * Return: 0 on success, negative error code on failure
> > > > > > + */
> > > > > > +static int conf_ugid(const char *runas, uid_t *uid, gid_t *gid)
> > > > > > +{
> > > > > > + const char root_uid_map[] = " 0 0 4294967295";
> > > > > > + struct passwd *pw;
> > > > > > + char buf[BUFSIZ];
> > > > > > + int ret;
> > > > > > + int fd;
> > > > > > +
> > > > > > + /* If user has specified --runas, that takes precedence */
> > > > > > + if (runas) {
> > > > > > + ret = conf_runas(runas, uid, gid);
> > > > > > + if (ret)
> > > > > > + err("Invalid --runas option: %s", runas);
> > > > > > + return ret;
> > > > > > + }
> > > > > > +
> > > > > > + /* Otherwise default to current user and group.. */
> > > > > > + *uid = geteuid();
> > > > > > + *gid = getegid();
> > > > > > +
> > > > > > + /* ..as long as it's not root.. */
> > > > > > + if (*uid)
> > > > > > + return 0;
> > > > > > +
> > > > > > + /* ..or at least not root in the init namespace.. */
> > > > > > + if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0)
> > > > > > + return 0;
> > > > > > +
> > > > > > + if (read(fd, buf, BUFSIZ) != sizeof(root_uid_map) ||
> > > > > > + strncmp(buf, root_uid_map, sizeof(root_uid_map) - 1)) {
> > > > > > + close(fd);
> > > > > > + return 0;
> > > > > > + }
> > > > > > +
> > > > > > + close(fd);
> > > > > > +
> > > > > > + /* ..otherwise use nobody:nobody */
> > > > >
> > > > > I'd change all those comment to use ellipses (...) instead of "..".
> > > >
> > > > Ok, nit picked.
> > > >
> > > > > I think your comments here help a lot, by the way (and I couldn't find
> > > > > a better way to check for UID 0 in non-init other than that hack).
> > > >
> > > > Right. The extra complexity - both of code and of mental model - in
> > > > going from "not UID 0" to "not UID 0 in the init namespace" is what
> > > > makes me really wonder if this check is worth having.
> > >
> > > I think it's desirable for two cases (rather important in my opinion):
> > >
> > > - running passt with a further isolation implemented by a user
> > > namespace (e.g. with pasta). There it's not really practical to use a
> > > non-zero UID, and allowing to do this easily is a relevant security
> > > feature
> > >
> > > - ...if I recall correctly (but I can't check right now) Podman does
> > > the same
> >
> > Sorry, I realize I wasn't clear. We absolutely need the ability to
> > run as "root" (UID 0) within a user namespace. What I'm questioning
> > is given that whether it's worth preventing running when UID 0 outside
> > a user namespace (as far as we can tell). There's arguably some edge
> > cases where it might be useful, and it's debatable whether it's
> > passt's job to prevent the user from shooting themselves in the foot
> > in this way.
>
> Ah, I see now.
>
> Well, I also think it's debatable, and I'd even tend to say it's not
> actually passt's job, but I still think there are three reasons to keep
> doing this:
>
> - user mistakes happen, and it's also arguably our job to make usage
> less error-prone
>
> - if users run this as root, we won't actually run as root, so we
> obtain an essentially equivalent level of security while letting
> lazy/distracted users do whatever... compared to the alternative, it
> sounds appealing
>
> - given that it's debatable (for instance, many other tools and daemons
> do the same), I'd keep erring on the side of caution, as this might
> significantly decide the perceived, or factual, severity of any
> vulnerability that might be found
>
> I also guess that forcing users to rebuild from source if they want to
> do is reasonable for those edge cases.
Yeah, I guess that's fair enough.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2022-09-13 5:20 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-08 3:58 [PATCH v2 00/10] Clean up handling of userns David Gibson
2022-09-08 3:58 ` [PATCH v2 01/10] Don't store UID & GID persistently in the context structure David Gibson
2022-09-08 3:58 ` [PATCH v2 02/10] Split checking for root from dropping root privilege David Gibson
2022-09-09 14:33 ` Stefano Brivio
2022-09-10 7:09 ` David Gibson
2022-09-08 3:59 ` [PATCH v2 03/10] Consolidate determination of UID/GID to run as David Gibson
2022-09-09 14:33 ` Stefano Brivio
2022-09-10 7:15 ` David Gibson
2022-09-10 20:43 ` Stefano Brivio
2022-09-12 9:53 ` David Gibson
2022-09-13 3:49 ` Stefano Brivio
2022-09-13 5:20 ` David Gibson [this message]
2022-09-08 3:59 ` [PATCH v2 04/10] Safer handling if we can't open /proc/self/uid_map David Gibson
2022-09-09 14:33 ` Stefano Brivio
2022-09-10 7:23 ` David Gibson
2022-09-08 3:59 ` [PATCH v2 05/10] Move self-isolation code into a separate file David Gibson
2022-09-09 14:33 ` Stefano Brivio
2022-09-10 7:23 ` David Gibson
2022-09-10 20:43 ` Stefano Brivio
2022-09-08 3:59 ` [PATCH v2 06/10] Consolidate validation of pasta namespace options David Gibson
2022-09-08 3:59 ` [PATCH v2 07/10] Clean up and rename conf_ns_open() David Gibson
2022-09-08 3:59 ` [PATCH v2 08/10] Correctly handle --netns-only in pasta_start_ns() David Gibson
2022-09-09 14:34 ` Stefano Brivio
2022-09-10 7:25 ` David Gibson
2022-09-11 8:26 ` David Gibson
2022-09-13 3:50 ` Stefano Brivio
2022-09-08 3:59 ` [PATCH v2 09/10] Handle userns isolation and dropping root at the same time David Gibson
2022-09-08 3:59 ` [PATCH v2 10/10] Allow --userns when pasta spawns a command David Gibson
2022-09-09 14:34 ` Stefano Brivio
2022-09-10 7:29 ` David Gibson
2022-09-10 20:42 ` Stefano Brivio
2022-09-09 14:36 ` [PATCH v2 00/10] Clean up handling of userns Stefano Brivio
2022-09-10 7:30 ` David Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YyATMbuHZfivWZJE@yekko \
--to=david@gibson.dropbear.id.au \
--cc=passt-dev@passt.top \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).