From patchwork Sat Jan 13 12:15:51 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Fedor Pchelkin X-Patchwork-Id: 13518976 Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.84]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 23AF2210E6; Sat, 13 Jan 2024 12:16:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ispras.ru Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ispras.ru Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ispras.ru header.i=@ispras.ru header.b="IcjNV4gE" Received: from localhost.ispras.ru (unknown [10.10.165.8]) by mail.ispras.ru (Postfix) with ESMTPSA id 2D59540755C7; Sat, 13 Jan 2024 12:16:12 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 mail.ispras.ru 2D59540755C7 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ispras.ru; s=default; t=1705148172; bh=EFLq5SwB1lpeS8irlCEfCM3yljqBF66A4Htq4s8Jbwk=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=IcjNV4gEYD4c1PhlXXPgCA9F+AI/IRDzgOkqCAgb01Zj4HpQDzQJrOvTZMNoc0V8z FkicwgCCKq4hgE1/er5sUzNdZ7Ww4bSfRbJUyDoOqkkyErpZG2bYPW1XmnhiIAvzOM 1bUJQ/OhjzOP/NMKEnXlKthB2AUPRm1/Beh3qqfQ= From: Fedor Pchelkin To: John Johansen Cc: Fedor Pchelkin , Paul Moore , James Morris , "Serge E. Hallyn" , apparmor@lists.ubuntu.com, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, Alexey Khoroshilov , lvc-project@linuxtesting.org Subject: [PATCH 1/2] apparmor: rename the data start flag inside verify_header Date: Sat, 13 Jan 2024 15:15:51 +0300 Message-ID: <20240113121556.12948-2-pchelkin@ispras.ru> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240113121556.12948-1-pchelkin@ispras.ru> References: <20240113121556.12948-1-pchelkin@ispras.ru> Precedence: bulk X-Mailing-List: linux-security-module@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 The current `required` flag indicates the packed data start thus requiring the header to be unpacked at this position. For the purposes of verify_header() refinement, rename that flag to `start` so that it can be used with this meaning in other part of the function. Found by Linux Verification Center (linuxtesting.org). Signed-off-by: Fedor Pchelkin --- security/apparmor/policy_unpack.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index a91b30100b77..54f7b4346506 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -1111,12 +1111,12 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name) /** * verify_header - unpack serialized stream header * @e: serialized data read head (NOT NULL) - * @required: whether the header is required or optional + * @start: whether the header is located at the start of data * @ns: Returns - namespace if one is specified else NULL (NOT NULL) * * Returns: error or 0 if header is good */ -static int verify_header(struct aa_ext *e, int required, const char **ns) +static int verify_header(struct aa_ext *e, int start, const char **ns) { int error = -EPROTONOSUPPORT; const char *name = NULL; @@ -1124,7 +1124,8 @@ static int verify_header(struct aa_ext *e, int required, const char **ns) /* get the interface version */ if (!aa_unpack_u32(e, &e->version, "version")) { - if (required) { + /* the header is required at the start of data */ + if (start) { audit_iface(NULL, NULL, NULL, "invalid profile format", e, error); return error; From patchwork Sat Jan 13 12:15:52 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Fedor Pchelkin X-Patchwork-Id: 13518977 Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.84]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0DE3220DF7; Sat, 13 Jan 2024 12:16:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ispras.ru Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ispras.ru Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ispras.ru header.i=@ispras.ru header.b="rBdODkxM" Received: from localhost.ispras.ru (unknown [10.10.165.8]) by mail.ispras.ru (Postfix) with ESMTPSA id E84F740762E4; Sat, 13 Jan 2024 12:16:14 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 mail.ispras.ru E84F740762E4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ispras.ru; s=default; t=1705148175; bh=UXsO+FATYJuTPtjVESRdxfPqcMfgJ+Z5TRk9Ig/cU7s=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=rBdODkxMg1L5H2zTmGYLSPw2S0CURDXH1HU32selNRUiQC7bDtQiED74SYYfHrq+G 6qL+D97qMSSKzxcWDtVNeZCWKugYZ+eB4jMUvCaDc3TjBixl3RikLhIYO4YlSvIZt6 weSxaEwRWSQ3XwZdgAJK0FCQ2VOZhkjGb8zdm3yA= From: Fedor Pchelkin To: John Johansen Cc: Fedor Pchelkin , Paul Moore , James Morris , "Serge E. Hallyn" , apparmor@lists.ubuntu.com, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, Alexey Khoroshilov , lvc-project@linuxtesting.org Subject: [PATCH 2/2] apparmor: fix namespace check in serialized stream headers from the same policy load Date: Sat, 13 Jan 2024 15:15:52 +0300 Message-ID: <20240113121556.12948-3-pchelkin@ispras.ru> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240113121556.12948-1-pchelkin@ispras.ru> References: <20240113121556.12948-1-pchelkin@ispras.ru> Precedence: bulk X-Mailing-List: linux-security-module@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Per commit 04dc715e24d0 ("apparmor: audit policy ns specified in policy load"), profiles in a single load must specify the same namespace. It is supposed to be detected inside aa_replace_profiles() and verify_header(), but seems not to be implemented correctly in the second function. Consider we have the following profile load profile :ns1:profile1 ... {} profile :ns2:profile2 ... {} The profiles specify different policy namespaces but if they are loaded as a single binary where the serialized stream headers contain different namespace values, it eventually leads to the profiles specified with different namespaces be included into the same one without any specific indication to user. *ns is assigned NULL in verify_header(), and the branch indicating an "invalid ns change" message is a dead code. Moreover, some of *ns strings is leaked as they are allocated every time verify_header() reads a namespace string. Actually, *ns is already assigned NULL in aa_unpack(), before the multiple profiles unpacking loop. So make verify_header() check if every new unpacked namespace declaration differs from the previous one in the same load. Note that similar to the namespace check in aa_replace_profiles(), if one profile in the load specifies some namespace declaration in the header and the other one doesn't specify any namespace in the header - it is also considered invalid, e.g. the following multiple profiles load will fail after this patch profile profile1 ... {} profile :ns:profile2 ... {} Found by Linux Verification Center (linuxtesting.org). Fixes: dd51c8485763 ("apparmor: provide base for multiple profiles to be replaced at once") Signed-off-by: Fedor Pchelkin --- security/apparmor/policy_unpack.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 54f7b4346506..5bd8ab7f1c88 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -1120,7 +1120,6 @@ static int verify_header(struct aa_ext *e, int start, const char **ns) { int error = -EPROTONOSUPPORT; const char *name = NULL; - *ns = NULL; /* get the interface version */ if (!aa_unpack_u32(e, &e->version, "version")) { @@ -1142,20 +1141,35 @@ static int verify_header(struct aa_ext *e, int start, const char **ns) return error; } - /* read the namespace if present */ + /* read the namespace if present and check it against policy load + * namespace specified in the data start header + */ if (aa_unpack_str(e, &name, "namespace")) { if (*name == '\0') { audit_iface(NULL, NULL, NULL, "invalid namespace name", e, error); return error; } + + /* don't allow different namespaces be specified in the same + * policy load set + */ + error = -EACCES; if (*ns && strcmp(*ns, name)) { - audit_iface(NULL, NULL, NULL, "invalid ns change", e, + audit_iface(NULL, NULL, NULL, + "policy load has mixed namespaces", e, error); - } else if (!*ns) { + return error; + } else if (!*ns && start) { + /* fill current policy load namespace at data start */ *ns = kstrdup(name, GFP_KERNEL); if (!*ns) return -ENOMEM; + } else if (!*ns) { + audit_iface(NULL, NULL, NULL, + "policy load has mixed namespaces", e, + error); + return error; } }