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

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.

Ar Sul, 2006-08-20 am 10:52 +0300, ysgrifennodd Kari Hurtta:

Quoted 6 lines

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.

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

diff -urpPX nopatch linux-2.4.33/kernel/sys.c linux/kernel/sys.c

kernel/sys.c +3 -1

@@ -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)

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 ?

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.

Solar Designer, Sun, Aug 20, 2006 02:38:40 +0200:

Quoted 5 lines

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?

Quoted 7 lines · Alex Riesen

Solar Designer, Sun, Aug 20, 2006 02:38:40 +0200:

Quoted 5 lines

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.

Quoted 1 line

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.

Quoted 12 lines · Solar Designer

Quoted 8 lines · Alex Riesen

Solar Designer, Sun, Aug 20, 2006 02:38:40 +0200:

Quoted 5 lines

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.

Quoted 4 lines

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

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

Quoted 2 lines · Arjan van de Ven

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.

Quoted 2 lines · Arjan van de Ven

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.

Quoted 2 lines · Arjan van de Ven

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.

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.

Ar Sul, 2006-08-20 am 19:30 +0400, ysgrifennodd Solar Designer:

Quoted 4 lines

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

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.

Ar Sul, 2006-08-20 am 20:10 +0200, ysgrifennodd Willy Tarreau:

Quoted 2 lines

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.

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 ?

Ar Sul, 2006-08-20 am 20:21 +0200, ysgrifennodd Willy Tarreau:

Quoted 5 lines

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.

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.

Ar Sul, 2006-08-20 am 21:01 +0200, ysgrifennodd Willy Tarreau:

Quoted 4 lines

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

Quoted 6 lines

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 ?

Quoted 2 lines

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

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.

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.

Ar Sul, 2006-08-20 am 04:38 +0400, ysgrifennodd Solar Designer:

Quoted 5 lines

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")

... redesigning expected behaviour

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

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.

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

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.

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 ?

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.

Ar Llu, 2006-08-21 am 02:12 +0400, ysgrifennodd Solar Designer:

Quoted 2 lines

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.

Quoted 11 lines

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.

Quoted 3 lines

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.

Quoted 4 lines

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.