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@canonical.com>
---
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) */
int virSecurityManagerDomainSetPathLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- const char *path);
+ const char *path,
+ bool allowSubtree);
int virSecurityManagerSetChardevLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 0815a02..c26cdac 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -3028,7 +3028,8 @@ virSecuritySELinuxGetSecurityMountOptions(virSecurityManagerPtr mgr,
static int
virSecuritySELinuxDomainSetPathLabel(virSecurityManagerPtr mgr,
virDomainDefPtr def,
- const char *path)
+ const char *path,
+ bool allowSubtree ATTRIBUTE_UNUSED)
{
virSecurityLabelDefPtr seclabel;
diff --git a/src/security/security_stack.c b/src/security/security_stack.c
index 0375e7d..9615f9f 100644
--- a/src/security/security_stack.c
+++ b/src/security/security_stack.c
@@ -704,7 +704,8 @@ virSecurityStackRestoreInputLabel(virSecurityManagerPtr mgr,
static int
virSecurityStackDomainSetPathLabel(virSecurityManagerPtr mgr,
virDomainDefPtr vm,
- const char *path)
+ const char *path,
+ bool allowSubtree)
{
virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
virSecurityStackItemPtr item = priv->itemsHead;
@@ -712,7 +713,7 @@ virSecurityStackDomainSetPathLabel(virSecurityManagerPtr mgr,
for (; item; item = item->next) {
if (virSecurityManagerDomainSetPathLabel(item->securityManager,
- vm, path) < 0)
+ vm, path, allowSubtree) < 0)
rc = -1;
}
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@canonical.com>
> ---
> 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.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jan 9, 2018 at 5:31 PM, Michal Privoznik <mprivozn@redhat.com> 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@canonical.com>
>> ---
>> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.