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:4.630 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site