On 08/23/2016 08:02 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. ---

Thanks for the advice Martin - I completely overlooked the free.

src/security/virt-aa-helper.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 49e12b9..b385d8c 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 = strdup(perms);

VIR_STRDUP Then what happens if perms_new == NULL? (eg, failure to strdup). Typically this is done via: if (VIR_STRDUP(perms_new, perms) < 0)) return -1 The other problem with what you've modified is that there's a number of places after allocation where return 0 or return rc are done, but the memory isn't free'd...

if (path == NULL) return rc; @@ -764,12 +765,12 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi return rc; }

Suggestion to add your VIR_STRDUP here since after here failure does the goto cleanup. John

- if (strchr(perms, 'w') != NULL) { + 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 +788,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); if (explicit_deny_rule) { virBufferAddLit(buf, " # don't audit writes to readonly files\n"); virBufferAsprintf(buf, " deny \"%s%s\" w,\n", tmp, recursive ? "/**" : ""); @@ -799,6 +800,7 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi

cleanup: VIR_FREE(tmp); + VIR_FREE(perms_new);

return rc; }

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;