set*uid() must not fail-and-return on OOM/rlimits
PATCH 1/1 set*uid() must not fail-and-return on OOM/rlimits
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
kernel/sys.c +3 -1
@@ -514,8 +514,10 @@ static int set_user(uid_t new_ruid, intstruct 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)
Solar Designer <[email protected]> writes in gmane.linux.kernel
Quoted 11 lines
Attached is a trivial patch (extracted from 2.4.33-ow1) that makes set*uid() kill the current process rather than proceed with
-EAGAINwhen 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
Quoted 7 lines · 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
-EAGAINwhen 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).
Quoted 3 lines
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.
Quoted 5 lines
Opinions are welcome.
Thanks,
Alexander
What do you (and others) think about this ?
Willy
Quoted 15 lines
diff -urpPX nopatch
linux-2.4.33/kernel/sys.clinux/kernel/sys.ckernel/sys.c +3
-1@@ -514,8 +514,10 @@ static int set_user(uid_t new_ruid, intstruct 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)
Solar Designer, Sun, Aug 20, 2006 02:38:40 +0200:
Quoted 7 lines
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
-EAGAINwhen 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 1 line · Willy Tarreau
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.
Quoted 12 lines
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); }
Quoted 1 line
Shouldn't we simply re-use the same code ?
I have no objections.
Thanks,
Alexander
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
-EAGAINwhen 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
Quoted 8 lines · Solar Designer
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
-EAGAINwhen 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 8 lines
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.
sounds like a good argument to get the setuid functions marked __must_check in glibc...
Signature · 1 line
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
* Solar Designer:
Quoted 1 line
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.
Quoted 13 lines · Arjan van de Ven
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
-EAGAINwhen 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 7 lines
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_checkin 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
Quoted 3 lines · Florian Weimer
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
Quoted 2 lines · Arjan van de Ven
sounds like a good argument to get the setuid functions marked
__must_checkin 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.
Signature · 1 line
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
Quoted 6 lines · Ulrich Drepper
Quoted 2 lines · Arjan van de Ven
sounds like a good argument to get the setuid functions marked
__must_checkin 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).....
Quoted 2 lines
Very common.
Signature · 1 line
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Quoted 6 lines · Ulrich Drepper
Quoted 2 lines · Arjan van de Ven
sounds like a good argument to get the setuid functions marked
__must_checkin 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.
Signature · 1 line
Greetings Michael.
Quoted 3 lines
Quoted 2 lines · Arjan van de Ven
sounds like a good argument to get the setuid functions marked
__must_checkin glibc...
I agree.
Quoted 2 lines · Ulrich Drepper
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
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
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.
Ar Sul, 2006-08-20 am 04:38 +0400, ysgrifennodd Solar Designer:
Quoted 7 lines
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
-EAGAINwhen 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")
Quoted 10 lines · Alan Cox
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.
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/[email protected]/msg03170.html
Quoted 1 line
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
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.
Quoted 6 lines · Alan Cox
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.
I know but those programs sometimes ship with distros. How many distros do not ship with either Xfree86 nor Xorg ?
Quoted 3 lines
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
Ar Sul, 2006-08-20 am 20:21 +0200, ysgrifennodd Willy Tarreau:
Quoted 5 lines
Arjan proposed to add a
__must_checkon 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.
Quoted 10 lines · Alan Cox
Ar Sul, 2006-08-20 am 20:21 +0200, ysgrifennodd Willy Tarreau:
Quoted 5 lines
Arjan proposed to add a
__must_checkon 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
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 8 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 ?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
Quoted 15 lines · Alan Cox
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 4 lines
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
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
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.
Quoted 8 lines · Alan Cox
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
-EAGAINwhen 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".
Quoted 1 line
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.
Quoted 1 line
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.
Quoted 1 line
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).
Quoted 1 line
NAK (think
/usr/games/banner"NAK")
OK, if you say so.
Quoted 1 line · you
... 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.
Quoted 1 line
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
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.
Containers are also likely to create more such problems.
Ar Sul, 2006-08-20 am 23:51 +0100, ysgrifennodd Alan Cox:
Quoted 1 line
being audited but I still think you are chasing the symptom, and its not
Umm s/not/a ...
Quoted 3 lines
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 ?
Quoted 3 lines · Alan Cox
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.
Quoted 3 lines
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).
Quoted 2 lines
The current
situation is not good, the updated situation could be far worse.
Well, I disagree.
Quoted 2 lines
The message is important, we want to know it happened in the memory shortage case anyway.
This I agree with.
Quoted 1 line
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
Quoted 36 lines · Alan Cox
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 ?
Quoted 7 lines
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
Signature · 4 lines
Peter Williams [email protected] "Learning, n. The kind of ignorance distinguishing the studious." -- Ambrose Bierce
Quoted 3 lines · Peter Williams
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
Quoted 11 lines · Alan Cox
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.
Commented code/patch used SIGKILL. By allocating new signal programs _are_ allowed intentionally let a setuid fail.
/ Kari Hurtta