From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm@xmission.com (Eric W. Biederman) To: Kees Cook Cc: Linus Torvalds , Andy Lutomirski , David Howells , Serge Hallyn , John Johansen , Casey Schaufler , Alexander Viro , Michal Hocko , Ben Hutchings , Hugh Dickins , Oleg Nesterov , "Jason A. Donenfeld" , Rik van Riel , James Morris , Greg Ungerer , Ingo Molnar , Nicolas Pitre , Stephen Smalley , Paul Moore , Vivek Goyal , =?utf-8?Q?Micka=C3=ABl_Sala=C3=BCn?= , Tetsuo Handa , "linux-fsdevel\@vger.kernel.org" , LKML , "" , SE Linux References: <1499673451-66160-1-git-send-email-keescook@chromium.org> <1499673451-66160-3-git-send-email-keescook@chromium.org> <87a84cr7oc.fsf@xmission.com> Date: Mon, 10 Jul 2017 12:18:48 -0500 In-Reply-To: (Kees Cook's message of "Mon, 10 Jul 2017 09:06:29 -0700") Message-ID: <87bmosmcqv.fsf@xmission.com> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [PATCH v2 2/8] exec: Move security_bprm_secureexec() earlier List-Id: "Security-Enhanced Linux \(SELinux\) mailing list" List-Post: List-Help: Kees Cook writes: > On Mon, Jul 10, 2017 at 1:57 AM, Eric W. Biederman > wrote: >> Kees Cook writes: >> >>> There are several places where exec needs to know if a privilege-gain has >>> happened. These should be using the results of security_bprm_secureexec() >>> but it is getting (needlessly) called very late. >> >> It is hard to tell at a glance but I believe this introduces a >> regression. >> >> cap_bprm_set_creds is currently called before cap_bprm_secureexec and >> it has a number of cases such as no_new_privs and ptrace that can result >> in some of the precomputed credential changes not happening. >> >> Without accounting for that I believe your cap_bprm_securexec now >> returns a postive value too early. > > It's still before cap_bprm_secureexec. cap_brpm_set_creds() is in > prepare_binprm(), which is well before exec_binprm() and it's eventual > call to setup_new_exec(). Good point. I didn't double check and the set in the name had me thinking it was setting the creds on current. Is there any reason we need a second security hook? It feels like we should be able to just fold the secureexec hook into the set_creds hook. The two are so interrelated I fear that having them separate only encourages them to diverge in trivial ways as it is easy to forget about some detail or other. Certainly having them called from different functions seems wrong. If we know enough in prepare_binprm we know enough. Eric From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman) Subject: Re: [PATCH v2 2/8] exec: Move security_bprm_secureexec() earlier Date: Mon, 10 Jul 2017 12:18:48 -0500 Message-ID: <87bmosmcqv.fsf@xmission.com> References: <1499673451-66160-1-git-send-email-keescook@chromium.org> <1499673451-66160-3-git-send-email-keescook@chromium.org> <87a84cr7oc.fsf@xmission.com> Mime-Version: 1.0 Content-Type: text/plain Cc: Nicolas Pitre , "Jason A. Donenfeld" , Andy Lutomirski , Tetsuo Handa , Michal Hocko , David Howells , SE Linux , Ingo Molnar , Hugh Dickins , Greg Ungerer , Stephen Smalley , Vivek Goyal , Rik van Riel , "linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Alexander Viro , James Morris , =?utf-8?Q?Micka=C3=ABl_Sala=C3=BCn?= , John Johansen , Ben Hutchings , Oleg Nesterov Return-path: In-Reply-To: (Kees Cook's message of "Mon, 10 Jul 2017 09:06:29 -0700") List-Post: List-Help: Errors-To: selinux-bounces-+05T5uksL2qpZYMLLGbcSA@public.gmane.org Sender: "Selinux" List-Id: linux-fsdevel.vger.kernel.org Kees Cook writes: > On Mon, Jul 10, 2017 at 1:57 AM, Eric W. Biederman > wrote: >> Kees Cook writes: >> >>> There are several places where exec needs to know if a privilege-gain has >>> happened. These should be using the results of security_bprm_secureexec() >>> but it is getting (needlessly) called very late. >> >> It is hard to tell at a glance but I believe this introduces a >> regression. >> >> cap_bprm_set_creds is currently called before cap_bprm_secureexec and >> it has a number of cases such as no_new_privs and ptrace that can result >> in some of the precomputed credential changes not happening. >> >> Without accounting for that I believe your cap_bprm_securexec now >> returns a postive value too early. > > It's still before cap_bprm_secureexec. cap_brpm_set_creds() is in > prepare_binprm(), which is well before exec_binprm() and it's eventual > call to setup_new_exec(). Good point. I didn't double check and the set in the name had me thinking it was setting the creds on current. Is there any reason we need a second security hook? It feels like we should be able to just fold the secureexec hook into the set_creds hook. The two are so interrelated I fear that having them separate only encourages them to diverge in trivial ways as it is easy to forget about some detail or other. Certainly having them called from different functions seems wrong. If we know enough in prepare_binprm we know enough. Eric