Re: [PATCH v4 00/15] exec: Use sane stack rlimit under secureexec

[Kees Cook]

Ugh, please ignore this v4. There's a typo that snuck in. I'll send a v5 soon...

-Kees

On Mon, Jul 31, 2017 at 4:51 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> As discussed with Linus and Andy, we need to reset the stack rlimit
> before we do memory layouts when execing a privilege-gaining (e.g.
> setuid) program. To do this, we need to know the results of the
> bprm_secureexec hook before memory layouts. As it turns out, this
> can be made _mostly_ trivial by collapsing bprm_secureexec into
> bprm_set_creds.
>
> The LSMs using bprm_secureexec nearly always save state between
> bprm_set_creds and bprm_secureexec. In the face of multiple calls to
> bprm_set_creds (via prepare_binprm() calls from binfmt_script, etc),
> all LSMs except commoncap only pay attention to the first call, so
> that aligns well with collapsing bprm_secureexec into bprm_set_creds.
> The commoncaps, though, needs to check the _last_ bprm_set_creds, so
> this series just swaps one bprm flag for another (cap_effective is no
> longer needed to save state between bprm_set_creds and bprm_secureexec,
> but we do need to keep a separate state, so we add the cap_elevated flag).
>
> Once secureexec is available to setup_new_exec() before the memory
> layout, we can add an rlimit sanity-check for setuid execs. (With no
> need to clean up since we're past the point of no return.)
>
> Along the way, this fixes comments, renames a variable, and consolidates
> dumpability and pdeath_signal clearing, which includes some commit log
> archeology to examine the subtle differences between what we had and
> what we need.
>
> Several folks have looked at this already (thank you!) but I'd appreciate
> any other eyes on this to make sure it isn't broken in some special
> way. Looking at the diffstat, even after all my long comments, this is
> a net reduction in lines. :)
>
> Given this crosses a bunch of areas, I think this is likely best to go
> via the -mm tree, which is where nearly all of my prior exec work has
> lived too. It's also after rc2 at this point, so I'd be slightly nervous
> to see this land directly in Linus's tree, but I leave that decision up
> to Linus. :) Regardless, very little has changed between v3 and v4, so I
> think this is ready to go.
>
> Thanks!
>
> -Kees
>
> ----------------------------------------------------------------
> Kees Cook (15):
>       exec: Rename bprm->cred_prepared to called_set_creds
>       exec: Correct comments about "point of no return"
>       binfmt: Introduce secureexec flag
>       apparmor: Refactor to remove bprm_secureexec hook
>       selinux: Refactor to remove bprm_secureexec hook
>       smack: Refactor to remove bprm_secureexec hook
>       commoncap: Refactor to remove bprm_secureexec hook
>       commoncap: Move cap_elevated calculation into bprm_set_creds
>       LSM: drop bprm_secureexec hook
>       exec: Use secureexec for setting dumpability
>       exec: Use secureexec for clearing pdeath_signal
>       smack: Remove redundant pdeath_signal clearing
>       exec: Consolidate dumpability logic
>       exec: Use sane stack rlimit under secureexec
>       exec: Consolidate pdeath_signal clearing
>
>  fs/binfmt_elf.c                    |  2 +-
>  fs/binfmt_elf_fdpic.c              |  2 +-
>  fs/binfmt_flat.c                   |  2 +-
>  fs/exec.c                          | 56 ++++++++++++++++++++++++++++----------
>  include/linux/binfmts.h            | 24 ++++++++++++----
>  include/linux/lsm_hooks.h          | 14 ++++------
>  include/linux/security.h           |  7 -----
>  security/apparmor/domain.c         | 24 ++--------------
>  security/apparmor/include/domain.h |  1 -
>  security/apparmor/include/file.h   |  3 --
>  security/apparmor/lsm.c            |  1 -
>  security/commoncap.c               | 50 ++++++++--------------------------
>  security/security.c                |  5 ----
>  security/selinux/hooks.c           | 26 ++++--------------
>  security/smack/smack_lsm.c         | 34 ++---------------------
>  security/tomoyo/tomoyo.c           |  2 +-
>  16 files changed, 91 insertions(+), 162 deletions(-)
>
> v4:
> - add {Ack,Review,Test}ed-bys
> - reorder patches to move trivial refactoring to the front
> - move secureexec flag set earlier in the series to setup_new_exec(); amluto
>
> v3:
> - collapse brpm_secureexec into bprm_set_creds; ebiederm.
> - continue to improve various comments
>
> v2:
> - fix missed current_security() uses in LSMs.
> - research/consolidate dumpability setting logic
> - research/consolidate pdeath_signal clearing logic
> - split up logical steps a little more for easier review (and bisection)
> - fix some old broken comments
>
>



-- 
Kees Cook
Pixel Security