Fixing setfsuid/setfsgid

Fixing setfsuid/setfsgid

[Florian Weimer]

At present, setfsuid and setfsgid return the same value on success and 
on error, so it is rather difficult to check for errors.  Since the 
userns changes at least, failure can not only be caused by lack of the 
CAP_SETUID capability, but also by an out-of-memory situation.

* Introduce new system calls setfsuid1, fetfsgid1 which have the usual 
"return -errno on failure, 0 on success" semantics.

* Introduce getfsuid, getfsgid, so that applications can check for 
failure by noting that the fs?id did not change.  These system calls 
might have other uses as well.  Emulation in glibc by parsing 
/proc/self/status is possible.

* Don't bother with fs?id at all anymore and attach effective security 
information to descriptors, which are then inherited by the *at 
functions.  This is far more invasive, but would solve the 
multi-threading ambiguity and could be reasonably extended to umask and 
security contexts.  This would need new system calls fsetuid, fsetgid, 
and probably socketat (for bind/connect) and perhaps socketpairat.

Comments?

-- 
Florian Weimer / Red Hat Product Security Team

Re: Fixing setfsuid/setfsgid

[Jeff Layton]

On Wed, 22 Jan 2014 13:40:46 +0100
Florian Weimer <fweimer@redhat.com> wrote:

> At present, setfsuid and setfsgid return the same value on success and 
> on error, so it is rather difficult to check for errors.  Since the 
> userns changes at least, failure can not only be caused by lack of the 
> CAP_SETUID capability, but also by an out-of-memory situation.
>

It is awkward, but how is it ambiguous? If you get back the same value
you passed in, then you know that nothing changed. I guess it could be
an issue if you pass in a fsuid that's equivalent to the current one though.
 
> * Introduce new system calls setfsuid1, fetfsgid1 which have the usual 
> "return -errno on failure, 0 on success" semantics.
>
> * Introduce getfsuid, getfsgid, so that applications can check for 
> failure by noting that the fs?id did not change.  These system calls 
> might have other uses as well.  Emulation in glibc by parsing 
> /proc/self/status is possible.
> 

#2 sounds quite reasonable. It's simple, and having a setfsuid()
without a way to definitively fetch the current value is dumb.

> * Don't bother with fs?id at all anymore and attach effective security 
> information to descriptors, which are then inherited by the *at 
> functions.  This is far more invasive, but would solve the 
> multi-threading ambiguity and could be reasonably extended to umask and 
> security contexts.  This would need new system calls fsetuid, fsetgid, 
> and probably socketat (for bind/connect) and perhaps socketpairat.
> 

(cc'ing Jim Lieb)

I know that Jim had been working on something along these lines, but I
don't know the current state of that work...

-- 
Jeff Layton <jlayton@redhat.com>

Re: Fixing setfsuid/setfsgid

[Florian Weimer]

On 01/22/2014 02:56 PM, Jeff Layton wrote:
> On Wed, 22 Jan 2014 13:40:46 +0100
> Florian Weimer <fweimer@redhat.com> wrote:
>
>> At present, setfsuid and setfsgid return the same value on success and
>> on error, so it is rather difficult to check for errors.  Since the
>> userns changes at least, failure can not only be caused by lack of the
>> CAP_SETUID capability, but also by an out-of-memory situation.
>>
>
> It is awkward, but how is it ambiguous? If you get back the same value
> you passed in, then you know that nothing changed.

It returns the old value, not the new value, just like umask.

>> * Introduce new system calls setfsuid1, fetfsgid1 which have the usual
>> "return -errno on failure, 0 on success" semantics.
>>
>> * Introduce getfsuid, getfsgid, so that applications can check for
>> failure by noting that the fs?id did not change.  These system calls
>> might have other uses as well.  Emulation in glibc by parsing
>> /proc/self/status is possible.
>>
>
> #2 sounds quite reasonable. It's simple, and having a setfsuid()
> without a way to definitively fetch the current value is dumb.

Yes, it's certainly simple, and we can try to do some magic in glibc to 
deprecate looking at the return value of setfsuid.  getumask seems to 
missing as well.

>> * Don't bother with fs?id at all anymore and attach effective security
>> information to descriptors, which are then inherited by the *at
>> functions.  This is far more invasive, but would solve the
>> multi-threading ambiguity and could be reasonably extended to umask and
>> security contexts.  This would need new system calls fsetuid, fsetgid,
>> and probably socketat (for bind/connect) and perhaps socketpairat.
>>
>
> (cc'ing Jim Lieb)
>
> I know that Jim had been working on something along these lines, but I
> don't know the current state of that work...

Interesting, thanks.

-- 
Florian Weimer / Red Hat Product Security Team

Re: Re: Fixing setfsuid/setfsgid

[Jim Lieb]

On Wednesday, January 22, 2014 15:06:01 Florian Weimer wrote:
> On 01/22/2014 02:56 PM, Jeff Layton wrote:
> > On Wed, 22 Jan 2014 13:40:46 +0100
> > 
> > Florian Weimer <fweimer@redhat.com> wrote:
> >> At present, setfsuid and setfsgid return the same value on success and
> >> on error, so it is rather difficult to check for errors.  Since the
> >> userns changes at least, failure can not only be caused by lack of the
> >> CAP_SETUID capability, but also by an out-of-memory situation.
> > 
> > It is awkward, but how is it ambiguous? If you get back the same value
> > you passed in, then you know that nothing changed.
> 
> It returns the old value, not the new value, just like umask.
> 
> >> * Introduce new system calls setfsuid1, fetfsgid1 which have the usual
> >> "return -errno on failure, 0 on success" semantics.
> >> 
> >> * Introduce getfsuid, getfsgid, so that applications can check for
> >> failure by noting that the fs?id did not change.  These system calls
> >> might have other uses as well.  Emulation in glibc by parsing
> >> /proc/self/status is possible.
> > 
> > #2 sounds quite reasonable. It's simple, and having a setfsuid()
> > without a way to definitively fetch the current value is dumb.
> 
> Yes, it's certainly simple, and we can try to do some magic in glibc to
> deprecate looking at the return value of setfsuid.  getumask seems to
> missing as well.
> 
> >> * Don't bother with fs?id at all anymore and attach effective security
> >> information to descriptors, which are then inherited by the *at
> >> functions.  This is far more invasive, but would solve the
> >> multi-threading ambiguity and could be reasonably extended to umask and
> >> security contexts.  This would need new system calls fsetuid, fsetgid,
> >> and probably socketat (for bind/connect) and perhaps socketpairat.
> > 
> > (cc'ing Jim Lieb)
> > 
> > I know that Jim had been working on something along these lines, but I
> > don't know the current state of that work...
> 
> Interesting, thanks.
It is still in the works although it has been delayed while I got V2.0 of nfs-
ganesha out and, of course, the holidays.  I plan to get back to it as soon as 
my current nfs-ganesha dev task is done.

I proposed a switch_creds syscall in mid-Oct (a couple of versions).  The last 
round ended with Tetso Handa pointing out a BUG() issue with override_creds in 
that it is possible to trigger the BUG() in between a switch_creds and its 
revert.  This only affects the thread that does this call but I have to get 
back to it and chase down all the cases.  An override/revert can only work 
presently inside the kernel for a single syscall that uses them.  Ultimately I 
don't think it will be a problem.  All the in-kernel cases are covered now and 
I just need to identify which syscalls are affected and whether the BUG() can 
be turned into an error return that doesn't break backwards compatibility.

You are right.  These syscalls are a bit strange and certainly not what we 
need for nfs-ganesha and probably for samba (and others) as well.  Error 
handling is at best "by inference" aka crap.  I'm not really interested in 
another set of syscalls unless we can fix some other problems as well.

1. Setting fsuid/gid is not enough. We also need to do a setgroups.

2. RCU is great but sloshing thru 6 of these calls (3 in + 3 out) is not good.

3. Using something similar to knfsd's nfsd_setuser() (what these calls do) is 
clean and efficient.  The only current stumbling block is the BUG case above.

4. Actually throw real errors like EPERM instead of fiddling returned uids...

It would be great if we can work together and get something we all need here.
I can send along the current patch set for you to review.  Like I said, I need 
to do more to deal with the BUG() call in commit_creds().

Jim
-- 
Jim Lieb
Linux Systems Engineer
Panasas Inc.

"If ease of use was the only requirement, we would all be riding tricycles"
- Douglas Engelbart 1925–2013
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fixing setfsuid/setfsgid

[Florian Weimer]

On 01/22/2014 07:21 PM, Jim Lieb wrote:

> I proposed a switch_creds syscall in mid-Oct (a couple of versions).

Got it.  The *at-based approach I sketched would achieve roughly the 
same thing.  I don't know how much glibc will like it if you run your 
threads with different effective user IDs, though.

Anyway, this means that for my initial question, adding 
getfsuid/getfsgid seems the answer.

-- 
Florian Weimer / Red Hat Product Security Team