Re: Fw: [PATCH 1/1] file capabilities: simplify signal check
On 02/24, Harald Welte wrote: > > On Sun, Feb 24, 2008 at 09:09:31PM +0300, Oleg Nesterov wrote: > > I just have an almost off-topic (sorry ;) question. Do we really need > > kill_pid_info_as_uid() ? Harald Welte cc'ed. > > > > From "[PATCH] Fix signal sending in usbdevio on async URB completion" > > commit 46113830a18847cff8da73005e57bc49c2f95a56 > > > > > If a process issues an URB from userspace and (starts to) terminate > > > before the URB comes back, we run into the issue described above. > > This > > > is because the urb saves a pointer to "current" when it is posted to > > the > > > device, but there's no guarantee that this pointer is still valid > > > afterwards. > > > > > > In fact, there are three separate issues: > > > > > > 1) the pointer to "current" can become invalid, since the task could > > be > > >completely gone when the URB completion comes back from the device. > > > > > > 2) Even if the saved task pointer is still pointing to a valid > > task_struct, > > >task_struct->sighand could have gone meanwhile. > > > > > > 3) Even if the process is perfectly fine, permissions may have > > changed, > > >and we can no longer send it a signal. > > > > The problems 1) and 2) are solved by converting to a struct pid. Is 3) a > > real > > problem? The task which does ioctl(USBDEVFS_SUBMITURB) explicitly asks to > > send > > the signal to it, should we deny the signal even if it changes its > > credentials > > in some way? > > At the time I discovered the abovementioned problem, '1' and '2' were > real practical issues that I was seeing on live systems, triggerable > from userspace with no problems. '3' was more of a theoretical issue > that was discovered while reading the code and spending some thought on > it. Yes, yes, I see, the patch was fine. > Whether or not we should deny the signal even if the process changes its > own credentials in some way sounds like a much more esoteric question to > me. I think it's fair to say that the resulting behavior is > "unspecified but shouldn't cause the process and/or kernel to misbehave" > > At least I'm not aware of any usbdevio logic that would require some > specific behaviour here. OK, thanks. So, the only reason why we can't kill kill_pid_info_as_uid() and just use kill_pid_info() is that USB uses .si_code = SI_ASYNCIO. The latter means that SI_FROMUSER(info) == T. I assume that it is not an option to change USB to use .si_code > 0, yes? Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Fw: [PATCH 1/1] file capabilities: simplify signal check
On 02/24, Harald Welte wrote: On Sun, Feb 24, 2008 at 09:09:31PM +0300, Oleg Nesterov wrote: I just have an almost off-topic (sorry ;) question. Do we really need kill_pid_info_as_uid() ? Harald Welte cc'ed. From [PATCH] Fix signal sending in usbdevio on async URB completion commit 46113830a18847cff8da73005e57bc49c2f95a56 If a process issues an URB from userspace and (starts to) terminate before the URB comes back, we run into the issue described above. This is because the urb saves a pointer to current when it is posted to the device, but there's no guarantee that this pointer is still valid afterwards. In fact, there are three separate issues: 1) the pointer to current can become invalid, since the task could be completely gone when the URB completion comes back from the device. 2) Even if the saved task pointer is still pointing to a valid task_struct, task_struct-sighand could have gone meanwhile. 3) Even if the process is perfectly fine, permissions may have changed, and we can no longer send it a signal. The problems 1) and 2) are solved by converting to a struct pid. Is 3) a real problem? The task which does ioctl(USBDEVFS_SUBMITURB) explicitly asks to send the signal to it, should we deny the signal even if it changes its credentials in some way? At the time I discovered the abovementioned problem, '1' and '2' were real practical issues that I was seeing on live systems, triggerable from userspace with no problems. '3' was more of a theoretical issue that was discovered while reading the code and spending some thought on it. Yes, yes, I see, the patch was fine. Whether or not we should deny the signal even if the process changes its own credentials in some way sounds like a much more esoteric question to me. I think it's fair to say that the resulting behavior is unspecified but shouldn't cause the process and/or kernel to misbehave At least I'm not aware of any usbdevio logic that would require some specific behaviour here. OK, thanks. So, the only reason why we can't kill kill_pid_info_as_uid() and just use kill_pid_info() is that USB uses .si_code = SI_ASYNCIO. The latter means that SI_FROMUSER(info) == T. I assume that it is not an option to change USB to use .si_code 0, yes? Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Fw: [PATCH 1/1] file capabilities: simplify signal check
On Sun, Feb 24, 2008 at 09:09:31PM +0300, Oleg Nesterov wrote: > I just have an almost off-topic (sorry ;) question. Do we really need > kill_pid_info_as_uid() ? Harald Welte cc'ed. > > From "[PATCH] Fix signal sending in usbdevio on async URB completion" > commit 46113830a18847cff8da73005e57bc49c2f95a56 > > > If a process issues an URB from userspace and (starts to) terminate > > before the URB comes back, we run into the issue described above. > This > > is because the urb saves a pointer to "current" when it is posted to > the > > device, but there's no guarantee that this pointer is still valid > > afterwards. > > > > In fact, there are three separate issues: > > > > 1) the pointer to "current" can become invalid, since the task could > be > >completely gone when the URB completion comes back from the device. > > > > 2) Even if the saved task pointer is still pointing to a valid > task_struct, > >task_struct->sighand could have gone meanwhile. > > > > 3) Even if the process is perfectly fine, permissions may have > changed, > >and we can no longer send it a signal. > > The problems 1) and 2) are solved by converting to a struct pid. Is 3) a real > problem? The task which does ioctl(USBDEVFS_SUBMITURB) explicitly asks to send > the signal to it, should we deny the signal even if it changes its credentials > in some way? At the time I discovered the abovementioned problem, '1' and '2' were real practical issues that I was seeing on live systems, triggerable from userspace with no problems. '3' was more of a theoretical issue that was discovered while reading the code and spending some thought on it. I personally am too remote to whatever you're currently doing to the code ('using struct pid') in order to give any comment. The overall process of 'saving the current pointer and re-using it at some later point while the original task might be gone or modified' must work. Whether or not we should deny the signal even if the process changes its own credentials in some way sounds like a much more esoteric question to me. I think it's fair to say that the resulting behavior is "unspecified but shouldn't cause the process and/or kernel to misbehave" At least I'm not aware of any usbdevio logic that would require some specific behaviour here. -- - Harald Welte <[EMAIL PROTECTED]> http://laforge.gnumonks.org/ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6) signature.asc Description: Digital signature
Re: Fw: [PATCH 1/1] file capabilities: simplify signal check
On 02/23, Eric W. Biederman wrote: > > Andrew Morton <[EMAIL PROTECTED]> writes: > > > um, is that code namespace-clean? > > Choke, gag. > > 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. (Can't comment, I never understood this security magic, but Eric's explanation looks very reasonable to me). I just have an almost off-topic (sorry ;) question. Do we really need kill_pid_info_as_uid() ? Harald Welte cc'ed. >From "[PATCH] Fix signal sending in usbdevio on async URB completion" commit 46113830a18847cff8da73005e57bc49c2f95a56 > If a process issues an URB from userspace and (starts to) terminate > before the URB comes back, we run into the issue described above. This > is because the urb saves a pointer to "current" when it is posted to the > device, but there's no guarantee that this pointer is still valid > afterwards. > > In fact, there are three separate issues: > > 1) the pointer to "current" can become invalid, since the task could be >completely gone when the URB completion comes back from the device. > > 2) Even if the saved task pointer is still pointing to a valid task_struct, >task_struct->sighand could have gone meanwhile. > > 3) Even if the process is perfectly fine, permissions may have changed, >and we can no longer send it a signal. The problems 1) and 2) are solved by converting to a struct pid. Is 3) a real problem? The task which does ioctl(USBDEVFS_SUBMITURB) explicitly asks to send the signal to it, should we deny the signal even if it changes its credentials in some way? Just curious. Thanks, Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Fw: [PATCH 1/1] file capabilities: simplify signal check
On 02/23, Eric W. Biederman wrote: Andrew Morton [EMAIL PROTECTED] writes: um, is that code namespace-clean? Choke, gag. 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. (Can't comment, I never understood this security magic, but Eric's explanation looks very reasonable to me). I just have an almost off-topic (sorry ;) question. Do we really need kill_pid_info_as_uid() ? Harald Welte cc'ed. From [PATCH] Fix signal sending in usbdevio on async URB completion commit 46113830a18847cff8da73005e57bc49c2f95a56 If a process issues an URB from userspace and (starts to) terminate before the URB comes back, we run into the issue described above. This is because the urb saves a pointer to current when it is posted to the device, but there's no guarantee that this pointer is still valid afterwards. In fact, there are three separate issues: 1) the pointer to current can become invalid, since the task could be completely gone when the URB completion comes back from the device. 2) Even if the saved task pointer is still pointing to a valid task_struct, task_struct-sighand could have gone meanwhile. 3) Even if the process is perfectly fine, permissions may have changed, and we can no longer send it a signal. The problems 1) and 2) are solved by converting to a struct pid. Is 3) a real problem? The task which does ioctl(USBDEVFS_SUBMITURB) explicitly asks to send the signal to it, should we deny the signal even if it changes its credentials in some way? Just curious. Thanks, Oleg. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Fw: [PATCH 1/1] file capabilities: simplify signal check
On Sun, Feb 24, 2008 at 09:09:31PM +0300, Oleg Nesterov wrote: I just have an almost off-topic (sorry ;) question. Do we really need kill_pid_info_as_uid() ? Harald Welte cc'ed. From [PATCH] Fix signal sending in usbdevio on async URB completion commit 46113830a18847cff8da73005e57bc49c2f95a56 If a process issues an URB from userspace and (starts to) terminate before the URB comes back, we run into the issue described above. This is because the urb saves a pointer to current when it is posted to the device, but there's no guarantee that this pointer is still valid afterwards. In fact, there are three separate issues: 1) the pointer to current can become invalid, since the task could be completely gone when the URB completion comes back from the device. 2) Even if the saved task pointer is still pointing to a valid task_struct, task_struct-sighand could have gone meanwhile. 3) Even if the process is perfectly fine, permissions may have changed, and we can no longer send it a signal. The problems 1) and 2) are solved by converting to a struct pid. Is 3) a real problem? The task which does ioctl(USBDEVFS_SUBMITURB) explicitly asks to send the signal to it, should we deny the signal even if it changes its credentials in some way? At the time I discovered the abovementioned problem, '1' and '2' were real practical issues that I was seeing on live systems, triggerable from userspace with no problems. '3' was more of a theoretical issue that was discovered while reading the code and spending some thought on it. I personally am too remote to whatever you're currently doing to the code ('using struct pid') in order to give any comment. The overall process of 'saving the current pointer and re-using it at some later point while the original task might be gone or modified' must work. Whether or not we should deny the signal even if the process changes its own credentials in some way sounds like a much more esoteric question to me. I think it's fair to say that the resulting behavior is unspecified but shouldn't cause the process and/or kernel to misbehave At least I'm not aware of any usbdevio logic that would require some specific behaviour here. -- - Harald Welte [EMAIL PROTECTED] http://laforge.gnumonks.org/ Privacy in residential applications is a desirable marketing option. (ETSI EN 300 175-7 Ch. A6) signature.asc Description: Digital signature
Re: Fw: [PATCH 1/1] file capabilities: simplify signal check
Andrew Morton <[EMAIL PROTECTED]> writes: > um, is that code namespace-clean? Choke, gag. 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" <[EMAIL PROTECTED]> > To: lkml > Subject: [PATCH 1/1] file capabilities: simplify signal check > > >>From bd076c7245d02be0cc01b7c09bd7170ec5946492 Mon Sep 17 00:00:00 2001 > From: Serge E. Hallyn <[EMAIL PROTECTED]> > 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 <[EMAIL PROTECTED]> > Signed-off-by: Andrew G. Morgan <[EMAIL PROTECTED]> > --- > 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) {
Re: Fw: [PATCH 1/1] file capabilities: simplify signal check
Andrew Morton [EMAIL PROTECTED] writes: um, is that code namespace-clean? Choke, gag. 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 [EMAIL PROTECTED] To: lkml linux-kernel@vger.kernel.org Subject: [PATCH 1/1] file capabilities: simplify signal check From bd076c7245d02be0cc01b7c09bd7170ec5946492 Mon Sep 17 00:00:00 2001 From: Serge E. Hallyn [EMAIL PROTECTED] 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 [EMAIL PROTECTED] Signed-off-by: Andrew G. Morgan [EMAIL PROTECTED] --- 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