[libvirt] [PATCH 1/3 trivial] fix typo in virt-aa-helper helptext
it's --dryrun not --dry-run Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com> --- src/security/virt-aa-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index a2d7226..b466626 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -107,7 +107,7 @@ vah_usage(void) " Options:\n" " -a | --add load profile\n" " -c | --create create profile from template\n" - " -d | --dry-run dry run\n" + " -d | --dryrun dry run\n" " -D | --delete unload and delete profile\n" " -f | --add-file <file> add file to profile\n" " -F | --append-file <file> append file to profile\n" -- 2.7.0
[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@suse.com> 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); + 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; } } -- 2.7.0
On Fri, 2016-03-11 at 20:07 +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.]
Well, that was so heavily changed with Jamie's comments I can't really claim paternity of anything on that patch ;) -- Cedric
From: Cèdric Bosdonnat <cbosdonnat@suse.com>
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); + 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; } }
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@suse.com>
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
Quoting Guido Günther (agx@sigxcpu.org):
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@suse.com>
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.
Good point - that should be switched over, if Jamie is ok with the patch in general. (Note I'm out this week, I can update next week or I don't mind if someone else take it over)
+ 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
[ This depends on patch 2/3, so don't cherrypick just this one :) ] Just because a disk element only requests read access doesn't mean there may not be another readwrite request. This fixes 'virsh blockcommit' which otherwise fails due to inability to write to the basefile. Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com> --- src/security/virt-aa-helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 34d08c8..2d05522 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -939,11 +939,11 @@ add_file_path(virDomainDiskDefPtr disk, if (depth == 0) { if (disk->src->readonly) - ret = vah_add_file(buf, path, "r"); + ret = vah_add_file(buf, path, "R"); else ret = vah_add_file(buf, path, "rw"); } else { - ret = vah_add_file(buf, path, "r"); + ret = vah_add_file(buf, path, "R"); } if (ret != 0) -- 2.7.0
participants (3)
-
Cedric Bosdonnat -
Guido Günther -
Serge Hallyn