Prevent setting 32 uids/gids in the error range
PATCH 1/1 Prevent setting 32 uids/gids in the error range
Tridge noticed that getegid() was returning EPERM.
I used -1000 since that's what PTR_ERR uses, but i386 _syscall macros use -125: I don't suppose it really matters.
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
Name: User and Group ID range check patch
Author: Rusty Russell
Status: Tested on 2.5.61-bk1
D: sys_getuid etc return long, which means that setting your gid to -1
D: on 32-bit platforms causes sys_getuid to return "EPERM", for example.
include/linux/types.h +4 -0
@@ -34,6 +34,10 @@ typedef __kernel_gid32_t gid_t;typedef __kernel_uid16_t uid16_t ;typedef __kernel_gid16_t gid16_t ; +/* If we allowed these to be set, getuid etc. would break (they return long) */ +#define is_valid_gid (gid) ((unsigned long)(gid) < -1000UL) +#define is_valid_uid (uid) ((unsigned long)(uid) < -1000UL) +#ifdef CONFIG_UID16 /* This is defined by include/asm-{arch}/posix_types.h */ typedef __kernel_old_uid_t old_uid_t ;
kernel/sys.c +23 -1
@@ -658,6 +658,9 @@ asmlinkage long sys_setuid(uid_t uid)if (retval)return retval; +if (!is_valid_uid (uid)) +return -EINVAL ; + old_ruid = new_ruid = current->uid; old_suid = current->suid; new_suid = old_suid;
@@ -700,6 +705,11 @@ asmlinkage long sys_setresuid(uid_t ruidif (retval)return retval; +if ((!is_valid_uid (ruid) && ruid != (uid_t)-1 ) + || (!is_valid_uid (euid) && euid != (uid_t)-1 ) + || (!is_valid_uid (suid) && suid != (uid_t)-1 )) +return -EINVAL ; +if (!capable (CAP_SETUID )) {if ((ruid != (uid_t) -1 ) && (ruid != current->uid) && (ruid != current->euid) && (ruid != current->suid))
@@ -752,6 +762,11 @@ asmlinkage long sys_setresgid(gid_t rgidif (retval)return retval; +if ((!is_valid_gid (rgid) && rgid != (gid_t)-1 ) + || (!is_valid_gid (egid) && egid != (gid_t)-1 ) + || (!is_valid_gid (sgid) && sgid != (gid_t)-1 )) +return -EINVAL ; +if (!capable (CAP_SETGID )) {if ((rgid != (gid_t) -1 ) && (rgid != current->gid) && (rgid != current->egid) && (rgid != current->sgid))
@@ -806,6 +821,9 @@ asmlinkage long sys_setfsuid(uid_t uid)if (retval)return retval; +if (!is_valid_uid (uid)) +return -EINVAL ; + old_fsuid = current->fsuid; if (uid == current->uid || uid == current->euid || uid == current->suid || uid == current->fsuid ||
@@ -838,6 +856,9 @@ asmlinkage long sys_setfsgid(gid_t gid)if (retval)return retval; +if (!is_valid_gid (gid)) +return -EINVAL ; + old_fsgid = current->fsgid; if (gid == current->gid || gid == current->egid || gid == current->sgid || gid == current->fsgid ||
@@ -1059,7 +1080,7 @@ asmlinkage long sys_getgroups(int gidsetasmlinkage long sys_setgroups (int gidsetsize,gid_t *grouplist) {gid_t groups[NGROUPS ]; -int retval; +int retval, i;if (!capable (CAP_SETGID ))return -EPERM ;
@@ -1067,6 +1088,9 @@ asmlinkage long sys_setgroups(int gidsetreturn -EINVAL ;if (copy_from_user (groups, grouplist, gidsetsize *sizeof (gid_t)))return -EFAULT ; +for (i =0 ; i < gidsetsize; i++) +if (!is_valid_gid (groups[i])) +return -EINVAL ; retval =security_task_setgroups (gidsetsize, groups);if (retval)return retval;
Quoted 4 lines · Rusty Russell
Tridge noticed that
getegid()was returning EPERM.I used -1000 since that's what
PTR_ERRuses, but i386_syscallmacros use -125: I don't suppose it really matters.
Thats a bug in the interface. getegid/getgid/setegid/setuid() is not permitted to fail. If libc is setting errno and returning -1 the libc wrapper is wrong.
set*id can fail, but unlike the get functions they do not return a uid but an error code.
Alan
Quoted 7 lines · Alan Cox
Quoted 4 lines · Rusty Russell
Tridge noticed that
getegid()was returning EPERM.I used -1000 since that's what
PTR_ERRuses, but i386_syscallmacros use -125: I don't suppose it really matters.Thats a bug in the interface. getegid/getgid/setegid/setuid() is not permitted to fail.
I meant geteuid/getuid not set* - set* can fail.
In message <[email protected]> you write:
Quoted 8 lines · Rusty Russell
Quoted 4 lines
Tridge noticed that
getegid()was returning EPERM.I used -1000 since that's what
PTR_ERRuses, but i386_syscallmacros use -125: I don't suppose it really matters.Thats a bug in the interface. getegid/getgid/setegid/setuid() is not permitted to fail. If libc is setting errno and returning -1 the libc wrapper is wrong.
OK, thanks. Note that out asm-i386/unistd.h syscallX macros do it wrong, too.
AFAICT, glibc (2.3.1) gets this wrong, and interprets 0xffffffff return from geteuid() as an error (ie. it's not just an strace bug). Tested by Tridge.
Paulus points out that this also means special handling on archs (PPC and PPC64 that I know of) which set a condition code on error: this can be sidestepped in glibc, as well.
Other archs beware.
Thanks for the clarification,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
Quoted 3 lines
AFAICT, glibc (2.3.1) gets this wrong, and interprets
0xffffffffreturn fromgeteuid()as an error (ie. it's not just an strace bug). Tested by Tridge.
In case anyone wants it, a simple test program is at
http://samba.org/ftp/unpacked/junkcode/getegid.c