[CFT][PATCH 00/10] Making new mounts of proc and sysfs as safe as bind mounts (take 2)

Serge E. Hallyn serge at hallyn.com
Thu May 28 21:52:14 UTC 2015


On Thu, May 28, 2015 at 04:42:34PM -0500, Eric W. Biederman wrote:
> "Serge E. Hallyn" <serge at hallyn.com> writes:
> 
> > On Thu, May 28, 2015 at 10:03:28AM -0500, Eric W. Biederman wrote:
> >> Serge Hallyn <serge.hallyn at ubuntu.com> writes:
> >> 
> >> > Quoting Andy Lutomirski (luto at amacapital.net):
> >> >> On Fri, May 22, 2015 at 10:39 AM, Eric W. Biederman
> >> >> <ebiederm at xmission.com> wrote:
> >> >> > I had hoped to get some Tested-By's on that patch series.
> >> >> 
> >> >> Sorry, I've been totally swamped.
> >> >> 
> >> >> I suspect that Sandstorm is okay, but I haven't had a chance to test
> >> >> it for real.  Sandstorm makes only limited use of proc and sysfs in
> >> >> containers, but I'll see if I can test it for real this weekend.
> >> >
> >> > Testing this with unprivileged containers, I get
> >> >
> >> > lxc-start: conf.c: lxc_mount_auto_mounts: 808 Operation not permitted
> >> > - error mounting sysfs on
> >> > /usr/lib/x86_64-linux-gnu/lxc/sys/devices/virtual/net flags 0
> >> 
> >> Grr..  I was afraid this would break something. :(
> >> 
> >> Looking at my system I see that sysfs is currently mounted
> >> "nosuid,nodev,noexec"
> >> 
> >> Looking at the lxc-start code I don't see it as including any of those
> >> mount options.  In practice for sysfs I think those options are
> >> meaningless (as there should be no devices and nothing executable in
> >> sysfs) but I can understand the past concerns with chmod on virtual
> >> filesystems that would incline people to use them, so I think the
> >> failure is reporting a legitimate security issue in the lxc userspace
> >> code where the the unprivileged code is currently attempting to give
> >> greater access to sysfs than was given by the original mount of sysfs.
> >> 
> >> As nosuid,nodev,noexec should not impair the operation of sysfs
> >> operation it looks like you can always specify those options and just
> >> make this concern go away.
> >> 
> >> Something like the untested patch below I expect.
> >> 
> >> diff --git a/src/lxc/conf.c b/src/lxc/conf.c
> >> index 9870455b3cae..d9ccd03afe68 100644
> >> --- a/src/lxc/conf.c
> >> +++ b/src/lxc/conf.c
> >> @@ -770,8 +770,8 @@ static int lxc_mount_auto_mounts(struct lxc_conf *conf, int flags, struct lxc_ha
> >>  		{ LXC_AUTO_PROC_MASK, LXC_AUTO_PROC_MIXED, "%r/proc/sysrq-trigger",                             "%r/proc/sysrq-trigger",        NULL,       MS_BIND,                        NULL },
> >>  		{ LXC_AUTO_PROC_MASK, LXC_AUTO_PROC_MIXED, NULL,                                                "%r/proc/sysrq-trigger",        NULL,       MS_REMOUNT|MS_BIND|MS_RDONLY,   NULL },
> >>  		{ LXC_AUTO_PROC_MASK, LXC_AUTO_PROC_RW,    "proc",                                              "%r/proc",                      "proc",     MS_NODEV|MS_NOEXEC|MS_NOSUID,   NULL },
> >> -		{ LXC_AUTO_SYS_MASK,  LXC_AUTO_SYS_RW,     "sysfs",                                             "%r/sys",                       "sysfs",    0,                              NULL },
> >> -		{ LXC_AUTO_SYS_MASK,  LXC_AUTO_SYS_RO,     "sysfs",                                             "%r/sys",                       "sysfs",    MS_RDONLY,                      NULL },
> >> +		{ LXC_AUTO_SYS_MASK,  LXC_AUTO_SYS_RW,     "sysfs",                                             "%r/sys",                       "sysfs",    MS_NODEV|MS_NOEXEC|MS_NOSUID,   NULL },
> >> +		{ LXC_AUTO_SYS_MASK,  LXC_AUTO_SYS_RO,     "sysfs",                                             "%r/sys",                       "sysfs",    MS_NODEV|MS_NOEXEC|MS_NOSUID|MS_RDONLY, NULL },
> >>  		{ LXC_AUTO_SYS_MASK,  LXC_AUTO_SYS_MIXED,  "sysfs",                                             "%r/sys",                       "sysfs",    MS_NODEV|MS_NOEXEC|MS_NOSUID,   NULL },
> >>  		{ LXC_AUTO_SYS_MASK,  LXC_AUTO_SYS_MIXED,  "%r/sys",                                            "%r/sys",                       NULL,       MS_BIND,                        NULL },
> >>  		{ LXC_AUTO_SYS_MASK,  LXC_AUTO_SYS_MIXED,  NULL,                                                "%r/sys",                       NULL,       MS_REMOUNT|MS_BIND|MS_RDONLY,   NULL },
> >
> > fwiw - the first one works, the second one does not due to an apparent
> > inability to statvfs the origin.
> 
> Good to hear.  That confirms there are no other gotchas waiting in the
> wings.
> 
> Apparently my second suggested patch is buggy due to an invalid source
> string.  The source would need to be %r/proc or %r/sysfs to use statvfs
> productively.

Right, in these cases they're only passing in "sysfs".  The first way
is more explicit anyway (though may not help some people who have a
"lxc.mount.entry = sysfs sys sysfs ro 0 0" line in their configuration
instead, so maybe we'll have to go with the second after all, d'oh.
I'll have to look into it next week)


More information about the Containers mailing list