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

[Michal Privoznik]

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 at 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