| Message ID | f8c84196122adc32f9067680e54a9de32a571756.1496132443.git.mprivozn@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, May 30, 2017 at 10:23:35AM +0200, Michal Privoznik wrote: > There's a problem with the way we currently parse ACL files. The > problem is, the bridge helper has usually SUID flag set and thus > runs as root in which case all the ACL files are parsed > (/etc/qemu/bridge.conf and all other files it includes). > Therefore, if there's say bob.conf owned by root:bob and I run > the bridge helper, it doesn't really matter whether I'm in the > bob group or not. Everything that is allowed to bob group is > allowed to me too. Ok so the ACL feature relies on the qemu-bridge-helper not having privileges to read the files included from the master bridge.conf file. The easy way for this to work is if the qemu-bridge-helper binary is given a filesystem capability eg setcap cap_net_admin=ep qemu-bridge-helper in this mode, the bridge helper always runs with uid=500, gid=500, so does not have permission to read included files which are group owned by a different user. If using SUID mode, the bridge helper would be given a custom group and SUID privs chown root.qemu qemu-bridge-helper chmod ug+s qemu-bridge-helper when it runs, it initially has effective uid=0 and effective gid=qemu It then uses libcapng to drop privileges and change user ID, so that it ends up with only cap_net_admin, and effective uid=500 and effective gid=500 again. So once again, it will be unable to read the included files which are group owned by a different user. IOW, I think everything is working normally. The only scenario in which you can run SUID, and permissions do not work would be if you compiled QEMU with libcapng support disabled. It would be wise to change the bridge helper to refuse to read any included files with uid != 0 or gid != 0, if libcapng support is disabled, so make it clear that permission checking is inoperative. Or better yet, just make libcapng mandatory when using the bridge helper. > The way this problem is fixed is whenever an ACL file is parsed, > the group owner of the file is stored among with the rules so > that it can be compared when evaluating. > > There is one exception though. If an ACL file is owned by root > group the rules within apply to all groups. This is because > that's the usual setup currently (the bridge.conf is usually > owned by root:root) and anybody from root group can plug the > interface themselves anyway. > > This idea of groups was introduced in bdef79a2994d6f0383 but got > broken by ditros setting SUID onto the binary. I don't believe it did, unless distros left out libcapng support when building it. Regards, Daniel
diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c index a7f9bf06cc..eab9ad5096 100644 --- a/qemu-bridge-helper.c +++ b/qemu-bridge-helper.c @@ -48,6 +48,7 @@ enum { typedef struct ACLRule { int type; + gid_t group; char iface[IFNAMSIZ]; QSIMPLEQ_ENTRY(ACLRule) entry; } ACLRule; @@ -65,8 +66,13 @@ static int parse_acl_file(const char *filename, ACLList *acl_list) FILE *f; char line[4096]; ACLRule *acl_rule; + struct stat stbuf; int ret = -1; + if (stat(filename, &stbuf) < 0) { + return -1; + } + f = fopen(filename, "r"); if (f == NULL) { return -1; @@ -117,6 +123,7 @@ static int parse_acl_file(const char *filename, ACLList *acl_list) acl_rule->type = ACL_DENY; snprintf(acl_rule->iface, IFNAMSIZ, "%s", arg); } + acl_rule->group = stbuf.st_gid; QSIMPLEQ_INSERT_TAIL(acl_list, acl_rule, entry); } else if (strcmp(cmd, "allow") == 0) { acl_rule = g_malloc(sizeof(*acl_rule)); @@ -126,6 +133,7 @@ static int parse_acl_file(const char *filename, ACLList *acl_list) acl_rule->type = ACL_ALLOW; snprintf(acl_rule->iface, IFNAMSIZ, "%s", arg); } + acl_rule->group = stbuf.st_gid; QSIMPLEQ_INSERT_TAIL(acl_list, acl_rule, entry); } else if (strcmp(cmd, "include") == 0) { /* ignore errors */ @@ -229,6 +237,7 @@ int main(int argc, char **argv) ACLRule *acl_rule; ACLList acl_list; int access_allowed, access_denied; + gid_t group; int rv, ret = EXIT_FAILURE; #ifdef CONFIG_LIBCAP @@ -275,24 +284,31 @@ int main(int argc, char **argv) */ access_allowed = 0; access_denied = 0; + group = getgid(); QSIMPLEQ_FOREACH(acl_rule, &acl_list, entry) { - switch (acl_rule->type) { - case ACL_ALLOW_ALL: - access_allowed = 1; - break; - case ACL_ALLOW: - if (strcmp(bridge, acl_rule->iface) == 0) { + /* If the acl policy file is owned by root group it + * applies to all groups. Otherwise it applies to that + * specific group. */ + if (!acl_rule->group || + (acl_rule->group && acl_rule->group == group)) { + switch (acl_rule->type) { + case ACL_ALLOW_ALL: access_allowed = 1; - } - break; - case ACL_DENY_ALL: - access_denied = 1; - break; - case ACL_DENY: - if (strcmp(bridge, acl_rule->iface) == 0) { + break; + case ACL_ALLOW: + if (strcmp(bridge, acl_rule->iface) == 0) { + access_allowed = 1; + } + break; + case ACL_DENY_ALL: access_denied = 1; + break; + case ACL_DENY: + if (strcmp(bridge, acl_rule->iface) == 0) { + access_denied = 1; + } + break; } - break; } }
There's a problem with the way we currently parse ACL files. The problem is, the bridge helper has usually SUID flag set and thus runs as root in which case all the ACL files are parsed (/etc/qemu/bridge.conf and all other files it includes). Therefore, if there's say bob.conf owned by root:bob and I run the bridge helper, it doesn't really matter whether I'm in the bob group or not. Everything that is allowed to bob group is allowed to me too. The way this problem is fixed is whenever an ACL file is parsed, the group owner of the file is stored among with the rules so that it can be compared when evaluating. There is one exception though. If an ACL file is owned by root group the rules within apply to all groups. This is because that's the usual setup currently (the bridge.conf is usually owned by root:root) and anybody from root group can plug the interface themselves anyway. This idea of groups was introduced in bdef79a2994d6f0383 but got broken by ditros setting SUID onto the binary. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- qemu-bridge-helper.c | 44 ++++++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 14 deletions(-)