Quoting [email protected] ([email protected]):
> Hello,
> I'm not sure it is you the right person to contact.
> I tried to run latest normal user wireshark with SUID dumpcap without success
> under linux-2.6.24.2. After looking around it seems to be related to the file
> in the kernel security/commoncap.c
> Inside function cap_task_kill (sorry I did not create a diff) I'm wondering if
> the check is correct:
> /*
> * 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;
>
> Should this check not be:
> if (p->suid == 0 && p->uid == current->uid)
> return 0;
>
> At least if I change this statement, wireshark runs correctly with as normal
> user.
> For explanation: during capture, wireshark spins a new suid process called
> dumpcap. To stop capture, wireshark sends SIGUSR1 to dumpcap. Hower, without
> changing the line, kill fails with EPERM.
>
> Thanks for your consideration.
> Kind Regards,
> Charles
Hi Charles,
thanks for the report. Interesting. I just downloaded the wireshark
source and as I suspected it's using capabilities. The problem then is
that the helper starts as setuid root, sets just the capabilities it
needs, then changes back to it's original userid. It's actually what
we'd like people to be doing. But they then rely upon the traditional
setuid behavior of the unprivileged process being able to kill the
privileged one. This is why checking suid instead of euid at
cap_task_kill() works for you.
Two quick fixes for you right now (apart from the one you've already
got :) would be
1. give wireshark cap_kill, by doing something like
capset cap_kill=ep /bin/wireshark
2. compile a kernel with SECURITY_FILE_CAPABILITIES=n
Andrew, this pretty much was bound to happen... we need to figure out
what our approach here should be. My preference is still to allow
signals when p->uid==current->uid so long as !SECURE_NOROOT. Then as
people start using secure_noroot process trees they at least must know
what they're asking for.
An alternative stance is to accept these things as they come up and try
to quickly work with the authors of such programs to work around it. I
suppose in a security sense that's the superior way :) But it also
seems likely to lead to most people choosing option 2 above and not
bothering to fix the problem.
thanks,
-serge
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Serge E. Hallyn wrote:
| Andrew, this pretty much was bound to happen... we need to figure out
| what our approach here should be. My preference is still to allow
| signals when p->uid==current->uid so long as !SECURE_NOROOT. Then as
| people start using secure_noroot process trees they at least must know
| what they're asking for.
I don't think there is anything special about root.
I've been trying to advocate that we remove the *uid == 0 part of this
check since we discussed it in November:
As I said 11/29/07 [Re: [patch 31/55] file capabilities: don't prevent
signaling setuid root programs]:
| I actually said (11/26/07):
|> >> Serge,
|> >>
|> >> I still feel a bit uneasy about this. Looking ahead, with filesystem
|> >> capabilities, one can simulate this same situation with a setuid
|> >> 'non-root' program as follows:
|> >>
|> >> [... example of simulating the same situation with setuid-non-root
...]
|> >>
|> >> Is there a compelling reason to include the euid==0 check?
So, independent of whether SECURE_NOROOT is in effect or not, I think
this particular line should simply read:
~ if (p->uid == current->uid)
~ return 0;
| An alternative stance is to accept these things as they come up and try
| to quickly work with the authors of such programs to work around it. I
| suppose in a security sense that's the superior way :) But it also
| seems likely to lead to most people choosing option 2 above and not
| bothering to fix the problem.
Cheers
Andrew
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
iD8DBQFHuN1V+bHCR3gb8jsRAkqnAJ9o9j9KALm/LxWRoU9PGo9f7UWNYgCdGTQC
Pm0daaJRMhWzcGSsTNgqj44=
=EkD2
-----END PGP SIGNATURE-----
Quoting Andrew G. Morgan ([email protected]):
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Serge E. Hallyn wrote:
> | Andrew, this pretty much was bound to happen... we need to figure out
> | what our approach here should be. My preference is still to allow
> | signals when p->uid==current->uid so long as !SECURE_NOROOT. Then as
> | people start using secure_noroot process trees they at least must know
> | what they're asking for.
>
> I don't think there is anything special about root.
>
> I've been trying to advocate that we remove the *uid == 0 part of this
> check since we discussed it in November:
>
> As I said 11/29/07 [Re: [patch 31/55] file capabilities: don't prevent
> signaling setuid root programs]:
> | I actually said (11/26/07):
> |> >> Serge,
> |> >>
> |> >> I still feel a bit uneasy about this. Looking ahead, with filesystem
> |> >> capabilities, one can simulate this same situation with a setuid
> |> >> 'non-root' program as follows:
> |> >>
> |> >> [... example of simulating the same situation with setuid-non-root
> ...]
> |> >>
> |> >> Is there a compelling reason to include the euid==0 check?
>
> So, independent of whether SECURE_NOROOT is in effect or not, I think
> this particular line should simply read:
>
> ~ if (p->uid == current->uid)
> ~ return 0;
Hmm, well unless I misunderstand I think I'm fine with that. And I must
have completely misunderstood you in November or my memory is just
playing tricks.
So the following patch against current -git is ok with you?
thanks,
-serge
>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.
Signed-off-by: Serge E. Hallyn <[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
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Serge E. Hallyn wrote:
| Signed-off-by: Serge E. Hallyn <[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;
Signed-off-by: Andrew G. Morgan <[email protected]>
Cheers
Andrew
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
iD8DBQFHuOWf+bHCR3gb8jsRAr5jAKCQ9MTWW9VNKGbbhacygeI6G7kqTACcCMEP
hyz+xgh91wN3+6dcL72S85c=
=Fjd8
-----END PGP SIGNATURE-----
--- "Serge E. Hallyn" <[email protected]> wrote:
> ....
>
> Two quick fixes for you right now (apart from the one you've already
> got :) would be
>
> 1. give wireshark cap_kill, by doing something like
>
> capset cap_kill=ep /bin/wireshark
>
> 2. compile a kernel with SECURITY_FILE_CAPABILITIES=n
>
> Andrew, this pretty much was bound to happen... we need to figure out
> what our approach here should be. My preference is still to allow
> signals when p->uid==current->uid so long as !SECURE_NOROOT. Then as
> people start using secure_noroot process trees they at least must know
> what they're asking for.
>
> An alternative stance is to accept these things as they come up and try
> to quickly work with the authors of such programs to work around it. I
> suppose in a security sense that's the superior way :) But it also
> seems likely to lead to most people choosing option 2 above and not
> bothering to fix the problem.
I probably just missed it when it went by, but do you have some
test cases for file capabilities lying about that I might use?
Thank you.
Casey Schaufler
[email protected]
Quoting Casey Schaufler ([email protected]):
>
> --- "Serge E. Hallyn" <[email protected]> wrote:
>
> > ....
> >
> > Two quick fixes for you right now (apart from the one you've already
> > got :) would be
> >
> > 1. give wireshark cap_kill, by doing something like
> >
> > capset cap_kill=ep /bin/wireshark
> >
> > 2. compile a kernel with SECURITY_FILE_CAPABILITIES=n
> >
> > Andrew, this pretty much was bound to happen... we need to figure out
> > what our approach here should be. My preference is still to allow
> > signals when p->uid==current->uid so long as !SECURE_NOROOT. Then as
> > people start using secure_noroot process trees they at least must know
> > what they're asking for.
> >
> > An alternative stance is to accept these things as they come up and try
> > to quickly work with the authors of such programs to work around it. I
> > suppose in a security sense that's the superior way :) But it also
> > seems likely to lead to most people choosing option 2 above and not
> > bothering to fix the problem.
>
> I probably just missed it when it went by, but do you have some
> test cases for file capabilities lying about that I might use?
Yup, please download the latest ltp cvs tree (I don't think it's in any
release yet, though I may be wrong) and look under
testcaes/kernel/security/filecaps. Or just do
cd ltp
make && make install && ./runltp -s filecap
-serge