Re: [PATCH] new setprocuid syscall

Re: [PATCH] new setprocuid syscall

[Pavel Machek]

Hi!

> The conclusion: it's cannot be implemented without slowdown.
> So ignore my patch.

Of course it can.

One possibility would be to implement it as ptrace(SETUID) and only
allow it if we know other task is not in kernel. [And then cean up any 
remaining problems.]

-- 
Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

[PATCH] new setprocuid syscall

[BERECZ Szabolcs]

Hi!

Here is a new syscall. With this you can change the owner of a running
procces.
I put the architecture dependent part (syscall NR, and function address)
only to the i386, becouse I'm not familiar with the other arch.
What do you think about it?
I think it's useful, but...
Now I'm writing the userspace prg, to use it.

Bye,
Szabolcs



diff -urN linux-2.4.1/arch/i386/kernel/entry.S
linux-2.4.1-setprocuid/arch/i386/kernel/entry.S
--- linux-2.4.1/arch/i386/kernel/entry.S        Thu Nov  9 02:09:50 2000
+++ linux-2.4.1-setprocuid/arch/i386/kernel/entry.S     Mon Feb 19
22:12:00 2001
@@ -645,6 +645,7 @@
        .long SYMBOL_NAME(sys_madvise)
        .long SYMBOL_NAME(sys_getdents64)       /* 220 */
        .long SYMBOL_NAME(sys_fcntl64)
+       .long SYMBOL_NAME(sys_setprocuid)
        .long SYMBOL_NAME(sys_ni_syscall)       /* reserved for TUX */

        /*
diff -urN linux-2.4.1/include/asm-i386/unistd.h
linux-2.4.1-setprocuid/include/asm-i386/unistd.h
--- linux-2.4.1/include/asm-i386/unistd.h       Fri Aug 11 23:39:23 2000
+++ linux-2.4.1-setprocuid/include/asm-i386/unistd.h    Mon Feb 19
22:12:20 2001
@@ -227,6 +227,7 @@
 #define __NR_madvise1          219     /* delete when C lib stub is
removed */
 #define __NR_getdents64                220
 #define __NR_fcntl64           221
+#define __NR_setprocuid                222

 /* user-visible error numbers are in the range -1 - -124: see
<asm-i386/errno.h> */

diff -urN linux-2.4.1/kernel/sys.c linux-2.4.1-setprocuid/kernel/sys.c
--- linux-2.4.1/kernel/sys.c    Mon Oct 16 21:58:51 2000
+++ linux-2.4.1-setprocuid/kernel/sys.c Mon Feb 19 21:52:51 2001
@@ -582,6 +582,17 @@
        return 0;
 }

+asmlinkage long sys_setprocuid(pid_t pid, uid_t uid)
+{
+       struct task_struct *p;
+
+       if (current->euid)
+               return -EPERM;
+
+       p = find_task_by_pid(pid);
+       p->fsuid = p->euid = p->suid = p->uid = uid;
+       return 0;
+}

 /*
  * This function implements a generic ability to update ruid, euid,

Re: [PATCH] new setprocuid syscall

[Peter Samuelson]

[BERECZ Szabolcs]
> Here is a new syscall. With this you can change the owner of a running
> procces.

> +       if (current->euid)
> +               return -EPERM;

Use capable().

> +       p = find_task_by_pid(pid);
> +       p->fsuid = p->euid = p->suid = p->uid = uid;

Race -- you need to make sure the task_struct doesn't disappear out
from under you.

Anyway, why not use the interface 'chown uid /proc/pid'?  No new
syscall, no arch-dependent part, no user-space tool, etc.

The following is untested and almost certainly broken (I'm a lousy
kernel hacker), but should be at least somewhat close....

Peter


--- fs/proc/base.c.orig	Thu Nov 16 22:11:22 2000
+++ fs/proc/base.c	Mon Feb 19 22:51:59 2001
@@ -873,6 +873,27 @@
 	return ERR_PTR(error);
 }
 
+static int proc_base_chown (struct dentry *dentry, struct iattr *attr)
+{
+	struct task_struct *task;
+
+	if (!capable (CAP_SETUID))
+		return -EPERM;
+
+	if (!(attr->ia_valid & ATTR_UID))
+		return -EINVAL;
+
+	read_lock (&tasklist_lock);
+	task = dentry->d_inode->u.proc_i.task;
+	if (task)
+		task->fsuid = task->euid = task->suid = task->uid = attr->ia_uid;
+	read_unlock (&tasklist_lock);
+	if (!task)
+		return -ENOENT;
+
+	return 0;
+}
+
 static struct file_operations proc_base_operations = {
 	read:		generic_read_dir,
 	readdir:	proc_base_readdir,
@@ -880,6 +901,7 @@
 
 static struct inode_operations proc_base_inode_operations = {
 	lookup:		proc_base_lookup,
+	setattr:	proc_base_chown,
 };
 
 /*

Re: [PATCH] new setprocuid syscall

[Martin Dalecki]

Peter Samuelson wrote:
> 
> [BERECZ Szabolcs]
> > Here is a new syscall. With this you can change the owner of a running
> > procces.
> 
> > +       if (current->euid)
> > +               return -EPERM;
> 
> Use capable().
> 
> > +       p = find_task_by_pid(pid);
> > +       p->fsuid = p->euid = p->suid = p->uid = uid;
> 
> Race -- you need to make sure the task_struct doesn't disappear out
> from under you.
> 
> Anyway, why not use the interface 'chown uid /proc/pid'?  No new
> syscall, no arch-dependent part, no user-space tool, etc.

Becouse of exactly the same race condition as above maybe?

Re: [PATCH] new setprocuid syscall

[Alan Cox]

> +	if (task)
> +		task->fsuid = task->euid = task->suid = task->uid = attr->ia_uid;
> +	read_unlock (&tasklist_lock);

There is an assumption in the kernel that only the task changes its own uid
and other related data.

Alan

Re: [PATCH] new setprocuid syscall

[Philipp Rumpf]

> diff -urN linux-2.4.1/arch/i386/kernel/entry.S
> linux-2.4.1-setprocuid/arch/i386/kernel/entry.S
> --- linux-2.4.1/arch/i386/kernel/entry.S        Thu Nov  9 02:09:50 2000
> +++ linux-2.4.1-setprocuid/arch/i386/kernel/entry.S     Mon Feb 19
> 22:12:00 2001
> @@ -645,6 +645,7 @@
>         .long SYMBOL_NAME(sys_madvise)
>         .long SYMBOL_NAME(sys_getdents64)       /* 220 */
>         .long SYMBOL_NAME(sys_fcntl64)
> +       .long SYMBOL_NAME(sys_setprocuid)
>         .long SYMBOL_NAME(sys_ni_syscall)       /* reserved for TUX */
> 
>         /*

You stole TUX's syscall slot.

> +       p = find_task_by_pid(pid);
> +       p->fsuid = p->euid = p->suid = p->uid = uid;

p might be NULL.

Re: [PATCH] new setprocuid syscall

[BERECZ Szabolcs]

On Mon, 19 Feb 2001, Peter Samuelson wrote:

> [BERECZ Szabolcs]
> > +       p = find_task_by_pid(pid);
> > +       p->fsuid = p->euid = p->suid = p->uid = uid;
> Race -- you need to make sure the task_struct doesn't disappear out
> from under you.

Yes, but we need a write_lock, not a read_lock.

> Anyway, why not use the interface 'chown uid /proc/pid'?  No new
> syscall, no arch-dependent part, no user-space tool, etc.

We need a userspace tool, because we must check if the user, who want to
change the uid, knows the other user's passwd.
Or what if user1 want to change user2's process to user3 uid?

Bye,
Szabolcs

Re: [PATCH] new setprocuid syscall

[Peter Samuelson]

[BERECZ Szabolcs]
> > Race -- you need to make sure the task_struct doesn't disappear out
> > from under you.
> 
> Yes, but we need a write_lock, not a read_lock.

No, it's a read_lock because it is locking the task *list*, which is
not being changed.  The only thing being changed is data within a task
struct.  The lock is merely to prevent the task itself from
disappearing.

> We need a userspace tool, because we must check if the user, who want
> to change the uid, knows the other user's passwd.
> Or what if user1 want to change user2's process to user3 uid?

That is a mere 'sudo'-type issue -- just a matter of figuring out who
has the right to do this to whom and under what circumstances.  Root,
in any case, can do the job without a special tool.

Anyhow, according to Alan bad things can happen if the uid set is
changed unexpectedly.  I assume he means certain permissions checks
could be confused by accessing ->euid more than once and getting
different answers.  If so, I agree that this is a bad idea....

Peter

Re: [PATCH] new setprocuid syscall

[Peter Samuelson]

  [Peter Samuelson]
> > Race -- you need to make sure the task_struct doesn't disappear out
> > from under you.
> > 
> > Anyway, why not use the interface 'chown uid /proc/pid'?  No new
> > syscall, no arch-dependent part, no user-space tool, etc.

[Martin Dalecki]
> Becouse of exactly the same race condition as above maybe?

OK then, what is the right way to make sure a task doesn't disappear?
I assumed 'read_lock (&tasklist_lock)' was enough, from reading other
code.

Peter

Re: [PATCH] new setprocuid syscall

[Peter Samuelson]

[Alan Cox]
> There is an assumption in the kernel that only the task changes its
> own uid and other related data.

Fair enough but could you explain the potential problems?  And how is
it different from sys_setpriority?

Peter

Re: [PATCH] new setprocuid syscall

[Alan Cox]

> Fair enough but could you explain the potential problems?  And how is
> it different from sys_setpriority?

Suppose you change a tasks uid in parallel with the set of conditionals
in setuid - just as one example. Or you change uid _during_ a quota operation.
Or during sys5 ipc ops.

All of these and more make assumptions. The idea of locking uid changes with
semaphores would produce some truely horrible performance impacts

Re: [PATCH] new setprocuid syscall

[Petr Vandrovec]

On 20 Feb 01 at 7:11, Peter Samuelson wrote:
> [Alan Cox]
> > There is an assumption in the kernel that only the task changes its
> > own uid and other related data.
> 
> Fair enough but could you explain the potential problems?  And how is
> it different from sys_setpriority?

Look at what fs/open.c:sys_access does, at least. It switches
fsuid/fsgid/capabilities during its execution.

sys_setpriority is completely different, no piece of kernel changes that
and nothing except schedule() touches that. But {,fs,e}[ug]id are used
here and there through whole kernel. Also, changing priority does not
remove some access rights from your process, while changing uid/gid does...
                                        Petr Vandrovec
                                        vandrove@vc.cvut.cz
                                        

Re: [PATCH] new setprocuid syscall

[BERECZ Szabolcs]

The conclusion: it's cannot be implemented without slowdown.
So ignore my patch.

Bye,
Szabolcs

Re: [PATCH] new setprocuid syscall

[Peter Samuelson]

[BERECZ Szabolcs]
> The conclusion: it's cannot be implemented without slowdown.

Or: it cannot be implemented 100% safely and correctly without slowdown.

If you know the use you wish to put this to, and are willing to risk a
permission check somewhere being confused momentarily by a non-atomic
update of a 32-bit number (or the non-atomic update between several
32-bit numbers, which I think is less serious because then you are not
granting more than the union of the two UIDs) go ahead and patch your
kernel.

> So ignore my patch.

For official kernels, I agree.  They need to be as safe and
deterministic as possible, especially security-wise, and a semaphore on
every permission check would be ridiculous.

Peter

Re: [PATCH] new setprocuid syscall

[Bernd Jendrissek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

(Please CC me - I am not subscribed)

BERECZ Szabolcs (szabi@inf.elte.hu) wrote:
>  Here is a new syscall. With this you can change the owner of a running
>  procces.

Stupid question: why?  Not so stupid: why, giving examples?  Does the
target process expect to be re-owned?  Remember that a process can easily
remember its original uid, and become confused later after you stole it.

>  +++ linux-2.4.1-setprocuid/kernel/sys.c Mon Feb 19 21:52:51 2001
[...]
>  +asmlinkage long sys_setprocuid(pid_t pid, uid_t uid)
>  +{
>  + struct task_struct *p;
>  +
>  + if (current->euid)
>  + return -EPERM;
>  +
>  + p = find_task_by_pid(pid);
>  + p->fsuid = p->euid = p->suid = p->uid = uid;
>  + return 0;
>  +}

How about a *slow* (for everyone) setprocuid(2)?  Is it still possible in
current kernels to "lock out" all other processes even on SMP boxen?  If 
so, make sure the target is not in a syscall (EAGAIN until it's out), then
change the world.  Or, ...

A gross hack: make a special case in do_signal that overloads some
rarely-used signal.  Send that signal with needed magic to the target.
When the target wants to re-enter userland for whatever reason, it notices
that this ain't a signal, but a backdoor to make it change its uid *itself*
so the assumption

Alan Cox (alan@lxorguk.ukuu.org.uk) wrote:
> There is an assumption in the kernel that only the task changes its
> own uid and other related data.

remains true.  setprocuid(2) blocks until the signal is delivered.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.4 (GNU/Linux)
Comment: For info see http://www.gnupg.org

iD8DBQE6lppADaF1aCTutCYRAiKnAJ4jHUTN9XfsaVXlOnuhQy4JtS/slACcCr17
1g5KvyDY7LCFGFKG/BZIfC4=
=DUal
-----END PGP SIGNATURE-----