lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240113121556.12948-3-pchelkin@ispras.ru>
Date: Sat, 13 Jan 2024 15:15:52 +0300
From: Fedor Pchelkin <pchelkin@...ras.ru>
To: John Johansen <john.johansen@...onical.com>
Cc: Fedor Pchelkin <pchelkin@...ras.ru>,
	Paul Moore <paul@...l-moore.com>,
	James Morris <jmorris@...ei.org>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	apparmor@...ts.ubuntu.com,
	linux-security-module@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Alexey Khoroshilov <khoroshilov@...ras.ru>,
	lvc-project@...uxtesting.org
Subject: [PATCH 2/2] apparmor: fix namespace check in serialized stream headers from the same policy load

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 <pchelkin@...ras.ru>
---
 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;
 		}
 	}
 
-- 
2.43.0


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ