Skip to contentSkip to conversation

Prevent setting 32 uids/gids in the error range

PATCH In review Rusty Russell 3 participants 5 messages Feb 17, 2003 – Feb 18, 2003

lore
Share & tools
lore
#0 lore raw

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
diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5.61-bk1/include/linux/types.h working-2.5.61-bk1-valid_ids/include/linux/types.h
@@ -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
diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5.61-bk1/kernel/sys.c working-2.5.61-bk1-valid_ids/kernel/sys.c
@@ -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 ruid
 	if (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 rgid
 	if (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 gidset
 asmlinkage 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 gidset
 		return -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;
#1 lore raw
Quoted 4 lines · Rusty Russell

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.

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

#2 lore raw
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_ERR uses, but i386 _syscall macros 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.

#3 lore raw
In reply to Alan Cox

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_ERR uses, but i386 _syscall macros 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.