[libvirt] [PATCH 0/4] apparmor: implement more domain callbacks
Based on a discussion in [1] I found that the AppArmor security module lacked some callbacks. Implementing those not only fixes the issue I had before but will also cover a few more cases I didn't even run into so far. [1]: https://www.redhat.com/archives/libvir-list/2017-December/msg00726.html Christian Ehrhardt (4): security, apparmor: implement domainSetPathLabel security: full path option for DomainSetPathLabel security, apparmor: add (Set|Restore)ChardevLabel apparmor, virt-aa-helper: drop static channel rule src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_process.c | 4 +- src/security/security_apparmor.c | 96 ++++++++++++++++++++++++++++++++++++++++ src/security/security_dac.c | 3 +- src/security/security_driver.h | 3 +- src/security/security_manager.c | 5 ++- src/security/security_manager.h | 3 +- src/security/security_selinux.c | 3 +- src/security/security_stack.c | 5 ++- src/security/virt-aa-helper.c | 2 - 10 files changed, 113 insertions(+), 13 deletions(-) -- 2.7.4
This came up in discussions around huge pages, but it will cover more per guest paths that should be added to the guests apparmor profile: - keys via qemuDomainWriteMasterKeyFile - per domain dirs via qemuProcessMakeDir - memory backing paths via qemuProcessBuildDestroyMemoryPathsImpl Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/security_apparmor.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 1db94c6..dcd6f52 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -953,6 +953,13 @@ AppArmorSetSavedStateLabel(virSecurityManagerPtr mgr, return reload_profile(mgr, def, savefile, true); } +static int +AppArmorSetPathLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + const char *path) +{ + return reload_profile(mgr, def, path, true); +} static int AppArmorRestoreSavedStateLabel(virSecurityManagerPtr mgr, @@ -1045,6 +1052,8 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainSetSavedStateLabel = AppArmorSetSavedStateLabel, .domainRestoreSavedStateLabel = AppArmorRestoreSavedStateLabel, + .domainSetPathLabel = AppArmorSetPathLabel, + .domainSetSecurityImageFDLabel = AppArmorSetFDLabel, .domainSetSecurityTapFDLabel = AppArmorSetFDLabel, -- 2.7.4
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 fullpath 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 | 3 ++- src/security/security_selinux.c | 3 ++- src/security/security_stack.c | 5 +++-- 9 files changed, 32 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 70fb406..ac3e182 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..60a8e08 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 fullpath) { - return reload_profile(mgr, def, path, true); + int rc = -1; + char *full_path = NULL; + + if (fullpath) { + 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..60c4f09 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 fullpath ATTRIBUTE_UNUSED) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityLabelDefPtr seclabel; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 47dad8b..20168a6 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 fullpath); 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..fbd4333 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 fullpath) { if (mgr->drv->domainSetPathLabel) { int ret; virObjectLock(mgr); - ret = mgr->drv->domainSetPathLabel(mgr, vm, path); + ret = mgr->drv->domainSetPathLabel(mgr, vm, path, fullpath); virObjectUnlock(mgr); return ret; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 013e3b9..4ef6bd8 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -182,7 +182,8 @@ int virSecurityManagerRestoreInputLabel(virSecurityManagerPtr mgr, int virSecurityManagerDomainSetPathLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, - const char *path); + const char *path, + bool fullpath); int virSecurityManagerSetChardevLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 0815a02..9a24b30 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 fullpath ATTRIBUTE_UNUSED) { virSecurityLabelDefPtr seclabel; diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 0375e7d..5ad4d99 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 fullpath) { 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, fullpath) < 0) rc = -1; } -- 2.7.4
On 01/03/2018 06:00 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 fullpath 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 | 3 ++- src/security/security_selinux.c | 3 ++- src/security/security_stack.c | 5 +++-- 9 files changed, 32 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 70fb406..ac3e182 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..60a8e08 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 fullpath)
@fullpath seems misleading to me. At first I though that this is absolute vs. relative path. Maybe allowSubtree is a better name? Also, I know we don't do it everywhere, but given how ambiguous this argument's name is can we have a comment describing the function and its arguments please?
{ - return reload_profile(mgr, def, path, true); + int rc = -1; + char *full_path = NULL; + + if (fullpath) { + 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);
Almost. Curly braces and else should be at one line. But then you get a syntax-check error because there's another rule saying that if one branch has curly braces the other one has to have them too. Michal
On Tue, Jan 9, 2018 at 11:02 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 01/03/2018 06:00 PM, Christian Ehrhardt wrote:
[...]
AppArmorSetPathLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, - const char *path) + const char *path, + bool fullpath)
@fullpath seems misleading to me. At first I though that this is absolute vs. relative path. Maybe allowSubtree is a better name?
Yes it is
Also, I know we don't do it everywhere, but given how ambiguous this argument's name is can we have a comment describing the function and its arguments please?
Yes reasonable, since this is implemented multiple times (by each security module) I'll add the details to the header. Otherwise I'd spread it all over the place in a redundant way which seems worse.
{ - return reload_profile(mgr, def, path, true); + int rc = -1; + char *full_path = NULL; + + if (fullpath) { + 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);
Almost. Curly braces and else should be at one line. But then you get a syntax-check error because there's another rule saying that if one branch has curly braces the other one has to have them too.
Ok same line AND curly braces for both. TL;DR ok with all suggestions - thanks for the review.
Since 1b4f66e "security: introduce virSecurityManager (Set|Restore)ChardevLabel" this is a public API of security manager. Implementing this in apparmor avoids miss any rules that should be added for devices labeled via these calls. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/security_apparmor.c | 74 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 60a8e08..e91f157 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -946,6 +946,77 @@ AppArmorRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, } static int +AppArmorSetChardevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd ATTRIBUTE_UNUSED) +{ + char *in = NULL, *out = NULL; + int ret = -1; + + virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def, + SECURITY_APPARMOR_NAME); + if (!secdef) + return 0; + + switch ((virDomainChrType) dev_source->type) { + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_FILE: + case VIR_DOMAIN_CHR_TYPE_UNIX: + case VIR_DOMAIN_CHR_TYPE_PTY: + ret = reload_profile(mgr, def, dev_source->data.file.path, true); + break; + + case VIR_DOMAIN_CHR_TYPE_PIPE: + if (virAsprintf(&in, "%s.in", dev_source->data.file.path) < 0 || + virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0) + goto done; + if (virFileExists(in)) { + if (reload_profile(mgr, def, in, true) < 0) + goto done; + } + if (virFileExists(out)) { + if (reload_profile(mgr, def, out, true) < 0) + goto done; + } + ret = reload_profile(mgr, def, dev_source->data.file.path, true); + break; + + case VIR_DOMAIN_CHR_TYPE_SPICEPORT: + case VIR_DOMAIN_CHR_TYPE_NULL: + case VIR_DOMAIN_CHR_TYPE_VC: + case VIR_DOMAIN_CHR_TYPE_STDIO: + case VIR_DOMAIN_CHR_TYPE_UDP: + case VIR_DOMAIN_CHR_TYPE_TCP: + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + case VIR_DOMAIN_CHR_TYPE_NMDM: + case VIR_DOMAIN_CHR_TYPE_LAST: + ret = 0; + break; + } + + done: + VIR_FREE(in); + VIR_FREE(out); + return ret; +} + +static int +AppArmorRestoreChardevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source ATTRIBUTE_UNUSED, + bool chardevStdioLogd ATTRIBUTE_UNUSED) +{ + virSecurityLabelDefPtr secdef = + virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); + + if (!secdef) + return 0; + + return reload_profile(mgr, def, NULL, false); +} + +static int AppArmorSetSavedStateLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, const char *savefile) @@ -1067,6 +1138,9 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainSetPathLabel = AppArmorSetPathLabel, + .domainSetSecurityChardevLabel = AppArmorSetChardevLabel, + .domainRestoreSecurityChardevLabel = AppArmorRestoreChardevLabel, + .domainSetSecurityImageFDLabel = AppArmorSetFDLabel, .domainSetSecurityTapFDLabel = AppArmorSetFDLabel, -- 2.7.4
On 01/03/2018 06:00 PM, Christian Ehrhardt wrote:
Since 1b4f66e "security: introduce virSecurityManager (Set|Restore)ChardevLabel" this is a public API of security manager.
Implementing this in apparmor avoids miss any rules that should be added for devices labeled via these calls.
I believe s/miss/missing/, s/labeled/labelled/
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/security_apparmor.c | 74 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 60a8e08..e91f157 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -946,6 +946,77 @@ AppArmorRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, }
static int +AppArmorSetChardevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd ATTRIBUTE_UNUSED) +{ + char *in = NULL, *out = NULL; + int ret = -1; + + virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def, + SECURITY_APPARMOR_NAME); + if (!secdef) + return 0;
There shouldn't be empty line in variable declaration. However, there should be one after the block. BTW: here and in the function below - if we need to call a function with a long name to initialize a variable we usually do that on a separate line. virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); if (!secdef) return 0;
+ + switch ((virDomainChrType) dev_source->type) { + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_FILE: + case VIR_DOMAIN_CHR_TYPE_UNIX: + case VIR_DOMAIN_CHR_TYPE_PTY: + ret = reload_profile(mgr, def, dev_source->data.file.path, true); + break; + + case VIR_DOMAIN_CHR_TYPE_PIPE: + if (virAsprintf(&in, "%s.in", dev_source->data.file.path) < 0 || + virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0) + goto done; + if (virFileExists(in)) { + if (reload_profile(mgr, def, in, true) < 0) + goto done; + } + if (virFileExists(out)) { + if (reload_profile(mgr, def, out, true) < 0) + goto done; + } + ret = reload_profile(mgr, def, dev_source->data.file.path, true); + break; + + case VIR_DOMAIN_CHR_TYPE_SPICEPORT: + case VIR_DOMAIN_CHR_TYPE_NULL: + case VIR_DOMAIN_CHR_TYPE_VC: + case VIR_DOMAIN_CHR_TYPE_STDIO: + case VIR_DOMAIN_CHR_TYPE_UDP: + case VIR_DOMAIN_CHR_TYPE_TCP: + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + case VIR_DOMAIN_CHR_TYPE_NMDM: + case VIR_DOMAIN_CHR_TYPE_LAST: + ret = 0; + break; + } + + done: + VIR_FREE(in); + VIR_FREE(out); + return ret; +} + +static int +AppArmorRestoreChardevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source ATTRIBUTE_UNUSED, + bool chardevStdioLogd ATTRIBUTE_UNUSED) +{ + virSecurityLabelDefPtr secdef = + virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); + + if (!secdef) + return 0; + + return reload_profile(mgr, def, NULL, false); +} + +static int AppArmorSetSavedStateLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, const char *savefile) @@ -1067,6 +1138,9 @@ virSecurityDriver virAppArmorSecurityDriver = {
.domainSetPathLabel = AppArmorSetPathLabel,
+ .domainSetSecurityChardevLabel = AppArmorSetChardevLabel, + .domainRestoreSecurityChardevLabel = AppArmorRestoreChardevLabel, + .domainSetSecurityImageFDLabel = AppArmorSetFDLabel, .domainSetSecurityTapFDLabel = AppArmorSetFDLabel,
Michal
+ char *in = NULL, *out = NULL; + int ret = -1; + + virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def, + SECURITY_APPARMOR_NAME); + if (!secdef) + return 0;
There shouldn't be empty line in variable declaration. However, there should be one after the block.
BTW: here and in the function below - if we need to call a function with a long name to initialize a variable we usually do that on a separate line.
Yes to both suggestions, thanks!
This is now covered by DomainSetPathLabel being implemented in apparmor. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 07ece73..f7ccae0 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1353,8 +1353,6 @@ main(int argc, char **argv) LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-%d-%.*s/*\" rw,\n", LOCALSTATEDIR, ctl->def->id, 20, ctl->def->name); - virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/channel/target/domain-%d-%.*s/*\" rw,\n", - LOCALSTATEDIR, ctl->def->id, 20, ctl->def->name); virBufferAsprintf(&buf, " \"%s/run/libvirt/**/%s.pid\" rwk,\n", LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"/run/libvirt/**/%s.pid\" rwk,\n", -- 2.7.4
On 01/03/2018 06:00 PM, Christian Ehrhardt wrote:
Based on a discussion in [1] I found that the AppArmor security module lacked some callbacks. Implementing those not only fixes the issue I had before but will also cover a few more cases I didn't even run into so far.
[1]: https://www.redhat.com/archives/libvir-list/2017-December/msg00726.html
Christian Ehrhardt (4): security, apparmor: implement domainSetPathLabel security: full path option for DomainSetPathLabel security, apparmor: add (Set|Restore)ChardevLabel apparmor, virt-aa-helper: drop static channel rule
src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_process.c | 4 +- src/security/security_apparmor.c | 96 ++++++++++++++++++++++++++++++++++++++++ src/security/security_dac.c | 3 +- src/security/security_driver.h | 3 +- src/security/security_manager.c | 5 ++- src/security/security_manager.h | 3 +- src/security/security_selinux.c | 3 +- src/security/security_stack.c | 5 ++- src/security/virt-aa-helper.c | 2 - 10 files changed, 113 insertions(+), 13 deletions(-)
Looking good, but I've raised some small nits. Can you take a look and possibly reply or send v2 directly? Michal
On Tue, Jan 9, 2018 at 11:02 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 01/03/2018 06:00 PM, Christian Ehrhardt wrote:
Based on a discussion in [1] I found that the AppArmor security module lacked some callbacks. Implementing those not only fixes the issue I had before but will also cover a few more cases I didn't even run into so far.
[1]: https://www.redhat.com/archives/libvir-list/2017-December/msg00726.html
Christian Ehrhardt (4): security, apparmor: implement domainSetPathLabel security: full path option for DomainSetPathLabel security, apparmor: add (Set|Restore)ChardevLabel apparmor, virt-aa-helper: drop static channel rule
src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_process.c | 4 +- src/security/security_apparmor.c | 96 ++++++++++++++++++++++++++++++++++++++++ src/security/security_dac.c | 3 +- src/security/security_driver.h | 3 +- src/security/security_manager.c | 5 ++- src/security/security_manager.h | 3 +- src/security/security_selinux.c | 3 +- src/security/security_stack.c | 5 ++- src/security/virt-aa-helper.c | 2 - 10 files changed, 113 insertions(+), 13 deletions(-)
Looking good, but I've raised some small nits. Can you take a look and possibly reply or send v2 directly?
Thanks for checking both feedbacks look good, I work on a V2 to be sent soon. If there is anything else than me implementing them I'll reply there, but from reading them once I think I'm ok with all suggested changes.
Based on a discussion in [1] I found that the AppArmor security module lacked some callbacks. Implementing those not only fixes the issue I had before but will also cover a few more cases I didn't even run into so far. [1]: https://www.redhat.com/archives/libvir-list/2017-December/msg00726.html *Updates in V2 due to feedback on V1* - variable name changes and documentation for full path option - syntax improvement in (Set|Restore)ChardevLabel Christian Ehrhardt (4): security, apparmor: implement domainSetPathLabel security: full path option for DomainSetPathLabel security, apparmor: add (Set|Restore)ChardevLabel apparmor, virt-aa-helper: drop static channel rule src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_process.c | 4 +- src/security/security_apparmor.c | 96 ++++++++++++++++++++++++++++++++++++++++ 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 ++- src/security/virt-aa-helper.c | 2 - 10 files changed, 125 insertions(+), 14 deletions(-) -- 2.7.4
This came up in discussions around huge pages, but it will cover more per guest paths that should be added to the guests apparmor profile: - keys via qemuDomainWriteMasterKeyFile - per domain dirs via qemuProcessMakeDir - memory backing paths via qemuProcessBuildDestroyMemoryPathsImpl Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/security_apparmor.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 1db94c6..dcd6f52 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -953,6 +953,13 @@ AppArmorSetSavedStateLabel(virSecurityManagerPtr mgr, return reload_profile(mgr, def, savefile, true); } +static int +AppArmorSetPathLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + const char *path) +{ + return reload_profile(mgr, def, path, true); +} static int AppArmorRestoreSavedStateLabel(virSecurityManagerPtr mgr, @@ -1045,6 +1052,8 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainSetSavedStateLabel = AppArmorSetSavedStateLabel, .domainRestoreSavedStateLabel = AppArmorRestoreSavedStateLabel, + .domainSetPathLabel = AppArmorSetPathLabel, + .domainSetSecurityImageFDLabel = AppArmorSetFDLabel, .domainSetSecurityTapFDLabel = AppArmorSetFDLabel, -- 2.7.4
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
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
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.
Since 1b4f66e "security: introduce virSecurityManager (Set|Restore)ChardevLabel" this is a public API of security manager. Implementing this in apparmor avoids miss any rules that should be added for devices labeled via these calls. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/security_apparmor.c | 74 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 432fab5..a989992 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -946,6 +946,77 @@ AppArmorRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, } static int +AppArmorSetChardevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source, + bool chardevStdioLogd ATTRIBUTE_UNUSED) +{ + char *in = NULL, *out = NULL; + int ret = -1; + virSecurityLabelDefPtr secdef; + + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); + if (!secdef) + return 0; + + switch ((virDomainChrType) dev_source->type) { + case VIR_DOMAIN_CHR_TYPE_DEV: + case VIR_DOMAIN_CHR_TYPE_FILE: + case VIR_DOMAIN_CHR_TYPE_UNIX: + case VIR_DOMAIN_CHR_TYPE_PTY: + ret = reload_profile(mgr, def, dev_source->data.file.path, true); + break; + + case VIR_DOMAIN_CHR_TYPE_PIPE: + if (virAsprintf(&in, "%s.in", dev_source->data.file.path) < 0 || + virAsprintf(&out, "%s.out", dev_source->data.file.path) < 0) + goto done; + if (virFileExists(in)) { + if (reload_profile(mgr, def, in, true) < 0) + goto done; + } + if (virFileExists(out)) { + if (reload_profile(mgr, def, out, true) < 0) + goto done; + } + ret = reload_profile(mgr, def, dev_source->data.file.path, true); + break; + + case VIR_DOMAIN_CHR_TYPE_SPICEPORT: + case VIR_DOMAIN_CHR_TYPE_NULL: + case VIR_DOMAIN_CHR_TYPE_VC: + case VIR_DOMAIN_CHR_TYPE_STDIO: + case VIR_DOMAIN_CHR_TYPE_UDP: + case VIR_DOMAIN_CHR_TYPE_TCP: + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + case VIR_DOMAIN_CHR_TYPE_NMDM: + case VIR_DOMAIN_CHR_TYPE_LAST: + ret = 0; + break; + } + + done: + VIR_FREE(in); + VIR_FREE(out); + return ret; +} + +static int +AppArmorRestoreChardevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainChrSourceDefPtr dev_source ATTRIBUTE_UNUSED, + bool chardevStdioLogd ATTRIBUTE_UNUSED) +{ + virSecurityLabelDefPtr secdef; + + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); + if (!secdef) + return 0; + + return reload_profile(mgr, def, NULL, false); +} + +static int AppArmorSetSavedStateLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, const char *savefile) @@ -1067,6 +1138,9 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainSetPathLabel = AppArmorSetPathLabel, + .domainSetSecurityChardevLabel = AppArmorSetChardevLabel, + .domainRestoreSecurityChardevLabel = AppArmorRestoreChardevLabel, + .domainSetSecurityImageFDLabel = AppArmorSetFDLabel, .domainSetSecurityTapFDLabel = AppArmorSetFDLabel, -- 2.7.4
This is now covered by DomainSetPathLabel being implemented in apparmor. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 07ece73..f7ccae0 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1353,8 +1353,6 @@ main(int argc, char **argv) LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/domain-%d-%.*s/*\" rw,\n", LOCALSTATEDIR, ctl->def->id, 20, ctl->def->name); - virBufferAsprintf(&buf, " \"%s/lib/libvirt/qemu/channel/target/domain-%d-%.*s/*\" rw,\n", - LOCALSTATEDIR, ctl->def->id, 20, ctl->def->name); virBufferAsprintf(&buf, " \"%s/run/libvirt/**/%s.pid\" rwk,\n", LOCALSTATEDIR, ctl->def->name); virBufferAsprintf(&buf, " \"/run/libvirt/**/%s.pid\" rwk,\n", -- 2.7.4
On 01/09/2018 04:04 PM, Christian Ehrhardt wrote:
Based on a discussion in [1] I found that the AppArmor security module lacked some callbacks. Implementing those not only fixes the issue I had before but will also cover a few more cases I didn't even run into so far.
[1]: https://www.redhat.com/archives/libvir-list/2017-December/msg00726.html
*Updates in V2 due to feedback on V1* - variable name changes and documentation for full path option - syntax improvement in (Set|Restore)ChardevLabel
Christian Ehrhardt (4): security, apparmor: implement domainSetPathLabel security: full path option for DomainSetPathLabel security, apparmor: add (Set|Restore)ChardevLabel apparmor, virt-aa-helper: drop static channel rule
src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_process.c | 4 +- src/security/security_apparmor.c | 96 ++++++++++++++++++++++++++++++++++++++++ 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 ++- src/security/virt-aa-helper.c | 2 - 10 files changed, 125 insertions(+), 14 deletions(-)
Usually we send v2 as new patch set and not a reply to v1. It avoids having long threads. I've fixed 2/4, ACKed and pushed. Michal
participants (2)
-
Christian Ehrhardt -
Michal Privoznik