[PATCH] selinux: fix empty write to keycreate file

[PATCH] selinux: fix empty write to keycreate file

[Ondrej Mosnacek]

When sid == 0 (we are resetting keycreate_sid to the default value), we
should skip the KEY__CREATE check.

Before this patch, doing a zero-sized write to /proc/self/keycreate
would check if the current task can create unlabeled keys (which would
usually fail with -EACCESS and generate an AVC). Now it skips the check
and correctly sets the task's keycreate_sid to 0.

Bug report: https://bugzilla.redhat.com/show_bug.cgi?id=1719067

Tested using the reproducer from the report above.

Fixes: 4eb582cf1fbd ("[PATCH] keys: add a way to store the appropriate context for newly-created keys")
Reported-by: Kir Kolyshkin <kir@sacred.ru>
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/hooks.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c61787b15f27..f77b314d0575 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6331,11 +6331,12 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
 	} else if (!strcmp(name, "fscreate")) {
 		tsec->create_sid = sid;
 	} else if (!strcmp(name, "keycreate")) {
-		error = avc_has_perm(&selinux_state,
-				     mysid, sid, SECCLASS_KEY, KEY__CREATE,
-				     NULL);
-		if (error)
-			goto abort_change;
+		if (sid) {
+			error = avc_has_perm(&selinux_state, mysid, sid,
+					     SECCLASS_KEY, KEY__CREATE, NULL);
+			if (error)
+				goto abort_change;
+		}
 		tsec->keycreate_sid = sid;
 	} else if (!strcmp(name, "sockcreate")) {
 		tsec->sockcreate_sid = sid;
-- 
2.20.1

Re: [PATCH] selinux: fix empty write to keycreate file

[Daniel Walsh]

On 6/12/19 4:12 AM, Ondrej Mosnacek wrote:
> When sid == 0 (we are resetting keycreate_sid to the default value), we
> should skip the KEY__CREATE check.
>
> Before this patch, doing a zero-sized write to /proc/self/keycreate
> would check if the current task can create unlabeled keys (which would
> usually fail with -EACCESS and generate an AVC). Now it skips the check
> and correctly sets the task's keycreate_sid to 0.
>
> Bug report: https://bugzilla.redhat.com/show_bug.cgi?id=1719067
>
> Tested using the reproducer from the report above.
>
> Fixes: 4eb582cf1fbd ("[PATCH] keys: add a way to store the appropriate context for newly-created keys")
> Reported-by: Kir Kolyshkin <kir@sacred.ru>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/hooks.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c61787b15f27..f77b314d0575 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6331,11 +6331,12 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>  	} else if (!strcmp(name, "fscreate")) {
>  		tsec->create_sid = sid;
>  	} else if (!strcmp(name, "keycreate")) {
> -		error = avc_has_perm(&selinux_state,
> -				     mysid, sid, SECCLASS_KEY, KEY__CREATE,
> -				     NULL);
> -		if (error)
> -			goto abort_change;
> +		if (sid) {
> +			error = avc_has_perm(&selinux_state, mysid, sid,
> +					     SECCLASS_KEY, KEY__CREATE, NULL);
> +			if (error)
> +				goto abort_change;
> +		}
>  		tsec->keycreate_sid = sid;
>  	} else if (!strcmp(name, "sockcreate")) {
>  		tsec->sockcreate_sid = sid;

This issue is causing us to add


allow XYZ_t unlabeled_t:key manage_key_perms

to any domains that are executing runc.

Re: [PATCH] selinux: fix empty write to keycreate file

[Paul Moore]

On Wed, Jun 12, 2019 at 4:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> When sid == 0 (we are resetting keycreate_sid to the default value), we
> should skip the KEY__CREATE check.
>
> Before this patch, doing a zero-sized write to /proc/self/keycreate
> would check if the current task can create unlabeled keys (which would
> usually fail with -EACCESS and generate an AVC). Now it skips the check
> and correctly sets the task's keycreate_sid to 0.
>
> Bug report: https://bugzilla.redhat.com/show_bug.cgi?id=1719067
>
> Tested using the reproducer from the report above.
>
> Fixes: 4eb582cf1fbd ("[PATCH] keys: add a way to store the appropriate context for newly-created keys")
> Reported-by: Kir Kolyshkin <kir@sacred.ru>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/hooks.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

Looks good to me, I just merged it into selinux/next.

Continuing on our best practices trend this week ... While I like
seeing links to publicly accessible bug trackers in patches, it is
important to remember that the patch description should contain all
the important information.  In other words, one should be able to look
at the git log on a island in the middle of the ocean - no network
connectivity - and make sense of the commit.  It isn't a big deal for
this patch, both because you explained the problem and the patch
itself if trivial, but it is something to keep in mind when linking to
external issue trackers.

I've never rejected a patch because the description was too long ;)

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c61787b15f27..f77b314d0575 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6331,11 +6331,12 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>         } else if (!strcmp(name, "fscreate")) {
>                 tsec->create_sid = sid;
>         } else if (!strcmp(name, "keycreate")) {
> -               error = avc_has_perm(&selinux_state,
> -                                    mysid, sid, SECCLASS_KEY, KEY__CREATE,
> -                                    NULL);
> -               if (error)
> -                       goto abort_change;
> +               if (sid) {
> +                       error = avc_has_perm(&selinux_state, mysid, sid,
> +                                            SECCLASS_KEY, KEY__CREATE, NULL);
> +                       if (error)
> +                               goto abort_change;
> +               }
>                 tsec->keycreate_sid = sid;
>         } else if (!strcmp(name, "sockcreate")) {
>                 tsec->sockcreate_sid = sid;
> --
> 2.20.1

-- 
paul moore
www.paul-moore.com