[libvirt] [PATCH] Avoid segfault in virt-aa-helper when handling read-only mount filesystems

[John Ferlan]

On 08/24/2016 07:15 PM, Rufo Dogav wrote:
> This patch fixes a segfault in virt-aa-helper caused by attempting to modify a
> string literal in situ.  It is triggered when a domain has a <filesystem> with
> type='mount' configured readonly, and libvirt is using the AppArmor security
> driver for sVirt confinement.
> ---
> 
> Existing code seems to use VIR_STRDUP_QUIET for similar purposes, please
> change if VIR_STRDUP is preferred.  Needed new cleanup rather than simply
> return -1; to avoid introducing leak of tmp.
> 
>  src/security/virt-aa-helper.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 

I adjusted the commit message slightly and made the adjustments listed
below and pushed...  Congrats on your first libvirt patch.

Tks

John

> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 49e12b9..0bcf642 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -740,6 +740,7 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi
>      bool readonly = true;
>      bool explicit_deny_rule = true;
>      char *sub = NULL;
> +    char *perms_new = NULL;
>  
>      if (path == NULL)
>          return rc;
> @@ -764,12 +765,15 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi
>          return rc;
>      }
>  
> -    if (strchr(perms, 'w') != NULL) {
> +    if (VIR_STRDUP_QUIET(perms_new, perms) < 0)

I kept the _QUIET for this one... Printing of errors in this module is a
bit different than libvirt usually uses. The _QUIET version won't
virReportError the failed allocation.

In this case, if the allocation failed "eventually" the stack seems to
unwind to get_files() which returns failure to vahParseArgv which will
print a very generic vah_error().

  -
> +        goto clean_tmp;

No need to create a new label... A VIR_FREE() of something that's been
initialized to NULL doesn't do anything.  A single "cleanup" label is
something that's more consistent with other libvirt code.

> +
> +    if (strchr(perms_new, 'w') != NULL) {
>          readonly = false;
>          explicit_deny_rule = false;
>      }
>  
> -    if ((sub = strchr(perms, 'R')) != NULL) {
> +    if ((sub = strchr(perms_new, 'R')) != NULL) {
>          /* Don't write the invalid R permission, replace it with 'r' */
>          sub[0] = 'r';
>          explicit_deny_rule = false;
> @@ -787,7 +791,7 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi
>      if (tmp[strlen(tmp) - 1] == '/')
>          tmp[strlen(tmp) - 1] = '\0';
>  
> -    virBufferAsprintf(buf, "  \"%s%s\" %s,\n", tmp, recursive ? "/**" : "", perms);
> +    virBufferAsprintf(buf, "  \"%s%s\" %s,\n", tmp, recursive ? "/**" : "", perms_new);

This is a long line - I moved perms_new to it's own line


>      if (explicit_deny_rule) {
>          virBufferAddLit(buf, "  # don't audit writes to readonly files\n");
>          virBufferAsprintf(buf, "  deny \"%s%s\" w,\n", tmp, recursive ? "/**" : "");
> @@ -798,6 +802,8 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi
>      }
>  
>   cleanup:
> +    VIR_FREE(perms_new);
> + clean_tmp:
>      VIR_FREE(tmp);
>  
>      return rc;
>