Re: [libvirt] [PATCH 4/3] Control LXC capabilities

Quoting Daniel P. Berrange (berrange(a)redhat.com): > This patch updates the LXC driver to make use of libcap-ng for managing > process capabilities. Previously Ryota Ozaki had provided code to remove > the CAP_BOOT capabilities inside the container, preventing host reboots. > In addition to that one, I believe we should be removing ability to > load kernel modules, change the system clock and changing audit/MAC. > So this patch also clears the following: > > CAP_SYS_MODULE, /* No kernel module loading */ > CAP_SYS_TIME, /* No changing the clock */ > CAP_AUDIT_CONTROL, /* No messing with auditing */ > CAP_AUDIT_WRITE, /* No messing with auditing */ > CAP_MAC_ADMIN, /* No messing with LSM */ > CAP_MAC_OVERRIDE, /* No messing with LSM */ >

Thanks, Daniel, this does look good. I wonder whether there is a more appropriate list to email caps-related patches (including libcap-ng itself) to. Not only does the code itself warrant discussion (for instance, should capng_lock() also set CAP_NOSUID_FIXUP?), but such discussions, in one place, about converting several programs to dropping capabilities would help others to do the same with this code. I can't think of anything other than the LSM list, so I'm cc:ing it here. > We use libcap-ng's capng_updatev/apply functions to remove these from > the permitted, inheritable, effective and bounding sets. Then we use > capng_lock to set NOROOT and NOROOT_LOCKED in the process securebits > to prevent them ever being re-acquired. > > The other thing I realized is that the 'libvirt_lxc' controller process > does not need to keep any capabilities at all once it has spawned the > container process, since all its doing is forwarding I/O between 2 open > file descripts. So I also clear all capabilities from that. We should > probably make it chuid/gid to a non-root user in future too. > > lxc_container.c | 66 +++++++++++++++++++++++++++++++++++++------------------ > lxc_controller.c | 28 +++++++++++++++++++++++ > 2 files changed, 73 insertions(+), 21 deletions(-) > > > Regards, > Daniel > > diff -r 7e766489c4a2 src/lxc_container.c > --- a/src/lxc_container.c Tue Jun 23 11:13:45 2009 +0100 > +++ b/src/lxc_container.c Tue Jun 23 11:54:10 2009 +0100 > @@ -41,8 +41,9 @@ > /* For MS_MOVE */ > #include <linux/fs.h> > > -#include <sys/prctl.h> > -#include <linux/capability.h> > +#if HAVE_CAPNG > +#include <cap-ng.h> > +#endif > > #include "virterror_internal.h" > #include "logging.h" > @@ -642,27 +643,50 @@ static int lxcContainerSetupMounts(virDo > return lxcContainerSetupExtraMounts(vmDef); > } > > -static int lxcContainerDropCapabilities(virDomainDefPtr vmDef ATTRIBUTE_UNUSED) > + > +/* > + * This is running as the 'init' process insid the container. > + * It removes some capabilities that could be dangerous to > + * host system, since they are not currently "containerized" > + */ > +static int lxcContainerDropCapabilities(void) > { > -#ifdef PR_CAPBSET_DROP > - int i; > - const struct { > - int id; > - const char *name; > - } caps[] = { > -#define ID_STRING(name) name, #name > - { ID_STRING(CAP_SYS_BOOT) }, > - }; > +#if HAVE_CAPNG > + int ret; > > - for (i = 0 ; i < ARRAY_CARDINALITY(caps) ; i++) { > - if (prctl(PR_CAPBSET_DROP, caps[i].id, 0, 0, 0)) { > - lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR, > - _("failed to drop %s"), caps[i].name); > - return -1; > - } > + capng_get_caps_process(); > + > + if ((ret = capng_updatev(CAPNG_DROP, > + CAPNG_EFFECTIVE | CAPNG_PERMITTED | > + CAPNG_INHERITABLE | CAPNG_BOUNDING_SET, > + CAP_SYS_BOOT, /* No use of reboot */ > + CAP_SYS_MODULE, /* No kernel module loading */ > + CAP_SYS_TIME, /* No changing the clock */ > + CAP_AUDIT_CONTROL, /* No messing with auditing */ > + CAP_AUDIT_WRITE, /* No messing with auditing */ > + CAP_MAC_ADMIN, /* No messing with LSM */ > + CAP_MAC_OVERRIDE, /* No messing with LSM */ > + -1 /* sentinal */)) < 0) { > + lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR, > + _("failed to remove capabilities %d"), ret); > + return -1; > } > -#else /* ! PR_CAPBSET_DROP */ > - VIR_WARN0(_("failed to drop capabilities PR_CAPBSET_DROP undefined")); > + > + if ((ret = capng_apply(CAPNG_SELECT_BOTH)) < 0) { > + lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR, > + _("failed to apply capabilities: %d"), ret); > + return -1; > + } > The only problem offhand with this idiom is that you need CAP_SETPCAP to set securebits and drop caps from bounding set, but I think a lot of applications could stand to drop CAP_SETPCAP otherwise. So I don't know if it would help to do the capng_lock() before capng_apply(). (To be clear, not bc you need to do so right here, but because others may well look at your code as example code.) > + /* Need to prevent them regaining any caps on exec */ > + if ((ret = capng_lock()) < 0) { > + lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR, > + _("failed to lock capabilities: %d"), ret); > + return -1; > + } > + > +#else > + VIR_WARN0(_("libcap-ng support not compiled in, unable to clear capabilities")); > #endif > return 0; > } > @@ -735,7 +759,7 @@ static int lxcContainerChild( void *data > return -1; > > /* drop a set of root capabilities */ > - if (lxcContainerDropCapabilities(vmDef) < 0) > + if (lxcContainerDropCapabilities() < 0) > return -1; > > /* this function will only return if an error occured */ > diff -r 7e766489c4a2 src/lxc_controller.c > --- a/src/lxc_controller.c Tue Jun 23 11:13:45 2009 +0100 > +++ b/src/lxc_controller.c Tue Jun 23 11:54:10 2009 +0100 > @@ -35,6 +35,10 @@ > #include <getopt.h> > #include <sys/mount.h> > > +#if HAVE_CAPNG > +#include <cap-ng.h> > +#endif > + > #include "virterror_internal.h" > #include "logging.h" > #include "util.h" > @@ -210,6 +214,25 @@ cleanup: > return rc; > } > > + > +static int lxcControllerClearCapabilities(void) > +{ > +#if HAVE_CAPNG > + int ret; > + > + capng_clear(CAPNG_SELECT_BOTH); > + > + if ((ret = capng_apply(CAPNG_SELECT_BOTH)) < 0) { > + lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR, > + _("failed to apply capabilities: %d"), ret); > + return -1; > + } > +#else > + VIR_WARN0(_("libcap-ng support not compiled in, unable to clear capabilities")); > +#endif > + return 0; > +} > + > typedef struct _lxcTtyForwardFd_t { > int fd; > int active; > @@ -562,6 +585,11 @@ lxcControllerRun(virDomainDefPtr def, > if (lxcContainerSendContinue(control[0]) < 0) > goto cleanup; > > + /* Now the container is running, there's no need for us to keep > + any elevated capabilities */ > + if (lxcControllerClearCapabilities() < 0) > + goto cleanup; > + > rc = lxcControllerMain(monitor, client, appPty, containerPty); > > cleanup: > > > -- > |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| > |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| > > -- > Libvir-list mailing list > Libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html