lkml.org 
[lkml]   [2017]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC][PATCH] exec: Use init rlimits for setuid exec
On Wed, Jul 05, 2017 at 09:32:35PM -0700, Kees Cook 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@chromium.org>
> ---
> 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()));
> +}
> +
> /*
> * 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));
> + }
> +

But you're not replacing just RLIMIT_STACK but all rlimits here. Please
don't do this, as I mentionned elsewhere in the thread it will really
become a pain at least for RLIMIT_CORE and RLIMIT_NOFILE. Better only
apply RLIMIT_STACK as the commit message mentions it.

Willy

\
 
 \ /
  Last update: 2017-07-06 07:48    [W:0.098 / U:16.640 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site