Hi, On Mon, Jan 18, 2016 at 01:45:08PM +0100, Cédric Bosdonnat wrote:

Better fix replacing c726af2d: introducing an 'R' permission to add read rule, but no explicit deny write rule. --- src/security/virt-aa-helper.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a2d7226..d0c5246 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -785,12 +785,18 @@ 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) { char *tmp = NULL; int rc = -1; bool readonly = true; + bool explicit_deny_rule = true; + char *sub = NULL;

if (path == NULL) return rc; @@ -815,8 +821,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 it with 'r' */ + sub[0] = 'r'; + explicit_deny_rule = false; + }

rc = valid_path(tmp, readonly); if (rc != 0) { @@ -831,7 +845,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 +1144,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 looks good to me but it would be great to have Jamie's input on that. Ceers, -- Guido