[systemd-devel] [PATCH 1/2] Add detect_userns to detect uid/gid shifts

[Stéphane Graber]

On Thu, Jan 08, 2015 at 11:17:44PM +0100, Lennart Poettering wrote:
> On Thu, 08.01.15 14:27, Stéphane Graber (stgraber at ubuntu.com) wrote:
> 
> > This adds a new detect_userns function in virt.c which will check
> > whether systemd is running in the host user namespace (single map of all
> > available uids and gids) or is using a uid/gid map.
> > 
> > The check makes sure that uid_map and gid_map are both exactly equal to
> > the default host map (assuming 32bit uid_t) for a process running in the
> > host namespace.
> 
> Hmm, so I think detecting userns is useful, and we probably should add
> that as one of the checks to detect_continer(), as a baseline
> container implementation check or so.
> 
> However, for the OOM adjust setting I'd really keep things simple, and
> just gracefully skip over things on any kind of EPERM, not just those
> related to userns. I mean, one day selinux or appormor or some other
> MAC might want to prohibit access to that too, and we shouldn't have
> to whitelist every single one of them, given how minor and relatively
> irrelevant the oom adjustment stuff is.
> 
> Since the fix for that is trivial I now made the check for it, see git
> d5243d628624038567c576e9b69c1d775eb05a05, please test.
> 
> I'd still be interested in getting the detect_userns() stuff into
> detect_container() as a fallback in some form, hence some comments on
> that:

Looks good, I'll give it a try in a tiny bit (with your subsequent fix).

I sent a V2 of my 1/2 patch that should fix most concerns and be a
re-usable for anyone who wants to expand detect_container.

> 
> > ---
> >  src/shared/virt.c | 22 ++++++++++++++++++++++
> >  src/shared/virt.h |  1 +
> >  2 files changed, 23 insertions(+)
> > 
> > diff --git a/src/shared/virt.c b/src/shared/virt.c
> > index f10baab..3d94e1f 100644
> > --- a/src/shared/virt.c
> > +++ b/src/shared/virt.c
> > @@ -363,3 +363,25 @@ int detect_virtualization(const char **id) {
> >  
> >          return VIRTUALIZATION_NONE;
> >  }
> > +
> > +/* Detect whether we run in a uid/gid shifted namespace */
> > +int detect_userns(void) {
> > +        int r;
> > +        static const char host_id_map[] = "         0          0
> >  4294967295";
> 
> Not a fan of "static const char[]" where a #define would do too. Also,
> would prefer parsing the values with sscanf() into numbers rather than
> checking the byte-by-byte string...

V2 fixes that.

> 
> > +        char *uid_map = NULL;
> > +        char *gid_map = NULL;
> 
> Something needs to free these, consider using "_cleanup_free_" for this.

And that.

I actually had them as _cleanup_free_ originally but that got dropped
when I was testing the code outside of systemd (I forgot to re-introduce
before submitting...).

> 
> > +
> > +        /* Check if we are uid-shifted */
> > +        r = read_one_line_file("/proc/self/uid_map", &uid_map);
> > +        if (r == 0 && !streq(uid_map, host_id_map))
> > +                return 1;
> > +
> > +        /* Check if we are gid-shifted */
> > +        r = read_one_line_file("/proc/self/gid_map", &gid_map);
> > +        if (r == 0 && !streq(gid_map, host_id_map))
> > +                return 1;
> > +
> > +        /* If both uid_map and gid_map don't exist or if they both match
> > +         * the full uid/gid range, then we're not inside a user namespace */
> > +        return 0;
> > +}
> 
> Also, I think the code should return errors when read_one_line_file()
> fails, except when it gets ENOENT, since that is an indication that
> userns is simply not enabled in the kernel.

V2 doesn't have that, but that's a fair point and should indeed be done
to catch any weirdness (LSMs or severe kernel bug are the only two I can
think of for that case).

> 
> Lennart
> 
> -- 
> Lennart Poettering, Red Hat

-- 
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20150108/55bc5d09/attachment.sig>