setfs[ug]id syscall return value and include/linux/security.h question

setfs[ug]id syscall return value and include/linux/security.h question

[Jakub Jelinek]

Hi!

Before include/linux/security.h was added, setfsuid/setfsgid always returned
old_fsuid, no matter if the fsuid was actually changed or not.
With the default security ops it seems to do the same, because both
security_task_setuid and security_task_post_setuid return 0, but these are
hooks which seem to return 0 on success, -errno on failure, so if some
non-default security hook is installed and ever returns -errno
in setfsuid/setfsgid, -errno will be returned from the syscall instead
of the expected old_fsuid. This makes it hard to distinguish uids
0xfffff001 .. 0xffffffff from errors of security hooks.
Shouldn't sys_setfsuid/sys_setfsgid be changed:

--- linux-2.5.66/kernel/sys.c.jj	Mon Mar 24 23:00:00 2003
+++ linux-2.5.66/kernel/sys.c	Thu Mar 27 00:11:20 2003
@@ -824,13 +824,11 @@ asmlinkage long sys_getresgid(gid_t *rgi
 asmlinkage long sys_setfsuid(uid_t uid)
 {
 	int old_fsuid;
-	int retval;
-
-	retval = security_task_setuid(uid, (uid_t)-1, (uid_t)-1, LSM_SETID_FS);
-	if (retval)
-		return retval;
 
 	old_fsuid = current->fsuid;
+	if (security_task_setuid(uid, (uid_t)-1, (uid_t)-1, LSM_SETID_FS))
+		return old_fsuid;
+
 	if (uid == current->uid || uid == current->euid ||
 	    uid == current->suid || uid == current->fsuid || 
 	    capable(CAP_SETUID))
@@ -843,9 +841,7 @@ asmlinkage long sys_setfsuid(uid_t uid)
 		current->fsuid = uid;
 	}
 
-	retval = security_task_post_setuid(old_fsuid, (uid_t)-1, (uid_t)-1, LSM_SETID_FS);
-	if (retval)
-		return retval;
+	security_task_post_setuid(old_fsuid, (uid_t)-1, (uid_t)-1, LSM_SETID_FS);
 
 	return old_fsuid;
 }
@@ -856,13 +852,11 @@ asmlinkage long sys_setfsuid(uid_t uid)
 asmlinkage long sys_setfsgid(gid_t gid)
 {
 	int old_fsgid;
-	int retval;
-
-	retval = security_task_setgid(gid, (gid_t)-1, (gid_t)-1, LSM_SETID_FS);
-	if (retval)
-		return retval;
 
 	old_fsgid = current->fsgid;
+	if (security_task_setgid(gid, (gid_t)-1, (gid_t)-1, LSM_SETID_FS))
+		return old_fsgid;
+
 	if (gid == current->gid || gid == current->egid ||
 	    gid == current->sgid || gid == current->fsgid || 
 	    capable(CAP_SETGID))

	Jakub

Re: setfs[ug]id syscall return value and include/linux/security.h question

[David Wagner]

Jakub Jelinek  wrote:
>Before include/linux/security.h was added, setfsuid/setfsgid always returned
>old_fsuid, no matter if the fsuid was actually changed or not.

Out of curiousity:

Do you have any idea why setfsuid() returns the old fsuid, rather than
0 or -EPERM like all the other set*id() calls?

I find it mysterious that setfsuid()'s interface is so different.
It is also strange that setfsuid() has no way to indicate whether the
call failed or succeeded.  Does this inconsistency with the rest of the
set*id() interface strike anyone else as a little odd?

It is also mysterious that there is no getfsuid() call.  One has to use
/proc to find this information.  Do you have any idea why the fsuid/fsgid
interface was designed this way?  Is this an old kludge that we now have
to live with, was it designed this way for a reason, or do we have the
opportunity to fix the semantics of the interface?

Re: setfs[ug]id syscall return value and include/linux/security.h question

[Chris Wright]

* David Wagner (daw@mozart.cs.berkeley.edu) wrote:
> Jakub Jelinek  wrote:
> >Before include/linux/security.h was added, setfsuid/setfsgid always returned
> >old_fsuid, no matter if the fsuid was actually changed or not.
> 
> Out of curiousity:
> 
> Do you have any idea why setfsuid() returns the old fsuid, rather than
> 0 or -EPERM like all the other set*id() calls?

I agree, it seems odd.

> I find it mysterious that setfsuid()'s interface is so different.
> It is also strange that setfsuid() has no way to indicate whether the
> call failed or succeeded.  Does this inconsistency with the rest of the
> set*id() interface strike anyone else as a little odd?

You're not alone ;-)  Even the manpage suggests this is a bug.

> It is also mysterious that there is no getfsuid() call.  One has to use
> /proc to find this information.  Do you have any idea why the fsuid/fsgid
> interface was designed this way?  Is this an old kludge that we now have
> to live with, was it designed this way for a reason, or do we have the
> opportunity to fix the semantics of the interface?

I can't comment on the history of the interface.  While it's Linux
specific, I'm not sure of the legacy impact of changing the semantics of
the current interface.  Ugh.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: setfs[ug]id syscall return value and include/linux/security.h question

[Chris Wright]

* Jakub Jelinek (jakub@redhat.com) wrote:
> Hi!
> 
> Before include/linux/security.h was added, setfsuid/setfsgid always returned
> old_fsuid, no matter if the fsuid was actually changed or not.
> With the default security ops it seems to do the same, because both
> security_task_setuid and security_task_post_setuid return 0, but these are
> hooks which seem to return 0 on success, -errno on failure, so if some
> non-default security hook is installed and ever returns -errno
> in setfsuid/setfsgid, -errno will be returned from the syscall instead
> of the expected old_fsuid. This makes it hard to distinguish uids
> 0xfffff001 .. 0xffffffff from errors of security hooks.
> Shouldn't sys_setfsuid/sys_setfsgid be changed:

Yes, thanks for the patch.  I'll apply this to the LSM tree and push to
Linus with the next batch of changes.  It's unfortunate that the
sys_setfs[ug]id interface can't report an error.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net