Re: [libvirt] [PATCH v2 2/4] security: full path option for DomainSetPathLabel

On 01/09/2018 04:04 PM, Christian Ehrhardt wrote: > virSecurityManagerDomainSetPathLabel is used to make a path known > to the security modules, but today is used interchangably for > - paths to files/dirs to be accessed directly > - paths to a dir, but the access will actually be to files therein > > Depending on the security module it is important to know which of > these types it will be. > > The argument allowSubtree augments the call to the implementations of > DomainSetPathLabel that can - per security module - decide if extra > actions shall be taken. > > For now dac/selinux handle this as before, but apparmor will make > use of it to add a wildcard to the path that was passed. > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; > --- > src/qemu/qemu_domain.c | 2 +- > src/qemu/qemu_process.c | 4 ++-- > src/security/security_apparmor.c | 17 +++++++++++++++-- > src/security/security_dac.c | 3 ++- > src/security/security_driver.h | 3 ++- > src/security/security_manager.c | 5 +++-- > src/security/security_manager.h | 16 ++++++++++++++-- > src/security/security_selinux.c | 3 ++- > src/security/security_stack.c | 5 +++-- > 9 files changed, 44 insertions(+), 14 deletions(-) > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 0f4c422..5c171e4 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -692,7 +692,7 @@ qemuDomainWriteMasterKeyFile(virQEMUDriverPtr driver, > } > > if (qemuSecurityDomainSetPathLabel(driver->securityManager, > - vm->def, path) < 0) > + vm->def, path, false) < 0) > goto cleanup; > > ret = 0; > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index a0f430f..1a0923a 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -3401,7 +3401,7 @@ qemuProcessBuildDestroyMemoryPathsImpl(virQEMUDriverPtr driver, > } > > if (qemuSecurityDomainSetPathLabel(driver->securityManager, > - def, path) < 0) { > + def, path, true) < 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("Unable to label %s"), path); > return -1; > @@ -4514,7 +4514,7 @@ qemuProcessMakeDir(virQEMUDriverPtr driver, > } > > if (qemuSecurityDomainSetPathLabel(driver->securityManager, > - vm->def, path) < 0) > + vm->def, path, true) < 0) > goto cleanup; > > ret = 0; > diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c > index dcd6f52..432fab5 100644 > --- a/src/security/security_apparmor.c > +++ b/src/security/security_apparmor.c > @@ -956,9 +956,22 @@ AppArmorSetSavedStateLabel(virSecurityManagerPtr mgr, > static int > AppArmorSetPathLabel(virSecurityManagerPtr mgr, > virDomainDefPtr def, > - const char *path) > + const char *path, > + bool allowSubtree) > { > - return reload_profile(mgr, def, path, true); > + int rc = -1; > + char *full_path = NULL; > + > + if (allowSubtree) { > + if (virAsprintf(&full_path, "%s/{,**}", path) < 0) > + return -1; > + rc = reload_profile(mgr, def, full_path, true); > + VIR_FREE(full_path); > + } else { > + rc = reload_profile(mgr, def, path, true); > + } > + > + return rc; > } > > static int > diff --git a/src/security/security_dac.c b/src/security/security_dac.c > index 609d259..74446d6 100644 > --- a/src/security/security_dac.c > +++ b/src/security/security_dac.c > @@ -2081,7 +2081,8 @@ virSecurityDACGetBaseLabel(virSecurityManagerPtr mgr, > static int > virSecurityDACDomainSetPathLabel(virSecurityManagerPtr mgr, > virDomainDefPtr def, > - const char *path) > + const char *path, > + bool allowSubtree ATTRIBUTE_UNUSED) > { > virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); > virSecurityLabelDefPtr seclabel; > diff --git a/src/security/security_driver.h b/src/security/security_driver.h > index 47dad8b..95e7c4d 100644 > --- a/src/security/security_driver.h > +++ b/src/security/security_driver.h > @@ -139,7 +139,8 @@ typedef int (*virSecurityDomainRestoreInputLabel) (virSecurityManagerPtr mgr, > virDomainInputDefPtr input); > typedef int (*virSecurityDomainSetPathLabel) (virSecurityManagerPtr mgr, > virDomainDefPtr def, > - const char *path); > + const char *path, > + bool allowSubtree); > typedef int (*virSecurityDomainSetChardevLabel) (virSecurityManagerPtr mgr, > virDomainDefPtr def, > virDomainChrSourceDefPtr dev_source, > diff --git a/src/security/security_manager.c b/src/security/security_manager.c > index 9249aba..4e80409 100644 > --- a/src/security/security_manager.c > +++ b/src/security/security_manager.c > @@ -1048,12 +1048,13 @@ virSecurityManagerGetNested(virSecurityManagerPtr mgr) > int > virSecurityManagerDomainSetPathLabel(virSecurityManagerPtr mgr, > virDomainDefPtr vm, > - const char *path) > + const char *path, > + bool allowSubtree) > { > if (mgr->drv->domainSetPathLabel) { > int ret; > virObjectLock(mgr); > - ret = mgr->drv->domainSetPathLabel(mgr, vm, path); > + ret = mgr->drv->domainSetPathLabel(mgr, vm, path, allowSubtree); > virObjectUnlock(mgr); > return ret; > } > diff --git a/src/security/security_manager.h b/src/security/security_manager.h > index 013e3b9..e1475b6 100644 > --- a/src/security/security_manager.h > +++ b/src/security/security_manager.h > @@ -179,10 +179,22 @@ int virSecurityManagerRestoreInputLabel(virSecurityManagerPtr mgr, > virDomainDefPtr vm, > virDomainInputDefPtr input); > > - > +/** > + * virSecurityManagerDomainSetPathLabel > + * @mgr: Storage file to chown > + * @vm: target uid > + * @path: string describing the path > + * @allowSubtree: > + * > + * set @allowSubtree to true if the call is not only meant for the actual path > + * in @path, but instead to also allow access to all potential subtress. > + * Example on @path = "/path": > + * False => /path > + * True => /path but also /path/... (including all deeper levels) */ Usually we put the description into .c rather than .h. Also, the description of the arguments looks a bit off. @mgr is not a 'storage file to chown' ;-). I'm fixing this and moving it to security_manager.c just above the function.