[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] security: Don't add seclabel of type none if there's already a seclabel



On Thu, Mar 21, 2013 at 11:46:18AM +0100, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=923946
> 
> The <seclabel type='none'/> should be added iff there is no other
> seclabel defined within a domain. This bug can be easily reproduced:
> 1) configure selinux seclabel for a domain
> 2) disable system's selinux and restart libvirtd
> 3) observe <seclabel type='none'/> being appended to a domain on its
>    startup
> ---
>  src/security/security_manager.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index c621366..26262ed 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -425,7 +425,7 @@ int virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
>                                 virDomainDefPtr vm)
>  {
>      int rc = 0;
> -    size_t i;
> +    size_t i, j, nsec_managers;
>      virSecurityManagerPtr* sec_managers = NULL;
>      virSecurityLabelDefPtr seclabel;
>  
> @@ -435,6 +435,26 @@ int virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
>      if ((sec_managers = virSecurityManagerGetNested(mgr)) == NULL)
>          return -1;
>  
> +    for (nsec_managers = 0; sec_managers[nsec_managers]; nsec_managers++)
> +        ;
> +
> +    for (i = 0; sec_managers[i]; i++) {
> +        if (STRNEQ(sec_managers[i]->drv->name, "none"))
> +            continue;
> +
> +        /* If there's a seclabel defined for a @vm other than NOP,
> +         * we don't want to define seclabel of type 'none' */
> +        for (j = 0; i < vm->nseclabels; j++) {
> +            if (vm->seclabels[j]->type == VIR_DOMAIN_SECLABEL_NONE)
> +                continue;
> +
> +            VIR_DEBUG("Skipping NOP security manager");
> +            memmove(sec_managers + i, sec_managers + i + 1,
> +                    (nsec_managers - i + 1) * sizeof(sec_managers));
> +            break;
> +        }
> +    }

I don't really like this code at all.

> +
>      virObjectLock(mgr);
>      for (i = 0; sec_managers[i]; i++) {
>          seclabel = virDomainDefGetSecurityLabelDef(vm,

IMHO the flaw is in this method - despite being a 'getter' it is
actually modifying the the domain def to add <seclabel> elements
when called. IMHO this is totally bogus behaviour that should be
removed. The only code which should be adding <seclabel> is this
security manager / driver code, not XML handling APIs.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]