[PATCH 0/2] seccomp: Make NEW_LISTENER and TSYNC flags exclusive

[PATCH 0/2] seccomp: Make NEW_LISTENER and TSYNC flags exclusive

[Kees Cook]

This is just sending out the tweaked fix from Tycho and the selftest
changes needed to support it. I intend to send this to Linus directly
after it's been in -next for a few days for v5.1 fixes.

Thanks!

-Kees

Kees Cook (1):
  selftests/seccomp: Prepare for exclusive seccomp flags

Tycho Andersen (1):
  seccomp: Make NEW_LISTENER and TSYNC flags exclusive

 kernel/seccomp.c                              | 17 ++++++++--
 tools/testing/selftests/seccomp/seccomp_bpf.c | 34 ++++++++++++++-----
 2 files changed, 40 insertions(+), 11 deletions(-)

-- 
2.17.1

[PATCH 1/2] selftests/seccomp: Prepare for exclusive seccomp flags

[Kees Cook]

Some seccomp flags will become exclusive, so the selftest needs to
be adjusted to mask those out and test them individually for the "all
flags" tests.

Cc: stable@vger.kernel.org # v5.0+
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 34 ++++++++++++++-----
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index f69d2ee29742..5019cdae5d0b 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -2166,11 +2166,14 @@ TEST(detect_seccomp_filter_flags)
 				 SECCOMP_FILTER_FLAG_LOG,
 				 SECCOMP_FILTER_FLAG_SPEC_ALLOW,
 				 SECCOMP_FILTER_FLAG_NEW_LISTENER };
-	unsigned int flag, all_flags;
+	unsigned int exclusive[] = {
+				SECCOMP_FILTER_FLAG_TSYNC,
+				SECCOMP_FILTER_FLAG_NEW_LISTENER };
+	unsigned int flag, all_flags, exclusive_mask;
 	int i;
 	long ret;
 
-	/* Test detection of known-good filter flags */
+	/* Test detection of individual known-good filter flags */
 	for (i = 0, all_flags = 0; i < ARRAY_SIZE(flags); i++) {
 		int bits = 0;
 
@@ -2197,16 +2200,29 @@ TEST(detect_seccomp_filter_flags)
 		all_flags |= flag;
 	}
 
-	/* Test detection of all known-good filter flags */
-	ret = seccomp(SECCOMP_SET_MODE_FILTER, all_flags, NULL);
-	EXPECT_EQ(-1, ret);
-	EXPECT_EQ(EFAULT, errno) {
-		TH_LOG("Failed to detect that all known-good filter flags (0x%X) are supported!",
-		       all_flags);
+	/*
+	 * Test detection of all known-good filter flags combined. But
+	 * for the exclusive flags we need to mask them out and try them
+	 * individually for the "all flags" testing.
+	 */
+	exclusive_mask = 0;
+	for (i = 0; i < ARRAY_SIZE(exclusive); i++)
+		exclusive_mask |= exclusive[i];
+	for (i = 0; i < ARRAY_SIZE(exclusive); i++) {
+		flag = all_flags & ~exclusive_mask;
+		flag |= exclusive[i];
+
+		ret = seccomp(SECCOMP_SET_MODE_FILTER, flag, NULL);
+		EXPECT_EQ(-1, ret);
+		EXPECT_EQ(EFAULT, errno) {
+			TH_LOG("Failed to detect that all known-good filter flags (0x%X) are supported!",
+			       flag);
+		}
 	}
 
-	/* Test detection of an unknown filter flag */
+	/* Test detection of an unknown filter flags, without exclusives. */
 	flag = -1;
+	flag &= ~exclusive_mask;
 	ret = seccomp(SECCOMP_SET_MODE_FILTER, flag, NULL);
 	EXPECT_EQ(-1, ret);
 	EXPECT_EQ(EINVAL, errno) {
-- 
2.17.1

[PATCH 2/2] seccomp: Make NEW_LISTENER and TSYNC flags exclusive

[Kees Cook]

From: Tycho Andersen <tycho@tycho.ws>

As the comment notes, the return codes for TSYNC and NEW_LISTENER
conflict, because they both return positive values, one in the case of
success and one in the case of error. So, let's disallow both of these
flags together.

While this is technically a userspace break, all the users I know
of are still waiting on me to land this feature in libseccomp, so I
think it'll be safe. Also, at present my use case doesn't require
TSYNC at all, so this isn't a big deal to disallow. If someone
wanted to support this, a path forward would be to add a new flag like
TSYNC_AND_LISTENER_YES_I_UNDERSTAND_THAT_TSYNC_WILL_JUST_RETURN_EAGAIN,
but the use cases are so different I don't see it really happening.

Finally, it's worth noting that this does actually fix a UAF issue: at the
end of seccomp_set_mode_filter(), we have:

        if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) {
                if (ret < 0) {
                        listener_f->private_data = NULL;
                        fput(listener_f);
                        put_unused_fd(listener);
                } else {
                        fd_install(listener, listener_f);
                        ret = listener;
                }
        }
out_free:
        seccomp_filter_free(prepared);

But if ret > 0 because TSYNC raced, we'll install the listener fd and then
free the filter out from underneath it, causing a UAF when the task closes
it or dies. This patch also switches the condition to be simply if (ret),
so that if someone does add the flag mentioned above, they won't have to
remember to fix this too.

Reported-by: syzbot+b562969adb2e04af3442@syzkaller.appspotmail.com
Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace")
CC: stable@vger.kernel.org # v5.0+
Signed-off-by: Tycho Andersen <tycho@tycho.ws>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/seccomp.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index df27e499956a..3582eeb59893 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -502,7 +502,10 @@ seccomp_prepare_user_filter(const char __user *user_filter)
  *
  * Caller must be holding current->sighand->siglock lock.
  *
- * Returns 0 on success, -ve on error.
+ * Returns 0 on success, -ve on error, or
+ *   - in TSYNC mode: the pid of a thread which was either not in the correct
+ *     seccomp mode or did not have an ancestral seccomp filter
+ *   - in NEW_LISTENER mode: the fd of the new listener
  */
 static long seccomp_attach_filter(unsigned int flags,
 				  struct seccomp_filter *filter)
@@ -1258,6 +1261,16 @@ static long seccomp_set_mode_filter(unsigned int flags,
 	if (flags & ~SECCOMP_FILTER_FLAG_MASK)
 		return -EINVAL;
 
+	/*
+	 * In the successful case, NEW_LISTENER returns the new listener fd.
+	 * But in the failure case, TSYNC returns the thread that died. If you
+	 * combine these two flags, there's no way to tell whether something
+	 * succeeded or failed. So, let's disallow this combination.
+	 */
+	if ((flags & SECCOMP_FILTER_FLAG_TSYNC) &&
+	    (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER))
+		return -EINVAL;
+
 	/* Prepare the new filter before holding any locks. */
 	prepared = seccomp_prepare_user_filter(filter);
 	if (IS_ERR(prepared))
@@ -1304,7 +1317,7 @@ static long seccomp_set_mode_filter(unsigned int flags,
 		mutex_unlock(&current->signal->cred_guard_mutex);
 out_put_fd:
 	if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) {
-		if (ret < 0) {
+		if (ret) {
 			listener_f->private_data = NULL;
 			fput(listener_f);
 			put_unused_fd(listener);
-- 
2.17.1

Re: [PATCH 1/2] selftests/seccomp: Prepare for exclusive seccomp flags

[Tycho Andersen]

On Wed, Apr 24, 2019 at 09:37:55AM -0700, Kees Cook wrote:
> Some seccomp flags will become exclusive, so the selftest needs to
> be adjusted to mask those out and test them individually for the "all
> flags" tests.
> 
> Cc: stable@vger.kernel.org # v5.0+
> Signed-off-by: Kees Cook <keescook@chromium.org>

Whoops, thanks for this too.

Reviewed-by: Tycho Andersen <tycho@tycho.ws>

Re: [PATCH 1/2] selftests/seccomp: Prepare for exclusive seccomp flags

[James Morris]

On Wed, 24 Apr 2019, Kees Cook wrote:

> Some seccomp flags will become exclusive, so the selftest needs to
> be adjusted to mask those out and test them individually for the "all
> flags" tests.
> 
> Cc: stable@vger.kernel.org # v5.0+
> Signed-off-by: Kees Cook <keescook@chromium.org>


Acked-by: James Morris <jamorris@linux.microsoft.com>

> ---
>  tools/testing/selftests/seccomp/seccomp_bpf.c | 34 ++++++++++++++-----
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index f69d2ee29742..5019cdae5d0b 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -2166,11 +2166,14 @@ TEST(detect_seccomp_filter_flags)
>  				 SECCOMP_FILTER_FLAG_LOG,
>  				 SECCOMP_FILTER_FLAG_SPEC_ALLOW,
>  				 SECCOMP_FILTER_FLAG_NEW_LISTENER };
> -	unsigned int flag, all_flags;
> +	unsigned int exclusive[] = {
> +				SECCOMP_FILTER_FLAG_TSYNC,
> +				SECCOMP_FILTER_FLAG_NEW_LISTENER };
> +	unsigned int flag, all_flags, exclusive_mask;
>  	int i;
>  	long ret;
>  
> -	/* Test detection of known-good filter flags */
> +	/* Test detection of individual known-good filter flags */
>  	for (i = 0, all_flags = 0; i < ARRAY_SIZE(flags); i++) {
>  		int bits = 0;
>  
> @@ -2197,16 +2200,29 @@ TEST(detect_seccomp_filter_flags)
>  		all_flags |= flag;
>  	}
>  
> -	/* Test detection of all known-good filter flags */
> -	ret = seccomp(SECCOMP_SET_MODE_FILTER, all_flags, NULL);
> -	EXPECT_EQ(-1, ret);
> -	EXPECT_EQ(EFAULT, errno) {
> -		TH_LOG("Failed to detect that all known-good filter flags (0x%X) are supported!",
> -		       all_flags);
> +	/*
> +	 * Test detection of all known-good filter flags combined. But
> +	 * for the exclusive flags we need to mask them out and try them
> +	 * individually for the "all flags" testing.
> +	 */
> +	exclusive_mask = 0;
> +	for (i = 0; i < ARRAY_SIZE(exclusive); i++)
> +		exclusive_mask |= exclusive[i];
> +	for (i = 0; i < ARRAY_SIZE(exclusive); i++) {
> +		flag = all_flags & ~exclusive_mask;
> +		flag |= exclusive[i];
> +
> +		ret = seccomp(SECCOMP_SET_MODE_FILTER, flag, NULL);
> +		EXPECT_EQ(-1, ret);
> +		EXPECT_EQ(EFAULT, errno) {
> +			TH_LOG("Failed to detect that all known-good filter flags (0x%X) are supported!",
> +			       flag);
> +		}
>  	}
>  
> -	/* Test detection of an unknown filter flag */
> +	/* Test detection of an unknown filter flags, without exclusives. */
>  	flag = -1;
> +	flag &= ~exclusive_mask;
>  	ret = seccomp(SECCOMP_SET_MODE_FILTER, flag, NULL);
>  	EXPECT_EQ(-1, ret);
>  	EXPECT_EQ(EINVAL, errno) {
> 

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 2/2] seccomp: Make NEW_LISTENER and TSYNC flags exclusive

[James Morris]

On Wed, 24 Apr 2019, Kees Cook wrote:

> Reported-by: syzbot+b562969adb2e04af3442@syzkaller.appspotmail.com
> Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace")
> CC: stable@vger.kernel.org # v5.0+
> Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> Signed-off-by: Kees Cook <keescook@chromium.org>


Acked-by: James Morris <jamorris@linux.microsoft.com>


-- 
James Morris
<jmorris@namei.org>