[libvirt] [PATCH v2 4/4] security_selinux: Take @privileged into account

[Cole Robinson]

On 09/10/2015 08:47 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1124841
> 
> If running in session mode it may happen that we fail to set
> correct SELinux label, but the image may still be readable to
> the qemu process. Take this into account.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/security/security_selinux.c | 126 +++++++++++++++++++++++++---------------
>  1 file changed, 78 insertions(+), 48 deletions(-)
> 

I was just poking around in this code trying to get back up to speed so I can
post patches for the ideas I proposed here:

https://www.redhat.com/archives/libvir-list/2015-April/msg01400.html

> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index c6da6b0..c2464c2 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -886,7 +886,8 @@ virSecuritySELinuxGetSecurityProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UN
>   * return 1 if labelling was not possible.  Otherwise, require a label
>   * change, and return 0 for success, -1 for failure.  */
>  static int
> -virSecuritySELinuxSetFileconHelper(const char *path, char *tcon, bool optional)
> +virSecuritySELinuxSetFileconHelper(const char *path, char *tcon,
> +                                   bool optional, bool privileged)
>  {
>      security_context_t econ;
>  
> @@ -915,7 +916,10 @@ virSecuritySELinuxSetFileconHelper(const char *path, char *tcon, bool optional)
>              virReportSystemError(setfilecon_errno,
>                                   _("unable to set security context '%s' on '%s'"),
>                                   tcon, path);
> -            if (security_getenforce() == 1)
> +            /* However, don't claim error if SELinux is in Enforcing mode and
> +             * we are running as unprivileged user and we really did see EPERM.
> +             * Otherwise we want to return error if SELinux is Enforcing. */
> +            if (security_getenforce() == 1 && (setfilecon_errno != EPERM || privileged))
>                  return -1;

Ignoring a labelling EPERM failure for readonly media for qemu:///session
seems fine to me (like the initial bug report), but this also ignores failure
for RW disk images, so it throws out main security benefit of svirt if the
source permissions are in the wrong state, which is worrying. I suggest
amending this to only skip the !privileged error if disk->readonly. Thoughts?

- Cole