Commit e89b8081 authored by Vegard Nossum's avatar Vegard Nossum Committed by John Johansen
Browse files

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

Fixes: bb646cdb
Reported-by: default avatarVegard Nossum <vegard.nossum@oracle.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: John Johansen <john.johansen@canonical.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Eric Paris <eparis@parisplace.org>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: stable@kernel.org
Signed-off-by: default avatarJohn Johansen <john.johansen@canonical.com>
Reviewed-by: default avatarTyler Hicks <tyhicks@canonical.com>
Signed-off-by: default avatarJames Morris <james.l.morris@oracle.com>
parent f4ee2def

security/apparmor/lsm.c

+19 −17
Original line number Diff line number Diff line
@@ -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,

CRA Git | Maintained and supported by SUSTech CRA and CCSE