On Tue, Jan 9, 2018 at 5:31 PM, Michal Privoznik <mprivozn@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxxx>
>> ---
>> 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.
I might have been too much in a hurry, but wanted to get V2 to you
before my meeting-mania started :-/
I beg your pardon and thank you twice for cleaning it up on commit.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
