[PATCH] set*uid() must not fail-and-return on OOM/rlimits

[PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Solar Designer]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

Willy and all,

Attached is a trivial patch (extracted from 2.4.33-ow1) that makes
set*uid() kill the current process rather than proceed with -EAGAIN when
the kernel is running out of memory.  Apparently, alloc_uid() can't fail
and return anyway due to properties of the allocator, in which case the
patch does not change a thing.  But better safe than sorry.

As you're probably aware, 2.6 kernels are affected to a greater extent,
where set*uid() may also fail on trying to exceed RLIMIT_NPROC.  That
needs to be fixed, too.

Opinions are welcome.

Thanks,

Alexander

[-- Attachment #2: linux-2.4.33-ow1-set_user.diff --]
[-- Type: text/plain, Size: 431 bytes --]

diff -urpPX nopatch linux-2.4.33/kernel/sys.c linux/kernel/sys.c
--- linux-2.4.33/kernel/sys.c	Fri Nov 28 21:26:21 2003
+++ linux/kernel/sys.c	Wed Aug 16 05:19:21 2006
@@ -514,8 +514,10 @@ static int set_user(uid_t new_ruid, int 
 	struct user_struct *new_user;
 
 	new_user = alloc_uid(new_ruid);
-	if (!new_user)
+	if (!new_user) {
+		force_sig(SIGSEGV, current);
 		return -EAGAIN;
+	}
 	switch_uid(new_user);
 
 	if(dumpclear)

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Kari Hurtta]

Solar Designer <solar@openwall.com> writes in gmane.linux.kernel

> Attached is a trivial patch (extracted from 2.4.33-ow1) that makes
> set*uid() kill the current process rather than proceed with -EAGAIN when
> the kernel is running out of memory.  Apparently, alloc_uid() can't fail
> and return anyway due to properties of the allocator, in which case the
> patch does not change a thing.  But better safe than sorry.
> 
> As you're probably aware, 2.6 kernels are affected to a greater extent,
> where set*uid() may also fail on trying to exceed RLIMIT_NPROC.  That
> needs to be fixed, too.
> 
> Opinions are welcome.

Perhaps stupid suggestion:

Should there be new signal for 'failure to drop privileges' ?
( perhaps SIGPRIV or is this name free )

By default signal terminates process.  

By setting this to SIG_IGN this allows deamons handle situation when 
becoming to user failed and give proper error message.

Still unaware root processes are killed and not causing privilge escalation.

/ Kari Hurtta

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Alan Cox]

Ar Sul, 2006-08-20 am 10:52 +0300, ysgrifennodd Kari Hurtta:
> Perhaps stupid suggestion:
> 
> Should there be new signal for 'failure to drop privileges' ?
> ( perhaps SIGPRIV or is this name free )
> 
> By default signal terminates process.  

Programs are allowed (and now and then do) intentionally let a setuid
fail. A custom selinux or audit rule might be appropriate but that kind
of hackery is not.

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Kari Hurtta]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> Ar Sul, 2006-08-20 am 10:52 +0300, ysgrifennodd Kari Hurtta:
> > Perhaps stupid suggestion:
> > 
> > Should there be new signal for 'failure to drop privileges' ?
> > ( perhaps SIGPRIV or is this name free )
> > 
> > By default signal terminates process.  
> 
> Programs are allowed (and now and then do) intentionally let a setuid
> fail. A custom selinux or audit rule might be appropriate but that kind
> of hackery is not.

Commented code/patch used SIGKILL.  By allocating new signal programs
_are_ allowed intentionally let a setuid fail.

/ Kari Hurtta

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Willy Tarreau]

On Sun, Aug 20, 2006 at 04:38:40AM +0400, Solar Designer wrote:
> Willy and all,
> 
> Attached is a trivial patch (extracted from 2.4.33-ow1) that makes
> set*uid() kill the current process rather than proceed with -EAGAIN when
> the kernel is running out of memory.  Apparently, alloc_uid() can't fail
> and return anyway due to properties of the allocator, in which case the
> patch does not change a thing.  But better safe than sorry.

Whether it can fail or not, alloc_uid()'s author intent was to report its
problems via NULL :

                new = kmem_cache_alloc(uid_cachep, SLAB_KERNEL);
                if (!new)
                        return NULL;

So your change to set_user() are consistent with this design choice.
Now, chosing to kill the process whe the kernel runs out of memory
seems consistent with what will happen a few milliseconds later to
other processes anyway.

I'm just wondering why you return a SIGSEGV. When the kernel kills
tasks on OOM conditions, it sends either SIGTERM or SIGKILL, as we
can see here in mm/oom_kill.c:__oom_kill_task() :

        p->flags |= PF_MEMALLOC | PF_MEMDIE;
        /* This process has hardware access, be more careful. */
        if (cap_t(p->cap_effective) & CAP_TO_MASK(CAP_SYS_RAWIO)) {
                force_sig(SIGTERM, p);
        } else {
                force_sig(SIGKILL, p);
        }

Shouldn't we simply re-use the same code ? (not the function, I would not
like to get OOM messages outside the OOM killer).

> As you're probably aware, 2.6 kernels are affected to a greater extent,
> where set*uid() may also fail on trying to exceed RLIMIT_NPROC.  That
> needs to be fixed, too.

I've followed the thread a little bit but am not aware of all the details.

> Opinions are welcome.
> 
> Thanks,
> 
> Alexander

What do you (and others) think about this ?
Willy


> diff -urpPX nopatch linux-2.4.33/kernel/sys.c linux/kernel/sys.c
> --- linux-2.4.33/kernel/sys.c	Fri Nov 28 21:26:21 2003
> +++ linux/kernel/sys.c	Wed Aug 16 05:19:21 2006
> @@ -514,8 +514,10 @@ static int set_user(uid_t new_ruid, int 
>  	struct user_struct *new_user;
>  
>  	new_user = alloc_uid(new_ruid);
> -	if (!new_user)
> +	if (!new_user) {
> +		force_sig(SIGSEGV, current);
>  		return -EAGAIN;
> +	}
>  	switch_uid(new_user);
>  
>  	if(dumpclear)

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Solar Designer]

On Sun, Aug 20, 2006 at 10:26:02AM +0200, Willy Tarreau wrote:
> I'm just wondering why you return a SIGSEGV.

I've taken the SIGSEGV from binfmt_elf.c, where it is used on "Unable to
load interpreter", a condition that commonly occurs on OOM.

> When the kernel kills
> tasks on OOM conditions, it sends either SIGTERM or SIGKILL, as we
> can see here in mm/oom_kill.c:__oom_kill_task() :
> 
>         p->flags |= PF_MEMALLOC | PF_MEMDIE;
>         /* This process has hardware access, be more careful. */
>         if (cap_t(p->cap_effective) & CAP_TO_MASK(CAP_SYS_RAWIO)) {
>                 force_sig(SIGTERM, p);
>         } else {
>                 force_sig(SIGKILL, p);
>         }
> 
> Shouldn't we simply re-use the same code ?

I have no objections.

Thanks,

Alexander

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Alex Riesen]

Solar Designer, Sun, Aug 20, 2006 02:38:40 +0200:
> Willy and all,
> 
> Attached is a trivial patch (extracted from 2.4.33-ow1) that makes
> set*uid() kill the current process rather than proceed with -EAGAIN when
> the kernel is running out of memory.  Apparently, alloc_uid() can't fail
> and return anyway due to properties of the allocator, in which case the
> patch does not change a thing.  But better safe than sorry.

Why not ENOMEM?

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Solar Designer]

On Sun, Aug 20, 2006 at 12:07:06PM +0200, Alex Riesen wrote:
> Solar Designer, Sun, Aug 20, 2006 02:38:40 +0200:
> > Attached is a trivial patch (extracted from 2.4.33-ow1) that makes
> > set*uid() kill the current process rather than proceed with -EAGAIN when
> > the kernel is running out of memory.  Apparently, alloc_uid() can't fail
> > and return anyway due to properties of the allocator, in which case the
> > patch does not change a thing.  But better safe than sorry.
> 
> Why not ENOMEM?

ENOMEM would not be any better than EAGAIN from the security standpoint.

The problem is that there are lots of privileged userspace programs that
do not bother to check the return value from set*uid() calls (or
otherwise check that the calls succeeded) before proceeding with work
that is only safe to do with the *uid switched as intended.

Alexander

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Arjan van de Ven]

On Sun, 2006-08-20 at 19:30 +0400, Solar Designer wrote:
> On Sun, Aug 20, 2006 at 12:07:06PM +0200, Alex Riesen wrote:
> > Solar Designer, Sun, Aug 20, 2006 02:38:40 +0200:
> > > Attached is a trivial patch (extracted from 2.4.33-ow1) that makes
> > > set*uid() kill the current process rather than proceed with -EAGAIN when
> > > the kernel is running out of memory.  Apparently, alloc_uid() can't fail
> > > and return anyway due to properties of the allocator, in which case the
> > > patch does not change a thing.  But better safe than sorry.
> > 
> > Why not ENOMEM?
> 
> ENOMEM would not be any better than EAGAIN from the security standpoint.
> 
> The problem is that there are lots of privileged userspace programs that
> do not bother to check the return value from set*uid() calls (or
> otherwise check that the calls succeeded) before proceeding with work
> that is only safe to do with the *uid switched as intended.

sounds like a good argument to get the setuid functions marked
__must_check in glibc...

-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Willy Tarreau]

On Sun, Aug 20, 2006 at 05:53:22PM +0200, Arjan van de Ven wrote:
> On Sun, 2006-08-20 at 19:30 +0400, Solar Designer wrote:
> > On Sun, Aug 20, 2006 at 12:07:06PM +0200, Alex Riesen wrote:
> > > Solar Designer, Sun, Aug 20, 2006 02:38:40 +0200:
> > > > Attached is a trivial patch (extracted from 2.4.33-ow1) that makes
> > > > set*uid() kill the current process rather than proceed with -EAGAIN when
> > > > the kernel is running out of memory.  Apparently, alloc_uid() can't fail
> > > > and return anyway due to properties of the allocator, in which case the
> > > > patch does not change a thing.  But better safe than sorry.
> > > 
> > > Why not ENOMEM?
> > 
> > ENOMEM would not be any better than EAGAIN from the security standpoint.
> > 
> > The problem is that there are lots of privileged userspace programs that
> > do not bother to check the return value from set*uid() calls (or
> > otherwise check that the calls succeeded) before proceeding with work
> > that is only safe to do with the *uid switched as intended.
> 
> sounds like a good argument to get the setuid functions marked
> __must_check in glibc...

Agreed, as I'm sure that I've not always checked it in some of my own
programs. A warning would have helped.

Willy

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]

Arjan van de Ven wrote:
> sounds like a good argument to get the setuid functions marked
> __must_check in glibc...

There are too many false positives.  E.g., in a SUID binaries switching
back from a non-root UID to root will not fail.  Very common.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Arjan van de Ven]

On Sun, 2006-08-20 at 09:28 -0700, Ulrich Drepper wrote:
> Arjan van de Ven wrote:
> > sounds like a good argument to get the setuid functions marked
> > __must_check in glibc...
> 
> There are too many false positives.  E.g., in a SUID binaries switching
> back from a non-root UID to root will not fail.

that is not entirely clear; there is apparently a memory allocation in
this codepath which can fail (the patch in this thread is patching
that).....

>   Very common.
> 
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Michael Buesch]

On Sunday 20 August 2006 18:28, Ulrich Drepper wrote:
> Arjan van de Ven wrote:
> > sounds like a good argument to get the setuid functions marked
> > __must_check in glibc...
> 
> There are too many false positives.  E.g., in a SUID binaries switching
> back from a non-root UID to root will not fail.  Very common.

Well, I would say it clearly depends on the actual kernel
implementation if that can fail or not.
So userspace should really always check.

-- 
Greetings Michael.

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Solar Designer]

> Arjan van de Ven wrote:
> > sounds like a good argument to get the setuid functions marked
> > __must_check in glibc...

I agree.

On Sun, Aug 20, 2006 at 09:28:51AM -0700, Ulrich Drepper wrote:
> There are too many false positives.  E.g., in a SUID binaries switching
> back from a non-root UID to root will not fail.  Very common.

I wouldn't call those false positives.  They're warnings of poorly
written code that might fail with further changes to the kernel or with
custom security modules, or on another Unix-like platform.

Of course, the kernel or security modules must not change the semantics
arbitrarily yet expect old apps to work, however expecting that apps
honor return value from set*[ug]id() would be reasonable.  (The only
reason why it is not is that there are so many broken apps out there and
more are being developed.)

Alexander

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Alan Cox]

Ar Sul, 2006-08-20 am 19:30 +0400, ysgrifennodd Solar Designer:
> The problem is that there are lots of privileged userspace programs that
> do not bother to check the return value from set*uid() calls (or
> otherwise check that the calls succeeded) before proceeding with work
> that is only safe to do with the *uid switched as intended.

People keep saying this but we seem short of current, commonly shipped
examples. And quite frankly any code that doesn't check setuid returns
is unlikely to be fit for purpose in any other way and presumably has
never been adequately audited.

Alan

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Willy Tarreau]

On Sun, Aug 20, 2006 at 07:03:33PM +0100, Alan Cox wrote:
> Ar Sul, 2006-08-20 am 19:30 +0400, ysgrifennodd Solar Designer:
> > The problem is that there are lots of privileged userspace programs that
> > do not bother to check the return value from set*uid() calls (or
> > otherwise check that the calls succeeded) before proceeding with work
> > that is only safe to do with the *uid switched as intended.
> 
> People keep saying this but we seem short of current, commonly shipped
> examples. And quite frankly any code that doesn't check setuid returns
> is unlikely to be fit for purpose in any other way and presumably has
> never been adequately audited.

This is a beginner's bug. It is a common misconception to believe that
because your program is started as root, it will be allowed to switch
to any other uid. People do not always realize that the syscall might
fail (and not on all OSes it seems), resulting in their program still
running with all privileges. I remember having stuffed some
'setuid(getuid())' in some of my programs a long time ago, I don't see
why others would not do the same. I'm not the only dumb person on this
planet :-)

There's an interesting paper about uid transitions here :

  http://seclab.cs.ucdavis.edu/papers/Hao-Chen-papers/usenix02.pdf

Also, for examples of programs affected till recently, look at the
date on this patch (few weeks ago) :

  http://ftp.x.org/pub/X11R7.1/patches/xf86dga-1.0.1-setuid.diff

and this one now (few days ago) :

  http://www.linuxfromscratch.org/patches/downloads/xorg-server/xorg-server-1.1.0-setuid-2.patch

Scary, both X servers are affected...

Now it's not hard to find programs still working like this. Googling
"setuid(getuid())" returns several ones like this :

  http://devel.squid-cache.org/hno/setfilelimit.c

Here, someone proposing to make tcpdump drop privileges :

  http://www.mail-archive.com/tcpdump-workers@sandelman.ottawa.on.ca/msg03170.html

> Alan

So I think that while it's bad code in userland, a misunderstood kernel
semantic caught the developpers. We can at least make the kernel help them.

Regards,
Willy

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Alan Cox]

Ar Sul, 2006-08-20 am 20:10 +0200, ysgrifennodd Willy Tarreau:
> So I think that while it's bad code in userland, a misunderstood kernel
> semantic caught the developpers. We can at least make the kernel help them.

Yeah we could. But unfortunately a competence test with the inability to
write C code isn't part of the Unix spec.

You can help them enormously using the gcc extensions so gcc warns about
any unchecked set*uid call, rather than redesigning expected behaviour
to cause obscure random kills that won't even be noticed/explained.

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Willy Tarreau]

On Sun, Aug 20, 2006 at 07:36:46PM +0100, Alan Cox wrote:
> Ar Sul, 2006-08-20 am 20:10 +0200, ysgrifennodd Willy Tarreau:
> > So I think that while it's bad code in userland, a misunderstood kernel
> > semantic caught the developpers. We can at least make the kernel help them.
> 
> Yeah we could. But unfortunately a competence test with the inability to
> write C code isn't part of the Unix spec.

I know but those programs sometimes ship with distros. How many distros do
not ship with either Xfree86 nor Xorg ?

> You can help them enormously using the gcc extensions so gcc warns about
> any unchecked set*uid call, rather than redesigning expected behaviour
> to cause obscure random kills that won't even be noticed/explained.

Arjan proposed to add a __must_check on the set*uid() function in glibc.
I think that if killing the program is what makes you nervous, we could
at least print a warning in the kernel logs so that the admin of a machine
being abused has a chance to detect what's going on. Would you accept
something like this ?

Willy

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Alan Cox]

Ar Sul, 2006-08-20 am 20:21 +0200, ysgrifennodd Willy Tarreau:
> Arjan proposed to add a __must_check on the set*uid() function in glibc.
> I think that if killing the program is what makes you nervous, we could
> at least print a warning in the kernel logs so that the admin of a machine
> being abused has a chance to detect what's going on. Would you accept
> something like this ?

That ratelimited doesn't sound unreasonable - you want to know its
happening whatever the cause. You could do it with the kernel or with
the audit daemon I guess.

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Willy Tarreau]

On Sun, Aug 20, 2006 at 07:52:59PM +0100, Alan Cox wrote:
> Ar Sul, 2006-08-20 am 20:21 +0200, ysgrifennodd Willy Tarreau:
> > Arjan proposed to add a __must_check on the set*uid() function in glibc.
> > I think that if killing the program is what makes you nervous, we could
> > at least print a warning in the kernel logs so that the admin of a machine
> > being abused has a chance to detect what's going on. Would you accept
> > something like this ?
> 
> That ratelimited doesn't sound unreasonable - you want to know its
> happening whatever the cause. You could do it with the kernel or with
> the audit daemon I guess.

Alan,

2.4 has no printk_ratelimit() function and I'm not sure it's worth adding
one for only this user. One could argue that once it's implemented, we can
uncomment some other warnings that are currently disabled due to lack of
ratelimit.

In this special case (set*uid), the only reason we might fail is because
kmem_cache_alloc(uid_cachep, SLAB_KERNEL) would return NULL. Do you think
it could intentionnally be tricked into failing, or that under OOM we might
bother about the excess of messages ?

If so I can backport the printk_ratelimit() function, I would just like an
advice on this.

Thanks,
Willy

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Alan Cox]

Ar Sul, 2006-08-20 am 21:01 +0200, ysgrifennodd Willy Tarreau:
> 2.4 has no printk_ratelimit() function and I'm not sure it's worth adding
> one for only this user. One could argue that once it's implemented, we can
> uncomment some other warnings that are currently disabled due to lack of
> ratelimit.

Agreed. But if it isnt ratelimited then people will be able to use it
flush other "interesting" log messages out of existance...

> 
> In this special case (set*uid), the only reason we might fail is because
> kmem_cache_alloc(uid_cachep, SLAB_KERNEL) would return NULL. Do you think
> it could intentionnally be tricked into failing, or that under OOM we might
> bother about the excess of messages ?
> 
> If so I can backport the printk_ratelimit() function, I would just like an
> advice on this.

If there are multiple potential users then a backport might be sensible

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Willy Tarreau]

On Sun, Aug 20, 2006 at 08:33:27PM +0100, Alan Cox wrote:
> Ar Sul, 2006-08-20 am 21:01 +0200, ysgrifennodd Willy Tarreau:
> > 2.4 has no printk_ratelimit() function and I'm not sure it's worth adding
> > one for only this user. One could argue that once it's implemented, we can
> > uncomment some other warnings that are currently disabled due to lack of
> > ratelimit.
> 
> Agreed. But if it isnt ratelimited then people will be able to use it
> flush other "interesting" log messages out of existance...
> 
> > 
> > In this special case (set*uid), the only reason we might fail is because
> > kmem_cache_alloc(uid_cachep, SLAB_KERNEL) would return NULL. Do you think
> > it could intentionnally be tricked into failing, or that under OOM we might
> > bother about the excess of messages ?
> > 
> > If so I can backport the printk_ratelimit() function, I would just like an
> > advice on this.
> 
> If there are multiple potential users then a backport might be sensible

Ok, I will proceed that way then. I see at least two places in binfmt_elf :

   631                  if ((interpreter_type & INTERPRETER_ELF) &&
   632                       interpreter_type != INTERPRETER_ELF) {
   633                          // FIXME - ratelimit this before re-enabling
   634                          // printk(KERN_WARNING "ELF: Ambiguous type, using ELF\n");
   635                          interpreter_type = INTERPRETER_ELF;
   636                  }


   824                  if (BAD_ADDR(elf_entry)) {
   825                          printk(KERN_ERR "Unable to load interpreter %.128s\n",
   826                                  elf_interpreter);
   827                          force_sig(SIGSEGV, current);
   828                          retval = IS_ERR((void *)elf_entry) ? PTR_ERR((void *)elf_entry) : -ENOEXEC;
   829                          goto out_free_dentry;
   830                  }

The first one might be interesting, while the second one should definitely
be ratelimited or removed.

Thanks,
willy

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Florian Weimer]

* Solar Designer:

> Opinions are welcome.

Distributors have already begun to patch userland to check for error
returns.  Arguably, this is the correct approach, but I fear it takes
far too long to fix all callers.

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Solar Designer]

On Sun, Aug 20, 2006 at 06:04:51PM +0200, Florian Weimer wrote:
> Distributors have already begun to patch userland to check for error
> returns.  Arguably, this is the correct approach, but I fear it takes
> far too long to fix all callers.

My opinion is that both userland apps need to (be patched to) check for
error returns from set*[ug]id() and the kernel must not let these calls
fail-and-return when the caller is appropriately privileged.

Alexander

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Alan Cox]

Ar Sul, 2006-08-20 am 04:38 +0400, ysgrifennodd Solar Designer:
> Willy and all,
> 
> Attached is a trivial patch (extracted from 2.4.33-ow1) that makes
> set*uid() kill the current process rather than proceed with -EAGAIN when
> the kernel is running out of memory.  Apparently, alloc_uid() can't fail
> and return anyway due to properties of the allocator, in which case the
> patch does not change a thing.  But better safe than sorry.

Major behaviour change, non-standards compliant and is just an attempt
to wallpaper over problems. Was rejected by previous maintainers
already.

NAK  (think /usr/games/banner "NAK")

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Solar Designer]

Alan,

Let me argue with you a little bit.  Please do not misinterpret this as
me pushing for this change (or any other change) to be included; I have
no problem maintaining them all in -ow patches.

On Sun, Aug 20, 2006 at 07:14:00PM +0100, Alan Cox wrote:
> Ar Sul, 2006-08-20 am 04:38 +0400, ysgrifennodd Solar Designer:
> > Attached is a trivial patch (extracted from 2.4.33-ow1) that makes
> > set*uid() kill the current process rather than proceed with -EAGAIN when
> > the kernel is running out of memory.  Apparently, alloc_uid() can't fail
> > and return anyway due to properties of the allocator, in which case the
> > patch does not change a thing.  But better safe than sorry.
> 
> Major behaviour change,

Huh?  The code path is hardly even triggerable on 2.4, while 2.2 and
earlier kernels did not even have this "functionality".

> non-standards compliant

Huh?

Are you referring to killing of processes on OOM?  That was in Linux
already, this patch does not introduce it.

As it relates to setuid() in particular, POSIX.1-2001 says:

     The setuid() function shall fail, return -1, and set errno to the
     corresponding value if one or more of the following are true:

     [EINVAL]
             The value of the uid argument is invalid and not supported by
             the implementation.
     [EPERM]                                                                                 The process does not have appropriate privileges and uid does
             not match the real user ID or the saved set-user-ID.

No other error conditions are defined.  No transient errors.  No EAGAIN.
And the language used does not imply that implementation-specific errors
may be returned.

I'd say that the behavior of returning EAGAIN is non-compliant.

> and is just an attempt to wallpaper over problems.

There are two problems: one is the kernel implementing this unsafe
behavior in 2.4 and beyond and the other is userspace apps not
checking return value from set*[ug]id().  In my opinion, both need to
be fixed.

> Was rejected by previous maintainers already.

Oh, I was not aware of that.  I certainly did not submit this before.

In fact, Linus appeared to agree that set*uid() failing on transient
errors is bad (specifically, when discussing RLIMIT_NPROC on 2.6) in the
discussion that occurred on vendor-sec and security at kernel.org a
couple of months back.  He did not mind the RLIMIT_NPROC check on
set*uid() dropped, while my suggestion was to move it to execve(2) (like
it is done in -ow patches under a configurable option).

> NAK  (think /usr/games/banner "NAK")

OK, if you say so.

In another message, you wrote:

> ... redesigning expected behaviour

I'd say that set*uid() returning EAGAIN is unexpected behavior for most
userspace programmers.  It is also non-standards compliant as I have
shown above.

> to cause obscure random kills that won't even be noticed/explained.

Now this makes sense - we should make those kills similar to regular OOM
kills, providing rate-limited messages.

But the kills are needed.  They are more correct and safer than
returning EAGAIN.  An alternative would be to not allocate memory on
set*uid() at all - like we did not in older kernels - but that would
be an inappropriate behavior change for 2.4.

Thanks for your time anyway,

Alexander

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Alan Cox]

Ar Llu, 2006-08-21 am 02:12 +0400, ysgrifennodd Solar Designer:
> Are you referring to killing of processes on OOM?  That was in Linux
> already, this patch does not introduce it.

(pedantic) Only if you have overcommit disabled.

> As it relates to setuid() in particular, POSIX.1-2001 says:
> 
>      The setuid() function shall fail, return -1, and set errno to the
>      corresponding value if one or more of the following are true:
> 
>      [EINVAL]
>              The value of the uid argument is invalid and not supported by
>              the implementation.
>      [EPERM]                                                                                 The process does not have appropriate privileges and uid does
>              not match the real user ID or the saved set-user-ID.
> 
> No other error conditions are defined.  

> I'd say that the behavior of returning EAGAIN is non-compliant.

You are allowed to return other errors. What you must not do is return a
different error for the description described in the text as I
understand it.

> But the kills are needed.  They are more correct and safer than
> returning EAGAIN.  An alternative would be to not allocate memory on
> set*uid() at all - like we did not in older kernels - but that would
> be an inappropriate behavior change for 2.4.

It is certainly an awkward case to get right when setuid code is not
being audited but I still think you are chasing the symptom, and its not
symptom of crap code, so you are not likely to "fix" security. A lot of
BSD code for example doesn't check malloc returns but you don't want an
auto-kill if mmap fails ?

The kill has the advantage that it stops the situation but it may also
be that you kill a program which can handle the case and you create a
new DoS attack (eg against a daemon switching to your uid). The current
situation is not good, the updated situation could be far worse.

The message is important, we want to know it happened in the memory
shortage case anyway.

Containers are also likely to create more such problems.

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Solar Designer]

On Sun, Aug 20, 2006 at 11:51:15PM +0100, Alan Cox wrote:
> A lot of
> BSD code for example doesn't check malloc returns but you don't want an
> auto-kill if mmap fails ?

That's quite different in that things happen to be fail-close anyway:
malloc() returns NULL, a program does not check for that but tries to
access memory via the pointer - and almost definitely crashes.  Yes,
there are special cases when only *(p + large_value) is accessed and
thus there might be misbehavior rather than crash, but those cases are
very uncommon.

> The kill has the advantage that it stops the situation but it may also
> be that you kill a program which can handle the case and you create a
> new DoS attack (eg against a daemon switching to your uid).

I doubt it (speaking of 2.4 and the proposed patch only).  The error
path that I proposed to change from EAGAIN to kill is only potentially
invoked when the kernel is running out of memory so badly that the
entire system is essentially already DoS'ed.

As it relates to fixing 2.6, I would _not_ propose killing the process
when it is about to exceed RLIMIT_NPROC.  Instead, I would propose that
the RLIMIT_NPROC check be removed (to match the behavior of 2.4 and
earlier kernels) or moved to execve (to match -ow patches with this
option enabled).

> The current
> situation is not good, the updated situation could be far worse.

Well, I disagree.

> The message is important, we want to know it happened in the memory
> shortage case anyway.

This I agree with.

> Containers are also likely to create more such problems.

Implementations should be careful to not break expectations of existing
application programs in dangerous ways.

Thanks,

Alexander

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Alan Cox]

Ar Sul, 2006-08-20 am 23:51 +0100, ysgrifennodd Alan Cox:
> being audited but I still think you are chasing the symptom, and its not

Umm s/not/a ...
> symptom of crap code, so you are not likely to "fix" security. A lot of
> BSD code for example doesn't check malloc returns but you don't want an
> auto-kill if mmap fails ?

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Peter Williams]

Alan Cox wrote:
> Ar Llu, 2006-08-21 am 02:12 +0400, ysgrifennodd Solar Designer:
>> Are you referring to killing of processes on OOM?  That was in Linux
>> already, this patch does not introduce it.
> 
> (pedantic) Only if you have overcommit disabled.
> 
>> As it relates to setuid() in particular, POSIX.1-2001 says:
>>
>>      The setuid() function shall fail, return -1, and set errno to the
>>      corresponding value if one or more of the following are true:
>>
>>      [EINVAL]
>>              The value of the uid argument is invalid and not supported by
>>              the implementation.
>>      [EPERM]                                                                                 The process does not have appropriate privileges and uid does
>>              not match the real user ID or the saved set-user-ID.
>>
>> No other error conditions are defined.  
> 
>> I'd say that the behavior of returning EAGAIN is non-compliant.
> 
> You are allowed to return other errors. What you must not do is return a
> different error for the description described in the text as I
> understand it.
> 
>> But the kills are needed.  They are more correct and safer than
>> returning EAGAIN.  An alternative would be to not allocate memory on
>> set*uid() at all - like we did not in older kernels - but that would
>> be an inappropriate behavior change for 2.4.
> 
> It is certainly an awkward case to get right when setuid code is not
> being audited but I still think you are chasing the symptom, and its not
> symptom of crap code, so you are not likely to "fix" security. A lot of
> BSD code for example doesn't check malloc returns but you don't want an
> auto-kill if mmap fails ?
> 
> The kill has the advantage that it stops the situation but it may also
> be that you kill a program which can handle the case and you create a
> new DoS attack (eg against a daemon switching to your uid). The current
> situation is not good, the updated situation could be far worse.
> 
> The message is important, we want to know it happened in the memory
> shortage case anyway.

How about going ahead with the uid change (if the current user is root) 
BUT still return -EAGAIN.  That way programs that ignore the return 
value will at least no longer have root privileges.

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce

Re: [PATCH] set*uid() must not fail-and-return on OOM/rlimits

[Solar Designer]

On Mon, Aug 21, 2006 at 10:23:35AM +1000, Peter Williams wrote:
> How about going ahead with the uid change (if the current user is root) 
> BUT still return -EAGAIN.  That way programs that ignore the return 
> value will at least no longer have root privileges.

That's bad.  It will break legitimate programs that assume that the
UID switch has failed if set*uid() indicates so with its return value.

Alexander