linux-kernel - [GIT PULL] Fix for AppArmor oops in apparmor

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>] [day] [month] [year] [list]

Message-ID: <alpine.LRH.2.20.1607081004001.20331@namei.org>
Date:	Fri, 8 Jul 2016 10:06:51 +1000 (AEST)
From:	James Morris <jmorris@...ei.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
cc:	linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: [GIT PULL] Fix for AppArmor oops in apparmor_setprocattr()

Please pull.

Note: it may be possible to get a local privilege escalation out of this 
bug.


The following changes since commit ac904ae6e6f0a56be7b9a1cf66fbd50dd025fb06:

  Merge branch 'for-linus' of git://git.kernel.dk/linux-block (2016-07-07 15:34:09 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git for-linus

Vegard Nossum (1):
      apparmor: fix oops, validate buffer size in apparmor_setprocattr()

 security/apparmor/lsm.c |   36 +++++++++++++++++++-----------------
 1 files changed, 19 insertions(+), 17 deletions(-)

---

commit 30a46a4647fd1df9cf52e43bf467f0d9265096ca
Author: Vegard Nossum <vegard.nossum@...cle.com>
Date:   Thu Jul 7 13:41:11 2016 -0700

    apparmor: fix oops, validate buffer size in apparmor_setprocattr()
    
    When proc_pid_attr_write() was changed to use memdup_user apparmor's
    (interface violating) assumption that the setprocattr buffer was always
    a single page was violated.
    
    The size test is not strictly speaking needed as proc_pid_attr_write()
    will reject anything larger, but for the sake of robustness we can keep
    it in.
    
    SMACK and SELinux look safe to me, but somebody else should probably
    have a look just in case.
    
    Based on original patch from Vegard Nossum <vegard.nossum@...cle.com>
    modified for the case that apparmor provides null termination.
    
    Fixes: bb646cdb12e75d82258c2f2e7746d5952d3e321a
    Reported-by: Vegard Nossum <vegard.nossum@...cle.com>
    Cc: Al Viro <viro@...iv.linux.org.uk>
    Cc: John Johansen <john.johansen@...onical.com>
    Cc: Paul Moore <paul@...l-moore.com>
    Cc: Stephen Smalley <sds@...ho.nsa.gov>
    Cc: Eric Paris <eparis@...isplace.org>
    Cc: Casey Schaufler <casey@...aufler-ca.com>
    Cc: stable@...nel.org
    Signed-off-by: John Johansen <john.johansen@...onical.com>
    Reviewed-by: Tyler Hicks <tyhicks@...onical.com>
    Signed-off-by: James Morris <james.l.morris@...cle.com>

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 2660fbc..7798e16 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -500,34 +500,34 @@ static int apparmor_setprocattr(struct task_struct *task, char *name,
 {
 	struct common_audit_data sa;
 	struct apparmor_audit_data aad = {0,};
-	char *command, *args = value;
+	char *command, *largs = NULL, *args = value;
 	size_t arg_size;
 	int error;
 
 	if (size == 0)
 		return -EINVAL;
-	/* args points to a PAGE_SIZE buffer, AppArmor requires that
-	 * the buffer must be null terminated or have size <= PAGE_SIZE -1
-	 * so that AppArmor can null terminate them
-	 */
-	if (args[size - 1] != '\0') {
-		if (size == PAGE_SIZE)
-			return -EINVAL;
-		args[size] = '\0';
-	}
-
 	/* task can only write its own attributes */
 	if (current != task)
 		return -EACCES;
 
-	args = value;
+	/* AppArmor requires that the buffer must be null terminated atm */
+	if (args[size - 1] != '\0') {
+		/* null terminate */
+		largs = args = kmalloc(size + 1, GFP_KERNEL);
+		if (!args)
+			return -ENOMEM;
+		memcpy(args, value, size);
+		args[size] = '\0';
+	}
+
+	error = -EINVAL;
 	args = strim(args);
 	command = strsep(&args, " ");
 	if (!args)
-		return -EINVAL;
+		goto out;
 	args = skip_spaces(args);
 	if (!*args)
-		return -EINVAL;
+		goto out;
 
 	arg_size = size - (args - (char *) value);
 	if (strcmp(name, "current") == 0) {
@@ -553,10 +553,12 @@ static int apparmor_setprocattr(struct task_struct *task, char *name,
 			goto fail;
 	} else
 		/* only support the "current" and "exec" process attributes */
-		return -EINVAL;
+		goto fail;
 
 	if (!error)
 		error = size;
+out:
+	kfree(largs);
 	return error;
 
 fail:
@@ -565,9 +567,9 @@ fail:
 	aad.profile = aa_current_profile();
 	aad.op = OP_SETPROCATTR;
 	aad.info = name;
-	aad.error = -EINVAL;
+	aad.error = error = -EINVAL;
 	aa_audit_msg(AUDIT_APPARMOR_DENIED, &sa, NULL);
-	return -EINVAL;
+	goto out;
 }
 
 static int apparmor_task_setrlimit(struct task_struct *task,