On Thu, Jul 6, 2017 at 11:40 PM, Kees Cook <keesc...@chromium.org> wrote:
> On Thu, Jul 6, 2017 at 10:49 PM, Kees Cook <keesc...@chromium.org> wrote:
>
> At Andy's suggestion I'm using security_bprm_secureexec() to test for
> setuid-ness. However, this seems to expect the credentials to have
> already been installed.
That function actually takes a look at current creds, so yes, it
currently works only after the creds have been installed.
It works for you because it *also* checks if the current cred is not
root, and then it looks at bprm->cap_effective too (indirectly - check
the commoncap code), which has been set earlier.
But it's crazy code. I literally don't know why it looks at
current_creds(), when it's all about the bprm, which has its own creds
- and then it would work any time.
That code is confusing. Somebody should check it. That whole
security_bprm_secureexec() hook seems mis-designed.
> And yet ... the following patch still works correctly when I call it "early".
So I definitely like this approach, as long as we clarify that crazy
security_bprm_secureexec() model. That code really is insane.
But I wonder:
> + if (security_bprm_secureexec(bprm)) {
> + struct rlimit default_stack = { _STK_LIM, RLIM_INFINITY };
> +
> + current->signal->rlim[RLIMIT_STACK] = default_stack;
Should we override the whole thing? And in particular, should we
override the hard limit at all?
If we have an existing lower stack limit, we might as well leave it be
entirely. And if the soft stack limit is higher, why modify the hard
limit?
In particular, the scenario I'm thinking of is "system admin on a
small machine has set everybodys stack limits to just 8M as a _hard_
limit".
Now, Bob and Jill agree to help each other to escape that limit, so
they make suid binaries for their own account, and let each other use
them - boom, they now have _STK_LIM and RLIM_INFINITY stack limits.
Not good.
In contrast, if the code just did:
if (security_bprm_secureexec(bprm)) {
if (current->signal->rlim[RLIMIT_STACK].rlim_cur > _STK_LIM)
current->signal->rlim[RLIMIT_STACK].rlim_cur = _STK_LIM;
}
and leave the hard limit alone entirely. At least that doesn't let
anybody escape the limits that the sysadmin has set.
Hmm? Yes, this allows people to try to attack suid binaries with a
really small stack. But that's a pre-existing attack - do we have
worries about it?
Linus
