[libvirt] [PATCH v3 11/18] security_selinux: Remember old labels

[John Ferlan]

On 12/12/18 7:40 AM, Michal Privoznik wrote:
> Similarly to what I did in DAC driver, this also requires the
> same SELinux label to be used for shared paths. If a path is
> already in use by a domain (or domains) then and the domain we
> are starting now wants to access the path it has to have the same
> SELinux label. This might look too restrictive as the new label
> can still guarantee access to already running domains but in
> reality it is very unlikely and usually an admin mistake.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/security/security_selinux.c | 177 +++++++++++++++++++++++---------
>  1 file changed, 130 insertions(+), 47 deletions(-)
> 

[...]

>  static int
> @@ -1362,7 +1429,8 @@ getContext(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
>   * errors that the caller(s) are already dealing with */
>  static int
>  virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr,
> -                                   const char *path)
> +                                   const char *path,
> +                                   bool recall)
>  {
>      bool privileged = virSecurityManagerGetPrivileged(mgr);
>      struct stat buf;
> @@ -1386,26 +1454,35 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr,
>          goto cleanup;
>      }
>  
> -    if (stat(newpath, &buf) != 0) {
> -        VIR_WARN("cannot stat %s: %s", newpath,
> -                 virStrerror(errno, ebuf, sizeof(ebuf)));
> -        goto cleanup;
> -    }
> -
> -    if (getContext(mgr, newpath, buf.st_mode, &fcon) < 0) {
> -        /* Any user created path likely does not have a default label,
> -         * which makes this an expected non error
> -         */
> -        VIR_WARN("cannot lookup default selinux label for %s", newpath);
> -        ret = 0;
> -        goto cleanup;
> -    }
> -
> -    if ((rc = virSecuritySELinuxTransactionAppend(path, fcon, false, true)) < 0)
> +    if ((rc = virSecuritySELinuxTransactionAppend(path, NULL, false, true)) < 0)
>          return -1;
>      else if (rc > 0)
>          return 0;
>  

Since you've touched the code, Coverity looks again and determines that
@newpath can be leaked above

John

> +    if (recall) {
> +        if ((rc = virSecuritySELinuxRecallLabel(newpath, &fcon)) < 0) {
> +            goto cleanup;
> +        } else if (rc > 0) {
> +            ret = 0;
> +            goto cleanup;
> +        }
> +    } else {
> +        if (stat(newpath, &buf) != 0) {
> +            VIR_WARN("cannot stat %s: %s", newpath,
> +                     virStrerror(errno, ebuf, sizeof(ebuf)));
> +            goto cleanup;
> +        }
> +
> +        if (getContext(mgr, newpath, buf.st_mode, &fcon) < 0) {
> +            /* Any user created path likely does not have a default label,
> +             * which makes this an expected non error
> +             */
> +            VIR_WARN("cannot lookup default selinux label for %s", newpath);
> +            ret = 0;
> +            goto cleanup;
> +        }
> +    }
> +

[...]