diff mbox series

Smack: fix use-after-free in smk_write_relabel_self()

Message ID 20200708201520.140376-1-ebiggers@kernel.org (mailing list archive)
State New, archived
Headers show
Series Smack: fix use-after-free in smk_write_relabel_self() | expand

Commit Message

Eric Biggers July 8, 2020, 8:15 p.m. UTC 
From: Eric Biggers <ebiggers@google.com>

smk_write_relabel_self() frees memory from the task's credentials with
no locking, which can easily cause a use-after-free because multiple
tasks can share the same credentials structure.

Fix this by using prepare_creds() and commit_creds() to correctly modify
the task's credentials.

Reproducer for "BUG: KASAN: use-after-free in smk_write_relabel_self":

	#include <fcntl.h>
	#include <pthread.h>
	#include <unistd.h>

	static void *thrproc(void *arg)
	{
		int fd = open("/sys/fs/smackfs/relabel-self", O_WRONLY);
		for (;;) write(fd, "foo", 3);
	}

	int main()
	{
		pthread_t t;
		pthread_create(&t, NULL, thrproc, NULL);
		thrproc(NULL);
	}

Reported-by: syzbot+e6416dabb497a650da40@syzkaller.appspotmail.com
Fixes: 38416e53936e ("Smack: limited capability for changing process label")
Cc: <stable@vger.kernel.org> # v4.4+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/smack/smackfs.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Comments

Sasha Levin July 16, 2020, 12:27 a.m. UTC | #1 
Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 38416e53936e ("Smack: limited capability for changing process label").

The bot has tested the following trees: v5.7.8, v5.4.51, v4.19.132, v4.14.188, v4.9.230, v4.4.230.

v5.7.8: Build OK!
v5.4.51: Build OK!
v4.19.132: Failed to apply! Possible dependencies:
    b17103a8b8ae9 ("Smack: Abstract use of cred security blob")

v4.14.188: Failed to apply! Possible dependencies:
    03450e271a160 ("fs: add ksys_fchmod() and do_fchmodat() helpers and ksys_chmod() wrapper; remove in-kernel calls to syscall")
    312db1aa1dc7b ("fs: add ksys_mount() helper; remove in-kernel calls to sys_mount()")
    3a18ef5c1b393 ("fs: add ksys_umount() helper; remove in-kernel call to sys_umount()")
    447016e968196 ("fs: add ksys_chdir() helper; remove in-kernel calls to sys_chdir()")
    819671ff849b0 ("syscalls: define and explain goal to not call syscalls in the kernel")
    9481769208b5e ("->file_open(): lose cred argument")
    a16fe33ab5572 ("fs: add ksys_chroot() helper; remove-in kernel calls to sys_chroot()")
    ae2bb293a3e8a ("get rid of cred argument of vfs_open() and do_dentry_open()")
    af04fadcaa932 ("Revert "fs: fold open_check_o_direct into do_dentry_open"")
    b17103a8b8ae9 ("Smack: Abstract use of cred security blob")
    c7248321a3d42 ("fs: add ksys_dup{,3}() helper; remove in-kernel calls to sys_dup{,3}()")
    d19dfe58b7ecb ("Smack: Privilege check on key operations")
    dcb569cf6ac99 ("Smack: ptrace capability use fixes")
    e3f20ae21079e ("security_file_open(): lose cred argument")
    e7a3e8b2edf54 ("fs: add ksys_write() helper; remove in-kernel calls to sys_write()")

v4.9.230: Failed to apply! Possible dependencies:
    078c73c63fb28 ("apparmor: add profile and ns params to aa_may_manage_policy()")
    121d4a91e3c12 ("apparmor: rename sid to secid")
    12557dcba21b0 ("apparmor: move lib definitions into separate lib include")
    2bd8dbbf22fe9 ("apparmor: add ns being viewed as a param to policy_view_capable()")
    5ac8c355ae001 ("apparmor: allow introspecting the loaded policy pre internal transform")
    637f688dc3dc3 ("apparmor: switch from profiles to using labels on contexts")
    73688d1ed0b8f ("apparmor: refactor prepare_ns() and make usable from different views")
    9481769208b5e ("->file_open(): lose cred argument")
    98849dff90e27 ("apparmor: rename namespace to ns to improve code line lengths")
    9a2d40c12d00e ("apparmor: add strn version of aa_find_ns")
    a6f233003b1af ("apparmor: allow specifying the profile doing the management")
    b17103a8b8ae9 ("Smack: Abstract use of cred security blob")
    cff281f6861e7 ("apparmor: split apparmor policy namespaces code into its own file")
    d19dfe58b7ecb ("Smack: Privilege check on key operations")
    dcb569cf6ac99 ("Smack: ptrace capability use fixes")
    f28e783ff668c ("Smack: Use cap_capable in privilege check")
    fd2a80438d736 ("apparmor: add ns being viewed as a param to policy_admin_capable()")
    fe6bb31f590c9 ("apparmor: split out shared policy_XXX fns to lib")

v4.4.230: Failed to apply! Possible dependencies:
    078c73c63fb28 ("apparmor: add profile and ns params to aa_may_manage_policy()")
    121d4a91e3c12 ("apparmor: rename sid to secid")
    12557dcba21b0 ("apparmor: move lib definitions into separate lib include")
    2bd8dbbf22fe9 ("apparmor: add ns being viewed as a param to policy_view_capable()")
    5ac8c355ae001 ("apparmor: allow introspecting the loaded policy pre internal transform")
    637f688dc3dc3 ("apparmor: switch from profiles to using labels on contexts")
    73688d1ed0b8f ("apparmor: refactor prepare_ns() and make usable from different views")
    79be093500791 ("Smack: File receive for sockets")
    9481769208b5e ("->file_open(): lose cred argument")
    98849dff90e27 ("apparmor: rename namespace to ns to improve code line lengths")
    9a2d40c12d00e ("apparmor: add strn version of aa_find_ns")
    a6f233003b1af ("apparmor: allow specifying the profile doing the management")
    b17103a8b8ae9 ("Smack: Abstract use of cred security blob")
    c60b906673eeb ("Smack: Signal delivery as an append operation")
    cff281f6861e7 ("apparmor: split apparmor policy namespaces code into its own file")
    d19dfe58b7ecb ("Smack: Privilege check on key operations")
    dcb569cf6ac99 ("Smack: ptrace capability use fixes")
    f28e783ff668c ("Smack: Use cap_capable in privilege check")
    fd2a80438d736 ("apparmor: add ns being viewed as a param to policy_admin_capable()")
    fe6bb31f590c9 ("apparmor: split out shared policy_XXX fns to lib")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?
Eric Biggers July 21, 2020, 12:38 a.m. UTC | #2 
On Wed, Jul 08, 2020 at 01:15:20PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> smk_write_relabel_self() frees memory from the task's credentials with
> no locking, which can easily cause a use-after-free because multiple
> tasks can share the same credentials structure.
> 
> Fix this by using prepare_creds() and commit_creds() to correctly modify
> the task's credentials.
> 
> Reproducer for "BUG: KASAN: use-after-free in smk_write_relabel_self":
> 
> 	#include <fcntl.h>
> 	#include <pthread.h>
> 	#include <unistd.h>
> 
> 	static void *thrproc(void *arg)
> 	{
> 		int fd = open("/sys/fs/smackfs/relabel-self", O_WRONLY);
> 		for (;;) write(fd, "foo", 3);
> 	}
> 
> 	int main()
> 	{
> 		pthread_t t;
> 		pthread_create(&t, NULL, thrproc, NULL);
> 		thrproc(NULL);
> 	}
> 
> Reported-by: syzbot+e6416dabb497a650da40@syzkaller.appspotmail.com
> Fixes: 38416e53936e ("Smack: limited capability for changing process label")
> Cc: <stable@vger.kernel.org> # v4.4+
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Ping.
Casey Schaufler July 21, 2020, 12:57 a.m. UTC | #3 
On 7/20/2020 5:38 PM, Eric Biggers wrote:
> On Wed, Jul 08, 2020 at 01:15:20PM -0700, Eric Biggers wrote:
>> From: Eric Biggers <ebiggers@google.com>
>>
>> smk_write_relabel_self() frees memory from the task's credentials with
>> no locking, which can easily cause a use-after-free because multiple
>> tasks can share the same credentials structure.
>>
>> Fix this by using prepare_creds() and commit_creds() to correctly modify
>> the task's credentials.
>>
>> Reproducer for "BUG: KASAN: use-after-free in smk_write_relabel_self":
>>
>> 	#include <fcntl.h>
>> 	#include <pthread.h>
>> 	#include <unistd.h>
>>
>> 	static void *thrproc(void *arg)
>> 	{
>> 		int fd = open("/sys/fs/smackfs/relabel-self", O_WRONLY);
>> 		for (;;) write(fd, "foo", 3);
>> 	}
>>
>> 	int main()
>> 	{
>> 		pthread_t t;
>> 		pthread_create(&t, NULL, thrproc, NULL);
>> 		thrproc(NULL);
>> 	}
>>
>> Reported-by: syzbot+e6416dabb497a650da40@syzkaller.appspotmail.com
>> Fixes: 38416e53936e ("Smack: limited capability for changing process label")
>> Cc: <stable@vger.kernel.org> # v4.4+
>> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Ping.

I have queued your patch and will be pushing it for next shortly.
diff mbox series

Patch

diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index c21b656b3263..840a192e9337 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -2720,7 +2720,6 @@  static int smk_open_relabel_self(struct inode *inode, struct file *file)
 static ssize_t smk_write_relabel_self(struct file *file, const char __user *buf,
 				size_t count, loff_t *ppos)
 {
-	struct task_smack *tsp = smack_cred(current_cred());
 	char *data;
 	int rc;
 	LIST_HEAD(list_tmp);
@@ -2745,11 +2744,21 @@  static ssize_t smk_write_relabel_self(struct file *file, const char __user *buf,
 	kfree(data);
 
 	if (!rc || (rc == -EINVAL && list_empty(&list_tmp))) {
+		struct cred *new;
+		struct task_smack *tsp;
+
+		new = prepare_creds();
+		if (!new) {
+			rc = -ENOMEM;
+			goto out;
+		}
+		tsp = smack_cred(new);
 		smk_destroy_label_list(&tsp->smk_relabel);
 		list_splice(&list_tmp, &tsp->smk_relabel);
+		commit_creds(new);
 		return count;
 	}
-
+out:
 	smk_destroy_label_list(&list_tmp);
 	return rc;
 }