Re: [RFC][PATCH] exec: Use init rlimits for setuid exec

[Andy Lutomirski]

On Wed, Jul 5, 2017 at 9:32 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> 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 <keescook@xxxxxxxxxxxx>
> ---
> 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(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 904199086490..80e8b2bd4284 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -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()?

>  /*
>   * 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));
> +               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.

Perhaps, if you fix this, the changelog should say:

Changes, omissions, or bugfixes from the original
code are mine and don't reflect the original grsecurity/PaX code.

(Hmm, is the grsecurity/PaX code exploitable?  If you can exec a
setuid program and arrange for it to fail while you're in a threaded
program, it looks like the patch you sent will result in corruption of
the rlimits in use by your other threads.  Admittedy, bypassing
rlimits is not exactly a huge deal.)

Also, should this whole thing have a sysctl?

--Andy