[GIT PULL] Fix for AppArmor oops in apparmor_setprocattr()

From: James Morris
Date: Thu Jul 07 2016 - 20:07:06 EST


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@xxxxxxxxxx>
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@xxxxxxxxxx>
modified for the case that apparmor provides null termination.

Fixes: bb646cdb12e75d82258c2f2e7746d5952d3e321a
Reported-by: Vegard Nossum <vegard.nossum@xxxxxxxxxx>
Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Cc: John Johansen <john.johansen@xxxxxxxxxxxxx>
Cc: Paul Moore <paul@xxxxxxxxxxxxxx>
Cc: Stephen Smalley <sds@xxxxxxxxxxxxx>
Cc: Eric Paris <eparis@xxxxxxxxxxxxxx>
Cc: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
Cc: stable@xxxxxxxxxx
Signed-off-by: John Johansen <john.johansen@xxxxxxxxxxxxx>
Reviewed-by: Tyler Hicks <tyhicks@xxxxxxxxxxxxx>
Signed-off-by: James Morris <james.l.morris@xxxxxxxxxx>

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,