----- "Hugh Dickins" <[email protected]> wrote:
> On Thu, 26 Feb 2009, Joe Malicki wrote:
> > ----- "Joe Malicki" <[email protected]> wrote:
> >
> > > Very rarely, we experience a setuid program not properly getting
> > > the euid of its owner.
> > >
> > > Thus far, we have only seen failures for the program being setuid
> > > root, being run by a non-root user, on a multi-core machine.
> Trying
> > > to
> > > setuid to a user from root, *or* booting with maxcpus=1 and trying
> to
> > > setuid from a non-root user to root, both fail.
> >
> > Sorry, misstated that.
> >
> > setuid from nonroot->root, or with maxcpus=1, always seems to work.
> >
> > Only multiple cores with setuid to root has failed for us.
>
> Here's a shot in the dark: I may be misreading things, and I don't
> quite see how it fits with the finer details you mention here; but
> it looks to me as if /proc/*/cwd and /proc/*/root lookup interferes
> with the fs->count check in fs/exec.c's unsafe_exec().
>
> If you would, please give this patch against 2.6.28* a try (applies
> to 2.6.29-rc too, but not to 2.6.24*), to see if it makes any
> difference to you. I'm hoping not to hear from you for a while!
>
> (I assume it's okay to read_lock fs->lock while holding task_lock:
> I didn't see anywhere else doing so, but lockdep hasn't objected
> yet.)
>
> Hugh
Hugh...
Thanks for the attention! This didn't seem to fix our problem
(surprisingly) since it does seem to fit with the finer details:
1) The software load we were running it on does a health check every few minutes
which, among other things, executes several lsof and ss (sockstat) processes.
2) In security/commoncap.c, the code:
void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
{
if (bprm->e_uid != current->uid || bprm->e_gid != current->gid ||
!cap_issubset(bprm->cap_post_exec_permitted,
current->cap_permitted)) {
set_dumpable(current->mm, suid_dumpable);
current->pdeath_signal = 0;
if (unsafe & ~LSM_UNSAFE_PTRACE_CAP) {
if (!capable(CAP_SETUID)) {
bprm->e_uid = current->uid;
bprm->e_gid = current->gid;
}
if (cap_limit_ptraced_target()) {
bprm->cap_post_exec_permitted = cap_intersect(
bprm->cap_post_exec_permitted,
current->cap_permitted);
}
}
}
.....
Looks like it would fail because of that (is the ~LSM_UNSAFE_PTRACE_CAP
actually the intended condition? It wasn't clear either way for me, due to
the lack of comments).
I could not reproduce the problem without our system-health-monitor process,
or on several other machines at home (Ubuntu 8.04 and Ubuntu 8.10 with updated
kernels, running multicore). So I am very suspicious of that race, although your
patch didn't seem to fix it.... (?!?!)
Thanks,
Joe Malicki
P.S. Michael Itz did a lot of work related to this issue, and managed to narrow
it down quite a bit, and I feel guilty putting a lot out there without mentioning that.
On Mon, 2 Mar 2009, Joe Malicki wrote:
> ----- "Hugh Dickins" <[email protected]> wrote:
> > On Thu, 26 Feb 2009, Joe Malicki wrote:
> > >
> > > > Very rarely, we experience a setuid program not properly getting
> > > > the euid of its owner.
> >
> > Here's a shot in the dark: I may be misreading things, and I don't
> > quite see how it fits with the finer details you mention here; but
> > it looks to me as if /proc/*/cwd and /proc/*/root lookup interferes
> > with the fs->count check in fs/exec.c's unsafe_exec().
>
> Thanks for the attention! This didn't seem to fix our problem
> (surprisingly) since it does seem to fit with the finer details:
I'm sorry if I've wasted your time, but I am not surprised now.
I went back to look closer, and the fs->count on /proc/*/{cwd,root}
is merely the most obvious case: files->count is equally vulnerable
to lookups on /proc/*/fd/*, via get_files_struct() calls (but the
third LSM_UNSAFE_SHARE, sighand->count, appears to be of no
interest to /proc, so safe from this point of view).
So I think my patch was seriously incomplete. However, the files->count
case looks a lot harder to fix than the fs->count one. Having started
on this issue, I'd better do my best to come up with a fix to the files
count side of it too, but must give it a little thought and time, and
will need to CC some good people even if I do manage a patch - it's
all too easy to fix this but introduce other more serious security
or data lifetime errors.
It would be nice to offer a preliminary patch which at least confirms
that it is this /proc access which is causing the problem; but I didn't
see how to do that without going all out for a fix. Perhaps I'll have
to compromise on a racy patch just to confirm the issue, we'll see.
>
> 1) The software load we were running it on does a health check every few minutes
> which, among other things, executes several lsof and ss (sockstat) processes.
lsof, yes, that fits exactly (perhaps ss equally but I don't know).
I'm afraid your health check is endangering the health of your system!
But I do think the kernel's unreliable setuid is unacceptable behaviour.
> 2) In security/commoncap.c, the code:
> void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
> {
...
> if (unsafe & ~LSM_UNSAFE_PTRACE_CAP) {
> if (!capable(CAP_SETUID)) {
> bprm->e_uid = current->uid;
> bprm->e_gid = current->gid;
> }
> }
...
>
> Looks like it would fail because of that (is the ~LSM_UNSAFE_PTRACE_CAP
> actually the intended condition? It wasn't clear either way for me, due to
> the lack of comments).
Yes, that's where I believe the transiently wrong result of unsafe_exec()
causes the wrong euid to be set (without even failing the exec). I'm
pretty sure that test is as intended: it is wanting to deal differently
with ptrace and cloned cases; but this is all unfamiliar territory to me.
>
> I could not reproduce the problem without our system-health-monitor process,
> or on several other machines at home (Ubuntu 8.04 and Ubuntu 8.10 with updated
> kernels, running multicore). So I am very suspicious of that race, although your
> patch didn't seem to fix it.... (?!?!)
I didn't manage to reproduce it here myself either,
though perhaps I should have tried on more machines.
>
> Thanks,
> Joe Malicki
>
> P.S. Michael Itz did a lot of work related to this issue, and managed to narrow
> it down quite a bit, and I feel guilty putting a lot out there without mentioning that.
Many thanks to the both of you: narrowing such things down is hard work,
but I do think you've made a very interesting and worthwhile discovery.
I'll get back to you... but not immediately.
Hugh
----- "Hugh Dickins" <[email protected]> wrote:
> >
> > Thanks for the attention! This didn't seem to fix our problem
> > (surprisingly) since it does seem to fit with the finer details:
>
> I'm sorry if I've wasted your time, but I am not surprised now.
Oh, not at all! We're glad to help you out since we have a platform
that can reproduce, it's not that much work at this point to test a
patch (given we've already got a minimal reproduction case etc.)
> I went back to look closer, and the fs->count on /proc/*/{cwd,root}
> is merely the most obvious case: files->count is equally vulnerable
> to lookups on /proc/*/fd/*, via get_files_struct() calls (but the
> third LSM_UNSAFE_SHARE, sighand->count, appears to be of no
> interest to /proc, so safe from this point of view).
Good catch, I missed that (I had trouble tracking down everything
involved in /proc - I was looking for that case but overlooked it).
> So I think my patch was seriously incomplete. However, the
> files->count
> case looks a lot harder to fix than the fs->count one. Having
> started
> on this issue, I'd better do my best to come up with a fix to the
> files
> count side of it too, but must give it a little thought and time, and
> will need to CC some good people even if I do manage a patch - it's
> all too easy to fix this but introduce other more serious security
> or data lifetime errors.
>
> It would be nice to offer a preliminary patch which at least confirms
> that it is this /proc access which is causing the problem; but I
> didn't
> see how to do that without going all out for a fix. Perhaps I'll
> have
> to compromise on a racy patch just to confirm the issue, we'll see.
I suppose we can test by ignoring the files->count for LSM_UNSAFE_SHARE
(it doesn't prove it's /proc, but at least narrows things down somewhat).
> >
> > 1) The software load we were running it on does a health check every
> few minutes
> > which, among other things, executes several lsof and ss
> (sockstat) processes.
>
> lsof, yes, that fits exactly (perhaps ss equally but I don't know).
>
> I'm afraid your health check is endangering the health of your
> system!
> But I do think the kernel's unreliable setuid is unacceptable
> behaviour.
The irony!
> >
> > I could not reproduce the problem without our system-health-monitor
> process,
> > or on several other machines at home (Ubuntu 8.04 and Ubuntu 8.10
> with updated
> > kernels, running multicore). So I am very suspicious of that race,
> although your
> > patch didn't seem to fix it.... (?!?!)
>
> I didn't manage to reproduce it here myself either,
> though perhaps I should have tried on more machines.
I suspect it is something subtle about our workload that we haven't
entirely isolated (merely running lsof in a loop oddly doesn't seem
sufficient...)
> I'll get back to you... but not immediately.
>
> Hugh
Given that this bug occurs exceedingly rarely "in the wild" outside of
our minimal test case, a delay isn't a concern.
Thanks!
Joe Malicki