Re: [PATCH 1/6] security: Add check for valid security model

[Ján Tomko <jtomko@xxxxxxxxxx>]

On Fri, Feb 06, 2015 at 07:13:23PM +0100, Erik Skultety wrote:
> We do have a check for valid per-domain security model, however we still
> do permit an invalid security model for a <disk> type device.
> This patch introduces a new function virSecurityStackCheckDiskLabels
> which compares user specified security model against currently
> registered security drivers. That being said, it also permits 'none'
> being specified as a device security model.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1165485
> ---
>  src/security/security_manager.c |  5 +++++
>  src/security/security_manager.h |  1 +
>  src/security/security_stack.c   | 48 +++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index 302f54d..8eacf0c 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -262,6 +262,11 @@ virSecurityManagerGetDriver(virSecurityManagerPtr mgr)
>      return mgr->virtDriver;
>  }
>  
> +const char *
> +virSecurityManagerGetDriverName(virSecurityManagerPtr mgr)
> +{
> +    return mgr->drv->name;
> +}
>  
>  const char *
>  virSecurityManagerGetDOI(virSecurityManagerPtr mgr)
> diff --git a/src/security/security_manager.h b/src/security/security_manager.h
> index 156f882..4626c4b 100644
> --- a/src/security/security_manager.h
> +++ b/src/security/security_manager.h
> @@ -70,6 +70,7 @@ void virSecurityManagerPostFork(virSecurityManagerPtr mgr);
>  void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr);
>  
>  const char *virSecurityManagerGetDriver(virSecurityManagerPtr mgr);
> +const char *virSecurityManagerGetDriverName(virSecurityManagerPtr mgr);
>  const char *virSecurityManagerGetDOI(virSecurityManagerPtr mgr);
>  const char *virSecurityManagerGetModel(virSecurityManagerPtr mgr);
>  const char *virSecurityManagerGetBaseLabel(virSecurityManagerPtr mgr, int virtType);
> diff --git a/src/security/security_stack.c b/src/security/security_stack.c
> index 1ded57b..75d8b96 100644
> --- a/src/security/security_stack.c
> +++ b/src/security/security_stack.c
> @@ -301,7 +301,46 @@ virSecurityStackRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr,
>      return rc;
>  }
>  
> +static int
> +virSecurityStackCheckSecurityDiskLabels(virSecurityManagerPtr mgr,
> +                                        virDomainDefPtr vm)
> +{
> +    virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> +    virSecurityStackItemPtr item = priv->itemsHead;
> +    size_t i, j;
> +    bool sec_model_valid = false;

The function would be nicer without this variable, assuming success by
default and returning an error right away if we did not find a driver.

> +
> +    for (i = 0; i < vm->ndisks; i++) {

Character devices can have seclabels too, see
virDomainChrSourceDefFormat.

> +        virStorageSourcePtr src = vm->disks[i]->src;
> +        for (j = 0; j < src->nseclabels; j++) {
> +            const char *sec_model = src->seclabels[j]->model;
> +
> +            if (STREQ_NULLABLE(sec_model, "none")) {
> +                sec_model_valid = true;

This value is never read again.

> +                continue;
> +            }
> +
> +            sec_model_valid = false;
> +            for (; item; item = item->next) {
> +                const char *drv_name = virSecurityManagerGetDriverName(mgr);
> +
> +                if (STREQ_NULLABLE(sec_model, drv_name)) {
> +                    sec_model_valid = true;
> +                    break;
> +                }
> +            }

virSecurityDriverLookup can be used here, after we've special-cased the
"none" driver.

> +            if (!sec_model_valid) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("No security driver matches security model "
> +                                 "'%s'"),
> +                               sec_model);
> +                return -1;
> +            }
> +        }
> +    }
>  
> +    return 0;
> +}
>  static int
>  virSecurityStackSetSecurityAllLabel(virSecurityManagerPtr mgr,
>                                      virDomainDefPtr vm,
> @@ -309,13 +348,18 @@ virSecurityStackSetSecurityAllLabel(virSecurityManagerPtr mgr,
>  {
>      virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr);
>      virSecurityStackItemPtr item = priv->itemsHead;
> -    int rc = 0;
> +    int rc = -1;
> +
> +    if (virSecurityStackCheckSecurityDiskLabels(mgr, vm) < 0)
> +        goto error;

Putting this check into the SecurityManager would make it work for
drivers not using the stack (LXC).

In QEMU driver, this function (SetSecurityAllLabel) is called after the fork, but
we can do the check much earlier - maybe in virSecurityManagerGenLabel
where we already check the per-domain labels.

Jan

Attachment: signature.asc
Description: Digital signature

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