From patchwork Tue May 30 08:23:35 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michal Privoznik X-Patchwork-Id: 9754059 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 71349602BF for ; Tue, 30 May 2017 08:32:05 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 61B54269DA for ; Tue, 30 May 2017 08:32:05 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 54E8227CEA; Tue, 30 May 2017 08:32:05 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id E382D269DA for ; Tue, 30 May 2017 08:32:04 +0000 (UTC) Received: from localhost ([::1]:52167 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dFcZP-0004QT-UY for patchwork-qemu-devel@patchwork.kernel.org; Tue, 30 May 2017 04:32:04 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50700) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dFcRK-0006VE-6w for qemu-devel@nongnu.org; Tue, 30 May 2017 04:23:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dFcRI-0001Pt-Qx for qemu-devel@nongnu.org; Tue, 30 May 2017 04:23:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44092) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dFcRI-0001P4-IZ for qemu-devel@nongnu.org; Tue, 30 May 2017 04:23:40 -0400 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9F1B78123E for ; Tue, 30 May 2017 08:23:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9F1B78123E Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=mprivozn@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 9F1B78123E Received: from moe.brq.redhat.com (dhcp129-131.brq.redhat.com [10.34.129.131]) by smtp.corp.redhat.com (Postfix) with ESMTP id 23D4C17151 for ; Tue, 30 May 2017 08:23:38 +0000 (UTC) From: Michal Privoznik To: qemu-devel@nongnu.org Date: Tue, 30 May 2017 10:23:35 +0200 Message-Id: In-Reply-To: References: In-Reply-To: References: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Tue, 30 May 2017 08:23:39 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH 3/3] qemu-bridge-helper: Take ACL file gid into account X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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 --- qemu-bridge-helper.c | 44 ++++++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 14 deletions(-) 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; } }