[v1] apparmor: implement more domain callbacks

[libvirt] [PATCH 3/4] security, apparmor: add (Set|Restore)ChardevLabel

Posted by Christian Ehrhardt 7 years, 10 months ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/4] security, apparmor: add (Set|Restore)ChardevLabel

Posted by Michal Privoznik 7 years, 10 months ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/4] security, apparmor: add (Set|Restore)ChardevLabel

Posted by Christian Ehrhardt 7 years, 10 months ago

>> +    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!

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/4] security, apparmor: implement domainSetPathLabel
[libvirt] [PATCH 2/4] security: full path option for DomainSetPathLabel
[libvirt] [PATCH 3/4] security, apparmor: add (Set|Restore)ChardevLabel
[libvirt] [PATCH 4/4] apparmor, virt-aa-helper: drop static channel rule