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

    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

    \
     
     \ /
      Last update: 2017-07-06 07:00    [W:4.179 / U:0.740 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site