| Message ID | 20190612081226.21004-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | selinux: fix empty write to keycreate file | expand |
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.
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
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;
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(-)