Messages in this thread | | | Date | Fri, 29 May 2020 14:06:33 -0700 | From | Kees Cook <> | Subject | Re: [PATCH 1/2] exec: Add a per bprm->file version of per_clear |
| |
On Fri, May 29, 2020 at 11:46:40AM -0500, Eric W. Biederman wrote: > > There is a small bug in the code that recomputes parts of bprm->cred > for every bprm->file. The code never recomputes the part of > clear_dangerous_personality_flags it is responsible for. > > Which means that in practice if someone creates a sgid script > the interpreter will not be able to use any of: > READ_IMPLIES_EXEC > ADDR_NO_RANDOMIZE > ADDR_COMPAT_LAYOUT > MMAP_PAGE_ZERO. > > This accentially clearing of personality flags probably does > not matter in practice because no one has complained > but it does make the code more difficult to understand. > > Further remaining bug compatible prevents the recomputation from being > removed and replaced by simply computing bprm->cred once from the > final bprm->file. > > Making this change removes the last behavior difference between > computing bprm->creds from the final file and recomputing > bprm->cred several times. Which allows this behavior change > to be justified for it's own reasons, and for any but hunts > looking into why the behavior changed to wind up here instead > of in the code that will follow that computes bprm->cred > from the final bprm->file. > > This small logic bug appears to have existed since the code > started clearing dangerous personality bits. > > History Tree: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git > Fixes: 1bb0fa189c6a ("[PATCH] NX: clean up legacy binary support") > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Yup, this looks good. Pointless nit because it's removed in the next patch, but pf_per_clear is following the same behavioral pattern as active_secureexec, it could be named active_per_clear, but since this already been bikeshed in v1, it's fine! :)
Reviewed-by: Kees Cook <keescook@chromium.org>
I wish we had more robust execve tests. :(
-- Kees Cook
| |