exec: Limit arg stack to at most _STK_LIM / 4 * 3
Subsystems (2)
PATCH 1/1 exec: Limit arg stack to at most _STK_LIM / 4 * 3
To avoid pathological stack usage or the need to special-case setuid execs, just limit all arg stack usage to at most _STK_LIM / 4 * 3 (6MB).
--- fs/exec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
fs/exec.c +4 -4
@@ -221,7 +221,6 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,if (write) {unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start;unsigned long ptr_size; -struct rlimit *rlim;/* * Since the stack will hold pointers to the strings, we
@@ -250,14 +249,15 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,return page;/* -* Limit to 1/4-th the stack size for the argv+env strings. +* Limit to 1/4 of the max stack size or 3/4 of _STK_LIM +* (whichever is smaller) for the argv+env strings. * This ensures that: * - the remaining binfmt code will not run out of stack space, * - the program will have a reasonable amount of stack left * to work from. */ - rlim = current->signal->rlim; -if (size >READ_ONCE (rlim[RLIMIT_STACK ].rlim_cur) /4 ) + if (size > min_t(unsigned long, rlimit(RLIMIT_STACK) / 4, + _STK_LIM /4 *3 )) goto fail; }
Signature · 6 lines
2.7.4 -- Kees Cook Pixel Security
Quoted 2 lines · Kees Cook
To avoid pathological stack usage or the need to special-case setuid execs, just limit all arg stack usage to at most
_STK_LIM/ 4 * 3 (6MB).
Ok, this I think I should just apply, but would prefer to avoid multi-line complex conditionals around things like this.
So how about the attached slightly edited version instead?
I didn't test it (and I'm not really committing it until I get an ack or two), but it seemed all ObviouslyCorrect(tm). FamousLastWords(tm).
Comments?
Linus
From 4f677953627bc64393bcc310f4f779282a089ae3 Mon Sep 17 00:00:00 2001
From: Kees Cook <[email protected]>
Date: Fri, 7 Jul 2017 11:57:29 -0700
Subject: [PATCH] exec: Limit arg stack to at most 75% of _STK_LIM
To avoid pathological stack usage or the need to special-case setuid execs, just limit all arg stack usage to at most 75% of _STK_LIM (6MB).
--- fs/exec.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
fs/exec.c +6 -5
@@ -220,8 +220,7 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,if (write) {unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; -unsigned long ptr_size; -struct rlimit *rlim; +unsigned long ptr_size, limit;/* * Since the stack will hold pointers to the strings, we
@@ -250,14 +249,16 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,return page;/* -* Limit to 1/4-th the stack size for the argv+env strings. +* Limit to 1/4 of the max stack size or 3/4 of _STK_LIM +* (whichever is smaller) for the argv+env strings. * This ensures that: * - the remaining binfmt code will not run out of stack space, * - the program will have a reasonable amount of stack left * to work from. */ - rlim = current->signal->rlim; -if (size >READ_ONCE (rlim[RLIMIT_STACK ].rlim_cur) /4 ) + limit = _STK_LIM /4 *3 ; + limit =min (limit,rlimit (RLIMIT_STACK ) /4 ); +if (size > limit) goto fail; }
Signature · 1 line
2.13.1.518.g0d864c4df
Quoted 7 lines · Linus Torvalds
Quoted 2 lines · Kees Cook
To avoid pathological stack usage or the need to special-case setuid execs, just limit all arg stack usage to at most
_STK_LIM/ 4 * 3 (6MB).Ok, this I think I should just apply, but would prefer to avoid multi-line complex conditionals around things like this.
Quoted 6 lines
So how about the attached slightly edited version instead?
I didn't test it (and I'm not really committing it until I get an ack or two), but it seemed all ObviouslyCorrect(tm). FamousLastWords(tm).
Comments?
That works for me, thanks. Testing showed the same sane results:
$ ulimit -s 32768 $ ./args Detected max args size near: 6291023 bytes $ ulimit -s 24576 $ ./args Detected max args size near: 6291022 bytes $ ulimit -s 20480 $ ./args Detected max args size near: 5242448 bytes $ ulimit -s 16384 $ ./args Detected max args size near: 4193871 bytes $ ulimit -s 8192 $ ./args Detected max args size near: 2096719 bytes $ ulimit -s 4096 $ ./args Detected max args size near: 1048143 bytes
-Kees
Signature · 2 lines
Kees Cook Pixel Security
I am not sure whether this is still actual because there are just too many pathes flying around these days. I am still trying to catch up...
Quoted 2 lines · Kees Cook
To avoid pathological stack usage or the need to special-case setuid execs, just limit all arg stack usage to at most
_STK_LIM/ 4 * 3 (6MB).
I am worried that we've grown users which rely on a large argument lists and now we are pulling more magic constants into the game. This just calls for another breakage.
I think we should simply step back and think about what we want to fix here actually. If this is the pathological case when the attacker can grow the stack too large and too close to a regular mappings then we already have means to address that (stack gap).
If we are worried that mmaps can get way too close to the stack then I would question why this is possible at all. Bottom-up layout will require consuming mmap space and top-down layout seems just broken because we do not try to offset the mmap_base relative to the stack and rather calculate both from TASK_SIZE. Or at least this is my current undestanding. Am I missing something? Aren't we just trying to fix a bug at a wrong place?
Quoted 43 lines
Signed-off-by Kees Cook <[email protected]>--- fs/exec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)fs/exec.c +4
-4@@ -221,7 +221,6 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,if (write) {unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start;unsigned long ptr_size; -struct rlimit *rlim;/* * Since the stack will hold pointers to the strings, we @@ -250,14 +249,15 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,return page;/* -* Limit to 1/4-th the stack size for the argv+env strings. +* Limit to 1/4 of the max stack size or 3/4 of _STK_LIM +* (whichever is smaller) for the argv+env strings. * This ensures that: * - the remaining binfmt code will not run out of stack space, * - the program will have a reasonable amount of stack left * to work from. */ - rlim = current->signal->rlim; -if (size >READ_ONCE (rlim[RLIMIT_STACK ].rlim_cur) /4 ) + if (size > min_t(unsigned long, rlimit(RLIMIT_STACK) / 4, + _STK_LIM /4 *3 )) goto fail; }Signature · 6 lines
2.7.4 -- Kees Cook Pixel Security
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 2 lines · Michal Hocko
I am not sure whether this is still actual because there are just too many pathes flying around these days. I am still trying to catch up...
Linus applied this one, yes.
Quoted 8 lines · Kees Cook
Quoted 2 lines
To avoid pathological stack usage or the need to special-case setuid execs, just limit all arg stack usage to at most
_STK_LIM/ 4 * 3 (6MB).I am worried that we've grown users which rely on a large argument lists and now we are pulling more magic constants into the game. This just calls for another breakage.
I think it would be best to only apply this to setuid processes, but Linus asked that this change be universal. After my secureexec refactoring, I think it should be possible to add a "how much stack has already been used?" check in setup_new_exec() and abort the privileged exec if it exceeds the secureexec stack limit.
Quoted 4 lines
I think we should simply step back and think about what we want to fix here actually. If this is the pathological case when the attacker can grow the stack too large and too close to a regular mappings then we already have means to address that (stack gap).
I think Linus's intention is to back off from the stack gap, but maybe I misunderstood.
Quoted 7 lines
If we are worried that mmaps can get way too close to the stack then I would question why this is possible at all. Bottom-up layout will require consuming mmap space and top-down layout seems just broken because we do not try to offset the
mmap_baserelative to the stack and rather calculate both fromTASK_SIZE. Or at least this is my current undestanding. Am I missing something? Aren't we just trying to fix a bug at a wrong place?
With a variable stack limit, we'll continue to run risks of gap-jumping if the compiler isn't doing stack probing, so while we might be able to further improve the layout logic, I think we still need to impose limits on setuid programs.
-Kees
Signature · 2 lines
Kees Cook Pixel Security
Quoted 5 lines · Kees Cook
Quoted 2 lines · Michal Hocko
I am not sure whether this is still actual because there are just too many pathes flying around these days. I am still trying to catch up...
Linus applied this one, yes.
Hmm, this is rather rushed...
Quoted 14 lines
Quoted 7 lines · Kees Cook
Quoted 2 lines
To avoid pathological stack usage or the need to special-case setuid execs, just limit all arg stack usage to at most
_STK_LIM/ 4 * 3 (6MB).I am worried that we've grown users which rely on a large argument lists and now we are pulling more magic constants into the game. This just calls for another breakage.
I think it would be best to only apply this to setuid processes, but Linus asked that this change be universal. After my secureexec refactoring, I think it should be possible to add a "how much stack has already been used?" check in
setup_new_exec()and abort the privileged exec if it exceeds the secureexec stack limit.
Quoted 7 lines
Quoted 4 lines
I think we should simply step back and think about what we want to fix here actually. If this is the pathological case when the attacker can grow the stack too large and too close to a regular mappings then we already have means to address that (stack gap).
I think Linus's intention is to back off from the stack gap, but maybe I misunderstood.
We will always need some gap inforcement. 256 pages enforced currently can be loosen after the stack probing is generally spread. But let's be realistic there are people using other (non-distribution) compilers and it would be good to have them covered as well, to some extent at least. Also we might remove the expand_stack enforcement but we will still need to keep a gap for new mmaps. With all that in place I am not really sure what this patch actually prevents from.
Quoted 8 lines
Quoted 7 lines
If we are worried that mmaps can get way too close to the stack then I would question why this is possible at all. Bottom-up layout will require consuming mmap space and top-down layout seems just broken because we do not try to offset the
mmap_baserelative to the stack and rather calculate both fromTASK_SIZE. Or at least this is my current undestanding. Am I missing something? Aren't we just trying to fix a bug at a wrong place?
Quoted 4 lines
With a variable stack limit, we'll continue to run risks of gap-jumping if the compiler isn't doing stack probing, so while we might be able to further improve the layout logic, I think we still need to impose limits on setuid programs.
So how exactly this patch helps if we really enforce the gap between the stack and the mmap base?
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 2 lines · Michal Hocko
We will always need some gap inforcement.
Considering the Java issue, that's rather questionable.
We really can't be breaking libreoffice. That's like a big classic no-no - it affects normal users that simply cannot be expected to work around it. For them, it's a "my office application no longer works" situation, and they just think the system is flaky.
Now, somebody who explicitly raised the stack limit past 24MB and gets bit because he also tries to use more than 6M of arguments - that's actually a different issue. Let's see if anybody ever even complains, and then we might make it a "only for suid binaries" thing.
But honestly, a security limit that isn't tested in normal working is not a security limit at all, it's just theory and likely bullshit. So I'd much rather *not* make it suid-specific if at all possible. That way it has some chance in hell of actually getting tested.
Linus
Quoted 10 lines · Linus Torvalds
Quoted 2 lines · Michal Hocko
We will always need some gap inforcement.
Considering the Java issue, that's rather questionable.
We really can't be breaking libreoffice. That's like a big classic no-no - it affects normal users that simply cannot be expected to work around it. For them, it's a "my office application no longer works" situation, and they just think the system is flaky.
I completely agree that breaking LO is a no-go and we should strive to find a way around it. One way to go is to increase the ulimit in the LO start up script I suspect (this can be done in distributions and work with the upstream LO to be done in future releases). Is it ideal? Not at all! But let's face it, the Java usage of the mapping below the stack is questionable at best (I would dare to call it broken) and I wory to weaken otherwise useful mitigation based on a fishy application.
Quoted 4 lines
Now, somebody who explicitly raised the stack limit past 24MB and gets bit because he also tries to use more than 6M of arguments - that's actually a different issue. Let's see if anybody ever even complains, and then we might make it a "only for suid binaries" thing.
Sure, I was not worried about suid only binaries. If we should restrict the argument/env size then they should behave the same way. Otherwise we just risk oddities. I am just questioning why do we need to cap the thing at all. I simply do not see what additional protection does it give.
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 32 lines · Kees Cook
To avoid pathological stack usage or the need to special-case setuid execs, just limit all arg stack usage to at most
_STK_LIM/ 4 * 3 (6MB).Signed-off-by Kees Cook <[email protected]>---
fs/exec.c | 8 ++++----1 file changed, 4 insertions(+), 4 deletions(-)fs/exec.c +0
-1@@ -221,7 +221,6 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, if (write) { unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; unsigned long ptr_size; -struct rlimit *rlim;
/*
* Since the stack will hold pointers to the strings, wefs/exec.c +0
-0@@ -250,14 +249,15 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, return page;
/*fs/exec.c +2
-1- * Limit to 1/4-th the stack size for the argv+env strings. + * Limit to 1/4 of the max stack size or 3/4 of _STK_LIM + * (whichever is smaller) for the argv+env strings.* This ensures that:
* - the remaining binfmt code will not run out of stack space, * - the program will have a reasonable amount of stack left * to work from.
*/
fs/exec.c static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, · line 260
Quoted 4 lines
fs/exec.c +2
-2- rlim = current->signal->rlim; -if (size >READ_ONCE (rlim[RLIMIT_STACK ].rlim_cur) /4 ) + if (size > min_t(unsigned long, rlimit(RLIMIT_STACK) / 4, + _STK_LIM /4 *3 ))
You're dropping a READ_ONCE(), which I assume is there to guard against races with prlimit(). That should probably be kept.
(When we exec a setuid program, is prlimit() by the real user already blocked at this point? If not then the stack limit could still be reduced so that the stack is full of arguments. But I don't see that this is exploitable, at least not in the same way as very large stacks.)
Ben.
Quoted 7 lines
goto fail;
}
--
2.7.4
Signature · 2 lines
Ben Hutchings Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer
Quoted 24 lines · Ben Hutchings
Quoted 23 lines · Kees Cook
To avoid pathological stack usage or the need to special-case setuid execs, just limit all arg stack usage to at most
_STK_LIM/ 4 * 3 (6MB).Signed-off-by Kees Cook <[email protected]>--- fs/exec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)fs/exec.c +0
-1@@ -221,7 +221,6 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,if (write) {unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start;unsigned long ptr_size; -struct rlimit *rlim;/* * Since the stack will hold pointers to the strings, we @@ -250,14 +249,15 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,return page;
Quoted 16 lines
fs/exec.c static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, · line 260
Quoted 13 lines
fs/exec.c +4
-3/* -* Limit to 1/4-th the stack size for the argv+env strings. +* Limit to 1/4 of the max stack size or 3/4 of _STK_LIM +* (whichever is smaller) for the argv+env strings. * This ensures that: * - the remaining binfmt code will not run out of stack space, * - the program will have a reasonable amount of stack left * to work from. */ - rlim = current->signal->rlim; -if (size >READ_ONCE (rlim[RLIMIT_STACK ].rlim_cur) /4 ) + if (size > min_t(unsigned long, rlimit(RLIMIT_STACK) / 4, + _STK_LIM /4 *3 ))You're dropping a
READ_ONCE(), which I assume is there to guard against races withprlimit(). That should probably be kept.
READ_ONCE() is in the rlimit() helper:
static inline unsigned long task_rlimit(const struct task_struct *tsk,
unsigned int limit)
{
return READ_ONCE(tsk->signal->rlim[limit].rlim_cur);
}static inline unsigned long rlimit(unsigned int limit)
{
return task_rlimit(current, limit);
}Quoted 5 lines
(When we exec a setuid program, is
prlimit()by the real user already blocked at this point? If not then the stack limit could still be reduced so that the stack is full of arguments. But I don't see that this is exploitable, at least not in the same way as very large stacks.)
Hm, prlimit64 lets you do remote tasks and has checks, but prlimit against current has no checks (i.e. current can always set its own rlimits). Additionally, I don't see anything that stops a race with any of the rlimits. (I think Andy mentioned this too.)
In this particular patch, the race doesn't matter since we're bounded by the _STK_LIM calculation, but everywhere else, it does seem to matter...
I think a two threaded process could spin with prlimit() calls while the other thread attempted to do execs()... :(
-Kees
Signature · 2 lines
Kees Cook Pixel Security