exec: Use init rlimits for setuid exec

In an attempt to provide sensible rlimit defaults for setuid execs, this inherits the namespace's init rlimits:

$ ulimit -s
8192
$ ulimit -s unlimited
$ /bin/sh -c 'ulimit -s'
unlimited
$ sudo /bin/sh -c 'ulimit -s'
8192

This is modified from Brad Spengler/PaX Team's hard-coded setuid exec stack rlimit (8MB) in the last public patch of grsecurity/PaX based on my understanding of the code. Changes or omissions from the original code are mine and don't reflect the original grsecurity/PaX code.

Signed-off-by Kees Cook <[email protected]>

---
Instead of copying all rlimits, we could also pick specific ones to copy (e.g. RLIMIT_STACK, or ones from Andy's list) or exclude from copying (probably better to blacklist than whitelist).

I think this is the right way to find the ns init task, but maybe it needs locking?

---
 fs/exec.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

fs/exec.c +0 -0

index 904199086490..80e8b2bd4284 100644

fs/exec.c +5 -0

 /*
  * sys_execve() executes a new program.
  */

@@ -1687,6 +1693,7 @@ static int do_execveat_common(int fd, struct filename *filename,
        struct linux_binprm *bprm;
        struct file *file;
        struct files_struct *displaced;
+       struct rlimit saved_rlim[RLIM_NLIMITS];
        int retval;
 
        if (IS_ERR(filename))

@@ -1771,24 +1778,38 @@ static int do_execveat_common(int fd, struct filename *filename,
        if (retval < 0)
                goto out;
 
+       /*
+        * From here forward, we've got credentials set up and we're
+        * using resources, so do rlimit replacement before we start
+        * copying strings. (Note that the RLIMIT_NPROC check has

Quoted 33 lines · Kees Cook

In an attempt to provide sensible rlimit defaults for setuid execs, this inherits the namespace's init rlimits:

$ ulimit -s
8192
$ ulimit -s unlimited
$ /bin/sh -c 'ulimit -s'
unlimited
$ sudo /bin/sh -c 'ulimit -s'
8192

This is modified from Brad Spengler/PaX Team's hard-coded setuid exec stack rlimit (8MB) in the last public patch of grsecurity/PaX based on my understanding of the code. Changes or omissions from the original code are mine and don't reflect the original grsecurity/PaX code.

Signed-off-by Kees Cook <[email protected]>

---
Instead of copying all rlimits, we could also pick specific ones to copy (e.g. RLIMIT_STACK, or ones from Andy's list) or exclude from copying (probably better to blacklist than whitelist).

I think this is the right way to find the ns init task, but maybe it needs locking?

---
 fs/exec.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

fs/exec.c +0 -0

index 904199086490..80e8b2bd4284 100644

fs/exec.c static int exec_binprm(struct linux_binprm *bprm) · line 1683

Quoted 9 lines

fs/exec.c +6 -0

@@ -1675,6 +1675,12 @@ static int exec_binprm(struct linux_binprm *bprm)
        return ret;
 }
 
+static inline bool is_setuid_exec(struct linux_binprm *bprm)
+{
+       return (!uid_eq(bprm->cred->euid, current_euid()) ||
+               !gid_eq(bprm->cred->egid, current_egid()));
+}
+

How about skipping this and using security_bprm_secureexec()?

Quoted 24 lines

fs/exec.c +10 -0

 /*
  * sys_execve() executes a new program.
  */

@@ -1687,6 +1693,7 @@ static int do_execveat_common(int fd, struct filename *filename,
        struct linux_binprm *bprm;
        struct file *file;
        struct files_struct *displaced;
+       struct rlimit saved_rlim[RLIM_NLIMITS];
        int retval;
 
        if (IS_ERR(filename))

@@ -1771,24 +1778,38 @@ static int do_execveat_common(int fd, struct filename *filename,
        if (retval < 0)
                goto out;
 
+       /*
+        * From here forward, we've got credentials set up and we're
+        * using resources, so do rlimit replacement before we start
+        * copying strings. (Note that the RLIMIT_NPROC check has
+        * already happened.)
+        */
+       BUILD_BUG_ON(sizeof(saved_rlim) != sizeof(current->signal->rlim));
+       if (is_setuid_exec(bprm)) {
+               memcpy(saved_rlim, current->signal->rlim, sizeof(saved_rlim));

fs/exec.c static int do_execveat_common(int fd, struct filename *filename, · line 1794

Quoted 5 lines

fs/exec.c +5 -0

+               memcpy(current->signal->rlim,
+                      task_active_pid_ns(current)->child_reaper->signal->rlim,
+                      sizeof(current->signal->rlim));
+       }
+

Is there any locking needed here? Is it possible that another thread with the same current->signal is running?

I think the answer is that this needs to be after de_thread(), which is called from exec_binprm(), which is rather annoying since the stack rlimit is used before that. It's not *that* bad, since I think you could replace all the RLIMIT_STACK accesses with explciit code to look at the rlimits that are actually in play, but you just can't actually install them until you've flushed the old exec.

How this is handled elsewhere in the code is to put the new values in bprm. Putting new rlimits in bprm and changing them in flush_old_exec or or setup_new_exec seems very sensible. It also allows for them to be accessed before de_thread when setting up the new mm and we can still fail.

In an attempt to provide sensible rlimit defaults for setuid execs, this inherits the namespace's init rlimits:

$ ulimit -s
8192
$ ulimit -s unlimited
$ /bin/sh -c 'ulimit -s'
unlimited
$ sudo /bin/sh -c 'ulimit -s'
8192

This is modified from Brad Spengler/PaX Team's hard-coded setuid exec stack rlimit (8MB) in the last public patch of grsecurity/PaX based on my understanding of the code. Changes or omissions from the original code are mine and don't reflect the original grsecurity/PaX code.

Signed-off-by Kees Cook <[email protected]>

---
Instead of copying all rlimits, we could also pick specific ones to copy (e.g. RLIMIT_STACK, or ones from Andy's list) or exclude from copying (probably better to blacklist than whitelist).

I think this is the right way to find the ns init task, but maybe it needs locking?

---
 fs/exec.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

fs/exec.c +11 -0

index 904199086490..80e8b2bd4284 100644
@@ -1675,6 +1675,12 @@ static int exec_binprm(struct linux_binprm *bprm)
 	return ret;
 }
 
+static inline bool is_setuid_exec(struct linux_binprm *bprm)
+{
+	return (!uid_eq(bprm->cred->euid, current_euid()) ||
+		!gid_eq(bprm->cred->egid, current_egid()));
+}
+
 /*
  * sys_execve() executes a new program.
  */

@@ -1687,6 +1693,7 @@ static int do_execveat_common(int fd, struct filename *filename,
 	struct linux_binprm *bprm;
 	struct file *file;
 	struct files_struct *displaced;
+	struct rlimit saved_rlim[RLIM_NLIMITS];
 	int retval;
 
 	if (IS_ERR(filename))

@@ -1771,24 +1778,38 @@ static int do_execveat_common(int fd, struct filename *filename,
 	if (retval < 0)
 		goto out;
 
+	/*
+	 * From here forward, we've got credentials set up and we're
+	 * using resources, so do rlimit replacement before we start
+	 * copying strings. (Note that the RLIMIT_NPROC check has

In an attempt to provide sensible rlimit defaults for setuid execs, this inherits the namespace's init rlimits:

$ ulimit -s
8192
$ ulimit -s unlimited
$ /bin/sh -c 'ulimit -s'
unlimited
$ sudo /bin/sh -c 'ulimit -s'
8192

This is modified from Brad Spengler/PaX Team's hard-coded setuid exec stack rlimit (8MB) in the last public patch of grsecurity/PaX based on my understanding of the code. Changes or omissions from the original code are mine and don't reflect the original grsecurity/PaX code.

Signed-off-by Kees Cook <[email protected]>

---
Instead of copying all rlimits, we could also pick specific ones to copy (e.g. RLIMIT_STACK, or ones from Andy's list) or exclude from copying (probably better to blacklist than whitelist).

I think this is the right way to find the ns init task, but maybe it needs locking?

---
 fs/exec.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

fs/exec.c +0 -0

index 904199086490..80e8b2bd4284 100644

fs/exec.c +5 -0

 /*
  * sys_execve() executes a new program.
  */

@@ -1687,6 +1693,7 @@ static int do_execveat_common(int fd, struct filename *filename,
 	struct linux_binprm *bprm;
 	struct file *file;
 	struct files_struct *displaced;
+	struct rlimit saved_rlim[RLIM_NLIMITS];
 	int retval;
 
 	if (IS_ERR(filename))

@@ -1771,24 +1778,38 @@ static int do_execveat_common(int fd, struct filename *filename,
 	if (retval < 0)
 		goto out;
 
+	/*
+	 * From here forward, we've got credentials set up and we're
+	 * using resources, so do rlimit replacement before we start
+	 * copying strings. (Note that the RLIMIT_NPROC check has

fs/exec.c +9 -4

 	retval = copy_strings_kernel(1, &bprm->filename, bprm);
 	if (retval < 0)
-		goto out;
+		goto out_restore;
 
 	bprm->exec = bprm->p;
 	retval = copy_strings(bprm->envc, envp, bprm);
 	if (retval < 0)
-		goto out;
+		goto out_restore;
 
 	retval = copy_strings(bprm->argc, argv, bprm);
 	if (retval < 0)
-		goto out;
+		goto out_restore;
 
 	would_dump(bprm, bprm->file);
 
 	retval = exec_binprm(bprm);
 	if (retval < 0)
-		goto out;
+		goto out_restore;
 
 	/* execve succeeded */
 	current->fs->in_exec = 0;

@@ -1802,6 +1823,11 @@ static int do_execveat_common(int fd, struct filename *filename,
 		put_files_struct(displaced);
 	return retval;
 
+out_restore:
+	if (is_setuid_exec(bprm)) {
+		memcpy(current->signal->rlim, saved_rlim, sizeof(saved_rlim));
+	}
+
 out:
 	if (bprm->mm) {
 		acct_arg_size(bprm, 0);

Signature · 1 line

2.7.4

Quoted 33 lines · Kees Cook

In an attempt to provide sensible rlimit defaults for setuid execs, this inherits the namespace's init rlimits:

$ ulimit -s
8192
$ ulimit -s unlimited
$ /bin/sh -c 'ulimit -s'
unlimited
$ sudo /bin/sh -c 'ulimit -s'
8192

This is modified from Brad Spengler/PaX Team's hard-coded setuid exec stack rlimit (8MB) in the last public patch of grsecurity/PaX based on my understanding of the code. Changes or omissions from the original code are mine and don't reflect the original grsecurity/PaX code.

Signed-off-by Kees Cook <[email protected]>

---
Instead of copying all rlimits, we could also pick specific ones to copy (e.g. RLIMIT_STACK, or ones from Andy's list) or exclude from copying (probably better to blacklist than whitelist).

I think this is the right way to find the ns init task, but maybe it needs locking?

---
 fs/exec.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

fs/exec.c +0 -0

index 904199086490..80e8b2bd4284 100644

fs/exec.c static int exec_binprm(struct linux_binprm *bprm) · line 1682

Quoted 8 lines

fs/exec.c +5 -0

@@ -1675,6 +1675,12 @@ static int exec_binprm(struct linux_binprm *bprm)
      return ret;
 }
 
+static inline bool is_setuid_exec(struct linux_binprm *bprm)
+{
+     return (!uid_eq(bprm->cred->euid, current_euid()) ||
+             !gid_eq(bprm->cred->egid, current_egid()));
+}

Awesome I can make an executable setuid to myself and get all of roots rlimits!

Scratch inheritable rlimits as useful for any kind of policy decision.

In an attempt to provide sensible rlimit defaults for setuid execs, this inherits the namespace's init rlimits:

(a) minimal: just use our existing default stack (and stack _only_)
limit value for suid binaries that actually get extra permissions: { _STK_LIM, RLIM_INFINITY }.

(c) perhaps encourage people to annotate their suid binaries with
initial resource requirements (and for stack, I mean the existing GNU_STACK ELF annotation in particular).

Quoted 2 lines · Kees Cook

In an attempt to provide sensible rlimit defaults for setuid execs, this inherits the namespace's init rlimits:

Yeah, so I have to admit to hating this patch.

I'd much rather see something like

(a) minimal: just use our existing default stack (and stack _only_)
limit value for suid binaries that actually get extra permissions: { _STK_LIM, RLIM_INFINITY }.

(c) perhaps encourage people to annotate their suid binaries with
initial resource requirements (and for stack, I mean the existing GNU_STACK ELF annotation in particular).

For an example of (a), that existing _STK_LIM define is what the kernel defaults to, and it's a 8MB stack. And looking at my Fedora install, I see that the default user rlimit is 8MB for the stack. [...]
Is that just coincidence, or is that just a sign of "nobody ever even modifies the default value"? So (a) feels like "nobody really cares, and 8MB is fine, and nobody even bothers changing it - just do the minimal thing".

And (c) would be the sane option, and what we already do for things like GNU_STACK to enable/disable executable stacks. It really feels like allowing the GNU_STACK segment to contain stack rlimit override information would be the perfect tool for binaries to say "Yeah, I need more stack than _STK_LIM".

So I see many different approaches (that could be combined: I like combining (a) and (c), for example), and absolutely none of them involve the random "take some values from init".

And yes, a large part of this may be that I no longer feel like I can trust "init" to do the sane thing. You all presumably know why.

Quoted 4 lines

(a) minimal: just use our existing default stack (and stack _only_)
limit value for suid binaries that actually get extra permissions: { _STK_LIM, RLIM_INFINITY }.

This would look a lot like the existing patch; it'd just not copy the init process rlimits.

Quoted 7 lines · Kees Cook

Quoted 4 lines

(a) minimal: just use our existing default stack (and stack _only_)
limit value for suid binaries that actually get extra permissions: { _STK_LIM, RLIM_INFINITY }.

This would look a lot like the existing patch; it'd just not copy the init process rlimits.

Can't we just do the final rlimit setting so late in execve that we don't need that whole "saved_rlimit" thing?

If the issue is the "people can use argv/envp to already fill the stack", then I'd actually be happier with just limiting that.

We already claim that our ARG_MAX is just 128kB (old legacy). And I was really happy when we changed our execve() to not have that nasty array of pages, and we could expand on the array sizes. But we could *easily* just say "limit execve arrays to 8MB", because while our code can handle more, you do have latency issues and just memory use issues too.

So right now we already limit the stack size artificially to 1/4 the stack rlimit (see get_arg_page()), and we could easily just further cap it at 8M total - right now people obviously actually run in practice with much less (ie for me that argument size is capped at a quarter of that 8MB default rlimit).

I have heard of people who want a big stack due to crazy recursion or due to just doing otherwise insane things. But needing more than 8MB of arg/envp? Not happening.

So I think we could easily do that stack rlimit thing at the very last minute, and not have to worry about restoring anything.

Quoted 11 lines · Linus Torvalds

Quoted 7 lines · Kees Cook

Quoted 4 lines

(a) minimal: just use our existing default stack (and stack _only_)
limit value for suid binaries that actually get extra permissions: { _STK_LIM, RLIM_INFINITY }.

This would look a lot like the existing patch; it'd just not copy the init process rlimits.

Can't we just do the final rlimit setting so late in execve that we don't need that whole "saved_rlimit" thing?

The stack rlimit defines the mmap layout too:

do_execveat_common() ->
exec_binprm() ->
search_binary_handler() ->
fmt->load_binary (load_elf_binary()) ->
setup_new_exec() ->
arch_pick_mmap_layout() ->
mmap_is_legacy() ->
rlimit(RLIMIT_STACK) == RLIM_INFINITY

exec_binprm() happens after the other stack setup (copy_strings()), so if we wanted to avoid saved_rlimit, we'd have to replumb how arch_pick_mmap_layout() works and how copy_strings() performs its calculations (neither looks too terrible).

How about a much simpler solution: don't read rlimit at all in copy_strings(), let alone try to enforce it.

How about a much simpler solution: don't read rlimit at all in copy_strings(), let alone try to enforce it. Instead, just before the point of no return, check how much stack space is already used and, if it's more than an appropriate threshold (e.g. 1/4 of the rlimit), abort. Sure, this adds overhead if we're going to abort, but does that really matter?

I don't see why using rlimit for layout control makes any sense whatsoever. Is there some historical reason we need that? As far as I can see (on insufficient inspection) is that the kernel is trying to guarantee that, if we have so much arg crap that our remaining stack is less than 128k, then we don't exceed our limit by a little bit.

Quoted 6 lines · Andy Lutomirski

How about a much simpler solution: don't read rlimit at all in copy_strings(), let alone try to enforce it. Instead, just before the point of no return, check how much stack space is already used and, if it's more than an appropriate threshold (e.g. 1/4 of the rlimit), abort. Sure, this adds overhead if we're going to abort, but does that really matter?

We should avoid using up tons of memory and then failing. Better to cap it as we use it. Plumbing a sane value into this shouldn't be hard at all. Just making this a hardcoded 2MB seems sane (1/4 of 8MB).

Quoted 5 lines

I don't see why using rlimit for layout control makes any sense whatsoever. Is there some historical reason we need that? As far as I can see (on insufficient inspection) is that the kernel is trying to guarantee that, if we have so much arg crap that our remaining stack is less than 128k, then we don't exceed our limit by a little bit.

IIUC, this is a big deal on 32-bit. Unlimited stack triggers top-down mmap instead of bottom-up. I mean, I'd be delighted to get rid of this, but I thought it was relied on by userspace.

Quoted 8 lines · Kees Cook

Quoted 6 lines · Andy Lutomirski

How about a much simpler solution: don't read rlimit at all in copy_strings(), let alone try to enforce it. Instead, just before the point of no return, check how much stack space is already used and, if it's more than an appropriate threshold (e.g. 1/4 of the rlimit), abort. Sure, this adds overhead if we're going to abort, but does that really matter?

Quoted 3 lines

We should avoid using up tons of memory and then failing. Better to cap it as we use it. Plumbing a sane value into this shouldn't be hard at all. Just making this a hardcoded 2MB seems sane (1/4 of 8MB).

Quoted 7 lines

Quoted 6 lines

I don't see why using rlimit for layout control makes any sense whatsoever. Is there some historical reason we need that? As far as I can see (on insufficient inspection) is that the kernel is trying to guarantee that, if we have so much arg crap that our remaining stack is less than 128k, then we don't exceed our limit by a little bit.

Quoted 3 lines

IIUC, this is a big deal on 32-bit. Unlimited stack triggers top-down mmap instead of bottom-up. I mean, I'd be delighted to get rid of this, but I thought it was relied on by userspace.

I always say this backwards. :P Default is top-down (allocate at high addresses and work down toward low). With unlimited stack, allocations start at low addresses and work up. Here's the results (shown with randomize_va_space sysctl set to 0):

Quoted 9 lines · Kees Cook

Quoted 8 lines · Kees Cook

Quoted 6 lines · Andy Lutomirski

How about a much simpler solution: don't read rlimit at all in copy_strings(), let alone try to enforce it. Instead, just before the point of no return, check how much stack space is already used and, if it's more than an appropriate threshold (e.g. 1/4 of the rlimit), abort. Sure, this adds overhead if we're going to abort, but does that really matter?

Quoted 3 lines

Quoted 3 lines

We should avoid using up tons of memory and then failing. Better to cap it as we use it. Plumbing a sane value into this shouldn't be hard at all. Just making this a hardcoded 2MB seems sane (1/4 of 8MB).

Aren't there real use cases that use many megs of arguments?

We could probably get away with saying max(rlimit(RLIMIT_STACK), 2MB) as long as we make sure later on that we don't screw up if we've overallocated?

Quoted 8 lines

Quoted 3 lines

IIUC, this is a big deal on 32-bit. Unlimited stack triggers top-down mmap instead of bottom-up. I mean, I'd be delighted to get rid of this, but I thought it was relied on by userspace.

I always say this backwards. :P Default is top-down (allocate at high addresses and work down toward low). With unlimited stack, allocations start at low addresses and work up. Here's the results (shown with randomize_va_space sysctl set to 0):

Uhh, crikey! Where's the code that does that?

The stack rlimit defines the mmap layout too:

do_execveat_common() ->
exec_binprm() ->
search_binary_handler() ->
fmt->load_binary (load_elf_binary()) ->
setup_new_exec() ->
arch_pick_mmap_layout() ->
mmap_is_legacy() ->
rlimit(RLIMIT_STACK) == RLIM_INFINITY

Quoted 2 lines · Andy Lutomirski

Aren't there real use cases that use many megs of arguments?

They'd be relatively new since the args were pretty limited before. I'd be curious to see them.

So 2+MB is still definitely something people can do (and probably *do* do).

Quoted 1 line · Linus Torvalds

So 2+MB is still definitely something people can do (and probably *do* do).

With the default 8MB stack, most people are already limited to 2MB here. I guess the question is, do people raise their stack rlimit to gain more arguments? Should I pick a different value for the args?

Quoted 7 lines · Kees Cook

Quoted 1 line · Linus Torvalds

So 2+MB is still definitely something people can do (and probably *do* do).

With the default 8MB stack, most people are already limited to 2MB here. I guess the question is, do people raise their stack rlimit to gain more arguments? Should I pick a different value for the args?

So I would not be at all surprised if people just made the stack limit higher when they hit the E2BIG issue in some script.

So yes, I'd make the max args cutoff be higher than 2MB.

I'd suggest we make the code do:

(a) keep the existing rlimit/4 check (so *most* people will see the
exact same behavior)

(b) add a static max arg check for something that is closer to 8MB
but leaves a somewhat reasonable stack size even if the stack size get reset to 8MB

I'd suggest that (b) case just be 6MB or something. Maybe make it (_STK_LIM/4*3) or whatever, in case we ever end up changing that value.

Quoted 1 line · Andy Lutomirski

Aren't there real use cases that use many megs of arguments?

They'd be relatively new since the args were pretty limited before. I'd be curious to see them.

Quoted 3 lines

We could probably get away with saying max(rlimit(RLIMIT_STACK), 2MB) as long as we make sure later on that we don't screw up if we've overallocated?

min, not max, but yeah. Here's part of what I have for get_arg_page():

                rlim = current->signal->rlim;

I always say this backwards. :P Default is top-down (allocate at high addresses and work down toward low). With unlimited stack, allocations start at low addresses and work up. Here's the results (shown with randomize_va_space sysctl set to 0):

Quoted 5 lines · Kees Cook

I always say this backwards. :P Default is top-down (allocate at high addresses and work down toward low). With unlimited stack, allocations start at low addresses and work up. Here's the results (shown with randomize_va_space sysctl set to 0):

But this doesn't affect the stack layout itself.

So we could do the stack copying without much caring, because that happens first, right?

So I think we can do all the envp/argv copying first, and then - as we change the credentials, change the rlimit. And the string copies wouldn't need to care much - although I guess they are also fine checking against a possible *smaller* stack rlimit, which is actually what we'd want.

And I think the credentials switch (which is the point of no return anyway) happens before we start mmap'ing the executable etc. We used to have some odd code there and do it in the completely wrong order (checking that the binary was executable for the *old* user, which makes no sense, iirc)

So I'm getting the sense that none of this should be a problem.

But it's entirely possible that I missed something, and am just full of shit. Our execve() path has traditionally been very hard to read. It's actually gotten a bit better, but the whole "jump back and forth between the generic fs/exec.c code and the binfmt code" is certainly still there.

On Thu, Jul 6, 2017 at 10:39 PM, Linus Torvalds

Quoted 5 lines

And I think the credentials switch (which is the point of no return anyway) happens before we start mmap'ing the executable etc. We used to have some odd code there and do it in the completely wrong order (checking that the binary was executable for the *old* user, which makes no sense, iirc)

Yeah, it all happens in setup_new_exec(). The first thing is layout selection, then switching credentials. It could be made to take a hint from GNU_STACK (which was parsed before setup_new_exec() is called), check security_bprm_secureexec() and then make the rlimit changes, all before the layout selection.

On Thu, Jul 6, 2017 at 10:49 PM, Kees Cook <[email protected]> wrote:

At Andy's suggestion I'm using security_bprm_secureexec() to test for setuid-ness. However, this seems to expect the credentials to have already been installed.

And yet ... the following patch still works correctly when I call it "early".

On Thu, Jul 6, 2017 at 11:40 PM, Kees Cook <[email protected]> wrote:
So I definitely like this approach, as long as we clarify that crazy security_bprm_secureexec() model. That code really is insane.

In contrast, if the code just did:

    if (security_bprm_secureexec(bprm)) {
        if (current->signal->rlim[RLIMIT_STACK].rlim_cur > _STK_LIM)
            current->signal->rlim[RLIMIT_STACK].rlim_cur = _STK_LIM;
    }

and leave the hard limit alone entirely. At least that doesn't let anybody escape the limits that the sysadmin has set.

Hmm? Yes, this allows people to try to attack suid binaries with a really small stack. But that's a pre-existing attack - do we have worries about it?

Quoted 11 lines · Linus Torvalds

Quoted 7 lines · Kees Cook

Quoted 4 lines

(a) minimal: just use our existing default stack (and stack _only_)
limit value for suid binaries that actually get extra permissions: { _STK_LIM, RLIM_INFINITY }.

This would look a lot like the existing patch; it'd just not copy the init process rlimits.

Can't we just do the final rlimit setting so late in execve that we don't need that whole "saved_rlimit" thing?

The stack rlimit defines the mmap layout too:

do_execveat_common() ->
exec_binprm() ->
search_binary_handler() ->
fmt->load_binary (load_elf_binary()) ->
setup_new_exec() ->
arch_pick_mmap_layout() ->
mmap_is_legacy() ->
rlimit(RLIMIT_STACK) == RLIM_INFINITY

Quoted 16 lines · Kees Cook

Quoted 11 lines · Linus Torvalds

Quoted 7 lines · Kees Cook

Quoted 4 lines

(a) minimal: just use our existing default stack (and stack _only_)
limit value for suid binaries that actually get extra permissions: { _STK_LIM, RLIM_INFINITY }.

This would look a lot like the existing patch; it'd just not copy the init process rlimits.

Can't we just do the final rlimit setting so late in execve that we don't need that whole "saved_rlimit" thing?

The stack rlimit defines the mmap layout too:

Quoted 8 lines

do_execveat_common() ->
exec_binprm() ->
search_binary_handler() ->
fmt->load_binary (load_elf_binary()) ->
setup_new_exec() ->
arch_pick_mmap_layout() ->
mmap_is_legacy() ->
rlimit(RLIMIT_STACK) == RLIM_INFINITY

FWIW this is gone in tip tree. See lkml.kernel.org/r/[email protected]

Sounds good to me, but won't large-memory users in 32-bit get annoyed?

Quoted 2 lines · Kees Cook

Sounds good to me, but won't large-memory users in 32-bit get annoyed?

We'll see.

I suspect that all large-memory users have long since upgraded to x86-64 (rule of thumb: if you are upgrading kernels today, you probably upgraded hardware ten years ago), and that this may be a non-issue today.

Quoted 26 lines · Michal Hocko

Quoted 24 lines · Kees Cook

Quoted 11 lines · Linus Torvalds

Quoted 7 lines · Kees Cook

Quoted 4 lines

(a) minimal: just use our existing default stack (and stack _only_)
limit value for suid binaries that actually get extra permissions: { _STK_LIM, RLIM_INFINITY }.

This would look a lot like the existing patch; it'd just not copy the init process rlimits.

Can't we just do the final rlimit setting so late in execve that we don't need that whole "saved_rlimit" thing?

The stack rlimit defines the mmap layout too:

do_execveat_common() ->
exec_binprm() ->
search_binary_handler() ->
fmt->load_binary (load_elf_binary()) ->
setup_new_exec() ->
arch_pick_mmap_layout() ->
mmap_is_legacy() ->
rlimit(RLIMIT_STACK) == RLIM_INFINITY

Quoted 2 lines

FWIW this is gone in tip tree. See lkml.kernel.org/r/[email protected]

Sounds good to me, but won't large-memory users in 32-bit get annoyed?

On Mon 10-07-17 09:12:11, Kees Cook wrote:

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

do_execveat_common() ->
exec_binprm() ->
search_binary_handler() ->
fmt->load_binary (load_elf_binary()) ->
setup_new_exec() ->
arch_pick_mmap_layout() ->
mmap_is_legacy() ->
rlimit(RLIMIT_STACK) == RLIM_INFINITY

Quoted 4 lines

Quoted 2 lines

FWIW this is gone in tip tree. See lkml.kernel.org/r/[email protected]

Sounds good to me, but won't large-memory users in 32-bit get annoyed?

Why would they? 32b do bottom up layouts by default.

OK, I misread the code. 32b applications on 64b systems do top down by
default and only if they override this by ADDR_COMPAT_LAYOUT personality. For some reason I thought that 32b userspace goes a different path and makes sure that they are always doing bottom up.

Anyway even if somebody really needs to grow stack really large we have
the personality to give them the legacy layout.

(a) minimal: just use our existing default stack (and stack _only_)
limit value for suid binaries that actually get extra permissions: { _STK_LIM, RLIM_INFINITY }.

(c) perhaps encourage people to annotate their suid binaries with
initial resource requirements (and for stack, I mean the existing GNU_STACK ELF annotation in particular).