Re: [libvirt] [PATCH 2/3] Introduce readonly without explicit deny-write

On Fri, Mar 11, 2016 at 08:07:02PM +0000, Serge Hallyn wrote: > [Sorry, the Ubuntu package suggests this came from Cèdric, although > I can't quite find this patch on the mailing list. Those patches > which I did see from Cèdric did not have a Signed-off-by, so I didn't > add one for him.] > > From: Cèdric Bosdonnat <cbosdonnat(a)suse.com&gt; > > Upstream changed get_files to unconditionally be "rw" to allow audit > files to be written. Still under discussion but the proposed approach > is to have a way of saying readonly but do not implicitely create a > write deny rule. > > --- > Changelog: do not overwrite const memory, that leads to segv (serge) > --- > src/security/virt-aa-helper.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c > index b466626..34d08c8 100644 > --- a/src/security/virt-aa-helper.c > +++ b/src/security/virt-aa-helper.c > @@ -785,12 +785,19 @@ get_definition(vahControl * ctl, const char *xmlStr) > return rc; > } > > +/* > + * The permissions allowed are apparmor valid permissions and 'R'. 'R' stands > + * for read with no explicit deny rule. > + */ > static int > -vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursive) > +vah_add_path(virBufferPtr buf, const char *path, const char *inperms, bool recursive) > { > char *tmp = NULL; > int rc = -1; > bool readonly = true; > + bool explicit_deny_rule = true; > + char *perms = strdupa(inperms); This would be a newcomer, the rest of the code uses VIR_STRDUP. I'm not sure how hard this is being "enforced" at the moment.

> + char *sub = NULL; > > if (path == NULL) > return rc; > @@ -815,8 +822,16 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi > return rc; > } > > - if (strchr(perms, 'w') != NULL) > + if (strchr(perms, 'w') != NULL) { > readonly = false; > + explicit_deny_rule = false; > + } > + > + if ((sub = strchr(perms, 'R')) != NULL) { > + /* Don't write the invalid 'R' permission, replace with 'r' */ > + sub[0] = 'r'; > + explicit_deny_rule = false; > + } > > rc = valid_path(tmp, readonly); > if (rc != 0) { > @@ -831,7 +846,7 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi > tmp[strlen(tmp) - 1] = '\0'; > > virBufferAsprintf(buf, " \"%s%s\" %s,\n", tmp, recursive ? "/**" : "", perms); > - if (readonly) { > + if (explicit_deny_rule) { > virBufferAddLit(buf, " # don't audit writes to readonly files\n"); > virBufferAsprintf(buf, " deny \"%s%s\" w,\n", tmp, recursive ? "/**" : ""); > } > @@ -1130,7 +1145,7 @@ get_files(vahControl * ctl) > /* We don't need to add deny rw rules for readonly mounts, > * this can only lead to troubles when mounting / readonly. > */ > - if (vah_add_path(&buf, fs->src, "rw", true) != 0) > + if (vah_add_path(&buf, fs->src, fs->readonly ? "R" : "rw", true) != 0) > goto cleanup; > } > } > -- This otherwise looks good to me (as does the whole series) but Jamie probably should comment on it. Cheers, -- Guido