[PATCH] Prevent setting 32 uids/gids in the error range

[PATCH] Prevent setting 32 uids/gids in the error range

[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.

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.

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
--- linux-2.5.61-bk1/include/linux/types.h	2003-02-11 14:26:19.000000000 +1100
+++ working-2.5.61-bk1-valid_ids/include/linux/types.h	2003-02-17 17:26:38.000000000 +1100
@@ -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;
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
--- linux-2.5.61-bk1/kernel/sys.c	2003-02-17 11:37:55.000000000 +1100
+++ working-2.5.61-bk1-valid_ids/kernel/sys.c	2003-02-17 18:11:01.000000000 +1100
@@ -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;

Re: [PATCH] Prevent setting 32 uids/gids in the error range

[Alan Cox]

On Mon, 2003-02-17 at 07:41, Rusty Russell wrote:
> 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

Re: [PATCH] Prevent setting 32 uids/gids in the error range

[Alan Cox]

On Mon, 2003-02-17 at 14:55, Alan Cox wrote:
> On Mon, 2003-02-17 at 07:41, Rusty Russell wrote:
> > 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.

Re: [PATCH] Prevent setting 32 uids/gids in the error range

[Rusty Russell]

In message <1045493720.19397.4.camel@irongate.swansea.linux.org.uk> you write:
> On Mon, 2003-02-17 at 07:41, Rusty Russell wrote:
> > 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.

Re: [PATCH] Prevent setting 32 uids/gids in the error range

[tridge]

> 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.

In case anyone wants it, a simple test program is at
 http://samba.org/ftp/unpacked/junkcode/getegid.c