| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20080227043306.GA9293@vino.hallyn.com>
Date: Tue, 26 Feb 2008 22:33:06 -0600
From: serge@...lyn.com
To: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Pavel Emelyanov <xemul@...nvz.org>,
Oleg Nesterov <oleg@...sign.ru>, linux-kernel@...r.kernel.org,
Andrew Morgan <morgan@...nel.org>
Subject: Re: Fw: [PATCH 1/1] file capabilities: simplify signal check
Quoting Eric W. Biederman (ebiederm@...ssion.com):
> Andrew Morton <akpm@...ux-foundation.org> writes:
>
> > um, is that code namespace-clean?
>
> Choke, gag.
Oh, sorry, I got lost in the set of patches in the message. To be
clear, my little 4-patch uid-ns-signal patchset can simply be updated
to make the cap_task_kill() uid check into if (task_user_equiv(current, p)
But Eric if you simply drop cap_task_kill() (don't make it return 0,
just drop the function and go back to not setting task_kill in the
capability_security_ops) I'll ack that. Else I'll write the patch
thursday. At this point the only thing that will be denied by
cap_task_kill() but not by check_kill_permission() is funky euid cases.
That's wrong. (cc'ing amorgan in the event I'm forgetting something
useful the fn is doing)
-serge
> There are uid namespace issues but since no one has finished the
> uid namespace that I am aware of that is minor.
>
> However the code does not appear clean/maintainable. The normal linux
> signal sending policy has already been enforce before we get to this
> point.
>
> So unless I am totally mistaken the code should read:
>
> int cap_task_kill(struct task_struct *p, struct siginfo *info,
> int sig, u32 secid)
> {
> if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
> return 0;
>
> if (!cap_issubset(p->cap_permitted, current->cap_permitted))
> return -EPERM;
>
> return 0;
> }
>
> Although doing it that way violates:
> /*
> * Running a setuid root program raises your capabilities.
> * Killing your own setuid root processes was previously
> * allowed.
> * We must preserve legacy signal behavior in this case.
> */
>
>
> Which says to me the code should really read:
> int cap_task_kill(struct task_struct *p, struct siginfo *info,
> int sig, u32 secid)
> {
> return 0;
> }
>
> The entire point of defining cap_task_kill under
> CONFIG_SECURITY_FILE_CAPABLITIES appears to be deny killing processes
> with more caps. Killing processes that we could ordinarily kill
> which have more caps appears to be precisely the case we have decided
> to allow. So the patched version of cap_task_kill appears to be an
> expensive way of doing nothing, just waiting for someone to complain
> about the last couple of cases it denies until it is truly a noop.
>
>
>
> > Thanks.
> >
> > Begin forwarded message:
> >
> > Date: Wed, 20 Feb 2008 10:15:50 -0600
> > From: "Serge E. Hallyn" <serue@...ibm.com>
> > To: lkml <linux-kernel@...r.kernel.org>
> > Subject: [PATCH 1/1] file capabilities: simplify signal check
> >
> >
> >>From bd076c7245d02be0cc01b7c09bd7170ec5946492 Mon Sep 17 00:00:00 2001
> > From: Serge E. Hallyn <serue@...ibm.com>
> > Date: Sun, 17 Feb 2008 20:28:07 -0500
> > Subject: [PATCH 1/1] file capabilities: simplify signal check
> >
> > Simplify the uid equivalence check in cap_task_kill(). Anyone
> > can kill a process owned by the same uid.
> >
> > Without this patch wireshark is reported to fail.
> >
> > Signed-off-by: Serge E. Hallyn <serue@...ibm.com>
> > Signed-off-by: Andrew G. Morgan <morgan@...nel.org>
> > ---
> > security/commoncap.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > index 5aba826..bb0c095 100644
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -552,7 +552,7 @@ int cap_task_kill(struct task_struct *p, struct siginfo
> > *info,
> > * allowed.
> > * We must preserve legacy signal behavior in this case.
> > */
> > - if (p->euid == 0 && p->uid == current->uid)
> > + if (p->uid == current->uid)
> > return 0;
> >
> > /* sigcont is permitted within same session */
> > --
> > 1.5.1.1.GIT
>
> So it looks to me like we should just give up trying to deny a few
> cases now.
>
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 5aba826..c1d1fd7 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -540,41 +540,6 @@ int cap_task_setnice (struct task_struct *p, int nice)
> return cap_safe_nice(p);
> }
>
> -int cap_task_kill(struct task_struct *p, struct siginfo *info,
> - int sig, u32 secid)
> -{
> - if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
> - return 0;
> -
> - /*
> - * Running a setuid root program raises your capabilities.
> - * Killing your own setuid root processes was previously
> - * allowed.
> - * We must preserve legacy signal behavior in this case.
> - */
> - if (p->euid == 0 && p->uid == current->uid)
> - return 0;
> -
> - /* sigcont is permitted within same session */
> - if (sig == SIGCONT && (task_session_nr(current) == task_session_nr(p)))
> - return 0;
> -
> - if (secid)
> - /*
> - * Signal sent as a particular user.
> - * Capabilities are ignored. May be wrong, but it's the
> - * only thing we can do at the moment.
> - * Used only by usb drivers?
> - */
> - return 0;
> - if (cap_issubset(p->cap_permitted, current->cap_permitted))
> - return 0;
> - if (capable(CAP_KILL))
> - return 0;
> -
> - return -EPERM;
> -}
> -
> /*
> * called from kernel/sys.c for prctl(PR_CABSET_DROP)
> * done without task_capability_lock() because it introduces
> @@ -605,13 +570,13 @@ int cap_task_setnice (struct task_struct *p, int nice)
> {
> return 0;
> }
> +#endif
> +
> int cap_task_kill(struct task_struct *p, struct siginfo *info,
> int sig, u32 secid)
> {
> return 0;
> }
> -#endif
> -
> void cap_task_reparent_to_init (struct task_struct *p)
> {
> cap_set_init_eff(p->cap_effective);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists