Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1683368 > unrolled thread

[PATCH] exec: Limit arg stack to at most _STK_LIM / 4 * 3

Back to article view | Back to linux.kernel

Contents

  [PATCH] exec: Limit arg stack to at most _STK_LIM / 4 * 3 Kees Cook <keescook@chromium.org> - 2017-07-07 21:00 +0200
    Re: [PATCH] exec: Limit arg stack to at most _STK_LIM / 4 * 3 Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-08 00:30 +0200
      Re: [PATCH] exec: Limit arg stack to at most _STK_LIM / 4 * 3 Kees Cook <keescook@chromium.org> - 2017-07-08 04:50 +0200
    Re: [PATCH] exec: Limit arg stack to at most _STK_LIM / 4 * 3 Michal Hocko <mhocko@kernel.org> - 2017-07-10 15:20 +0200
      Re: [PATCH] exec: Limit arg stack to at most _STK_LIM / 4 * 3 Kees Cook <keescook@chromium.org> - 2017-07-10 17:50 +0200
        Re: [PATCH] exec: Limit arg stack to at most _STK_LIM / 4 * 3 Michal Hocko <mhocko@kernel.org> - 2017-07-10 18:00 +0200
          Re: [PATCH] exec: Limit arg stack to at most _STK_LIM / 4 * 3 Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-10 20:30 +0200
            Re: [PATCH] exec: Limit arg stack to at most _STK_LIM / 4 * 3 Michal Hocko <mhocko@kernel.org> - 2017-07-10 20:40 +0200
    Re: [PATCH] exec: Limit arg stack to at most _STK_LIM / 4 * 3 Ben Hutchings <ben@decadent.org.uk> - 2017-07-10 15:50 +0200
      Re: [PATCH] exec: Limit arg stack to at most _STK_LIM / 4 * 3 Kees Cook <keescook@chromium.org> - 2017-07-10 17:40 +0200

#1683368 — [PATCH] 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).

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/exec.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 904199086490..ddca2cf15f71 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -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;
 	}
 
-- 
2.7.4


-- 
Kees Cook
Pixel Security

[toc] | [next] | [standalone]

#1683445

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jul 7, 2017 at 11:57 AM, Kees Cook <keescook@chromium.org> wrote:
> 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

[toc] | [prev] | [next] | [standalone]

#1683516

On Fri, Jul 7, 2017 at 3:24 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jul 7, 2017 at 11:57 AM, Kees Cook <keescook@chromium.org> wrote:
>> 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?

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

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1684227

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

On Fri 07-07-17 11:57:29, Kees Cook wrote:
> 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?

> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/exec.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 904199086490..ddca2cf15f71 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -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;
>  	}
>  
> -- 
> 2.7.4
> 
> 
> -- 
> Kees Cook
> Pixel Security

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1684373

On Mon, Jul 10, 2017 at 6:13 AM, Michal Hocko <mhocko@kernel.org> wrote:
> 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.

>
> On Fri 07-07-17 11:57:29, Kees Cook wrote:
>> 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.

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

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

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

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1684381

On Mon 10-07-17 08:39:43, Kees Cook wrote:
> On Mon, Jul 10, 2017 at 6:13 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > 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...
 
> > On Fri 07-07-17 11:57:29, Kees Cook wrote:
> >> 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.
> 
> > 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.

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

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1684560

On Mon, Jul 10, 2017 at 8:59 AM, Michal Hocko <mhocko@kernel.org> wrote:
>
> 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

[toc] | [prev] | [next] | [standalone]

#1684564

On Mon 10-07-17 11:24:55, Linus Torvalds wrote:
> On Mon, Jul 10, 2017 at 8:59 AM, Michal Hocko <mhocko@kernel.org> wrote:
> >
> > 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.

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

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1684246

[Multipart message — attachments visible in raw view] — view raw

On Fri, 2017-07-07 at 11:57 -0700, Kees Cook wrote:
> 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 <keescook@chromium.org>
> ---
>  fs/exec.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 904199086490..ddca2cf15f71 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -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))

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.

>  			goto fail;
>  	}
>  
> -- 
> 2.7.4
> 
> 
-- 
Ben Hutchings
Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer

[toc] | [prev] | [next] | [standalone]

#1684357

On Mon, Jul 10, 2017 at 6:44 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Fri, 2017-07-07 at 11:57 -0700, Kees Cook wrote:
>> 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 <keescook@chromium.org>
>> ---
>>  fs/exec.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/exec.c b/fs/exec.c
>> index 904199086490..ddca2cf15f71 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -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))
>
> You're dropping a READ_ONCE(), which I assume is there to guard against
> races with prlimit().  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);
}


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

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web