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

[ebiederm@xmission.com (Eric W. Biederman)]

Andy Lutomirski <luto@kernel.org> writes:

> 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?

tasklist_lock protects child_reaper, and child_reaper changes.
I suppose rcu_read_lock should be enough for the case above if we just
want a snapshot in time.

It is 100% possible another thread is running and could take advantage
of the new limits.

> 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.

Eric