Re: [PATCH v2 3/8] exec: Convert security_bprm_set_creds into security_bprm_repopulate_creds
On Wed, May 20, 2020 at 03:22:38PM -0500, Eric W. Biederman wrote:
> Kees Cook writes:
>
> > On Tue, May 19, 2020 at 02:03:23PM -0500, Eric W. Biederman wrote:
> >> Kees Cook writes:
> >>
> >> > On Mon, May 18, 2020 at 07:31:14PM -0500, Eric W. Biederman wrote:
> >> >> [...]
> >> >> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> >> >> index d1217fcdedea..8605ab4a0f89 100644
> >> >> --- a/include/linux/binfmts.h
> >> >> +++ b/include/linux/binfmts.h
> >> >> @@ -27,10 +27,10 @@ struct linux_binprm {
> >> >> unsigned long argmin; /* rlimit marker for copy_strings() */
> >> >> unsigned int
> >> >> /*
> >> >> -* True if most recent call to cap_bprm_set_creds
> >> >> +* True if most recent call to security_bprm_set_creds
> >> >> * resulted in elevated privileges.
> >> >> */
> >> >> - cap_elevated:1,
> >> >> + active_secureexec:1,
> >> >
> >> > Also, I'd like it if this comment could be made more verbose as well, for
> >> > anyone trying to understand the binfmt execution flow for the first time.
> >> > Perhaps:
> >> >
> >> > /*
> >> > * Must be set True during the any call to
> >> > * bprm_set_creds hook where the execution would
> >> > * reuslt in elevated privileges. (The hook can be
> >> > * called multiple times during nested interpreter
> >> > * resolution across binfmt_script, binfmt_misc, etc).
> >> > */
> >> Well it is not during but after the call that it becomes true.
> >> I think most recent covers the case of multiple calls.
> >
> > I'm thinking of an LSM writing reading these comments to decide what
> > they need to do to the flags, so it's a direction to them to set it to
> > true if they have determined that privilege was gained. (Though in
> > theory, this is all moot since only the commoncap hook cares.)
>
> The comments for an LSM writer are in include/linux/lsm_hooks.h
>
> * @bprm_repopulate_creds:
> *Assuming that the relevant bits of @bprm->cred->security have been
> *previously set, examine @bprm->file and regenerate them. This is
> *so that the credentials derived from the interpreter the code is
> *actually going to run are used rather than credentials derived
> *from a script. This done because the interpreter binary needs to
> *reopen script, and may end up opening something completely different.
> *This hook may also optionally check permissions (e.g. for
> *transitions between security domains).
> *The hook must set @bprm->active_secureexec to 1 if AT_SECURE should be
> set to
> *request libc enable secure mode.
> *@bprm contains the linux_binprm structure.
> *Return 0 if the hook is successful and permission is granted.
>
> I hope that is detailed enough.
>
> I will leave the rest of the comments for the maintainer of the code.
>
> I really don't think we should duplicate the prescriptive comments in
> multiple locations.
Okay, that's fair enough. Thanks!
--
Kees Cook
Re: [PATCH v2 3/8] exec: Convert security_bprm_set_creds into security_bprm_repopulate_creds
Kees Cook writes:
> On Tue, May 19, 2020 at 02:03:23PM -0500, Eric W. Biederman wrote:
>> Kees Cook writes:
>>
>> > On Mon, May 18, 2020 at 07:31:14PM -0500, Eric W. Biederman wrote:
>> >> [...]
>> >> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
>> >> index d1217fcdedea..8605ab4a0f89 100644
>> >> --- a/include/linux/binfmts.h
>> >> +++ b/include/linux/binfmts.h
>> >> @@ -27,10 +27,10 @@ struct linux_binprm {
>> >> unsigned long argmin; /* rlimit marker for copy_strings() */
>> >> unsigned int
>> >> /*
>> >> - * True if most recent call to cap_bprm_set_creds
>> >> + * True if most recent call to security_bprm_set_creds
>> >>* resulted in elevated privileges.
>> >>*/
>> >> - cap_elevated:1,
>> >> + active_secureexec:1,
>> >
>> > Also, I'd like it if this comment could be made more verbose as well, for
>> > anyone trying to understand the binfmt execution flow for the first time.
>> > Perhaps:
>> >
>> >/*
>> > * Must be set True during the any call to
>> > * bprm_set_creds hook where the execution would
>> > * reuslt in elevated privileges. (The hook can be
>> > * called multiple times during nested interpreter
>> > * resolution across binfmt_script, binfmt_misc, etc).
>> > */
>> Well it is not during but after the call that it becomes true.
>> I think most recent covers the case of multiple calls.
>
> I'm thinking of an LSM writing reading these comments to decide what
> they need to do to the flags, so it's a direction to them to set it to
> true if they have determined that privilege was gained. (Though in
> theory, this is all moot since only the commoncap hook cares.)
The comments for an LSM writer are in include/linux/lsm_hooks.h
* @bprm_repopulate_creds:
* Assuming that the relevant bits of @bprm->cred->security have been
* previously set, examine @bprm->file and regenerate them. This is
* so that the credentials derived from the interpreter the code is
* actually going to run are used rather than credentials derived
* from a script. This done because the interpreter binary needs to
* reopen script, and may end up opening something completely different.
* This hook may also optionally check permissions (e.g. for
* transitions between security domains).
* The hook must set @bprm->active_secureexec to 1 if AT_SECURE should be
set to
* request libc enable secure mode.
* @bprm contains the linux_binprm structure.
* Return 0 if the hook is successful and permission is granted.
I hope that is detailed enough.
I will leave the rest of the comments for the maintainer of the code.
I really don't think we should duplicate the prescriptive comments in
multiple locations.
Eric
Re: [PATCH v2 3/8] exec: Convert security_bprm_set_creds into security_bprm_repopulate_creds
James Morris writes: > On Mon, 18 May 2020, Eric W. Biederman wrote: > >> diff --git a/fs/exec.c b/fs/exec.c >> index 9e70da47f8d9..8e3b93d51d31 100644 >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -1366,7 +1366,7 @@ int begin_new_exec(struct linux_binprm * bprm) >> * the final state of setuid/setgid/fscaps can be merged into the >> * secureexec flag. >> */ >> -bprm->secureexec |= bprm->cap_elevated; >> +bprm->secureexec |= bprm->active_secureexec; > > Which kernel tree are these patches for? Seems like begin_new_exec() is > from a prerequisite patchset. The base is: git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git exec-next I should have mentioned. I am several round deep in cleaning up exec already. begin_new_exec is essentially forget_old_exec. Eric
Re: [PATCH v2 3/8] exec: Convert security_bprm_set_creds into security_bprm_repopulate_creds
On Mon, 18 May 2020, Eric W. Biederman wrote: > diff --git a/fs/exec.c b/fs/exec.c > index 9e70da47f8d9..8e3b93d51d31 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1366,7 +1366,7 @@ int begin_new_exec(struct linux_binprm * bprm) >* the final state of setuid/setgid/fscaps can be merged into the >* secureexec flag. >*/ > - bprm->secureexec |= bprm->cap_elevated; > + bprm->secureexec |= bprm->active_secureexec; Which kernel tree are these patches for? Seems like begin_new_exec() is from a prerequisite patchset. -- James Morris
Re: [PATCH v2 3/8] exec: Convert security_bprm_set_creds into security_bprm_repopulate_creds
On Tue, May 19, 2020 at 02:03:23PM -0500, Eric W. Biederman wrote:
> Kees Cook writes:
>
> > On Mon, May 18, 2020 at 07:31:14PM -0500, Eric W. Biederman wrote:
> >> [...]
> >> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> >> index d1217fcdedea..8605ab4a0f89 100644
> >> --- a/include/linux/binfmts.h
> >> +++ b/include/linux/binfmts.h
> >> @@ -27,10 +27,10 @@ struct linux_binprm {
> >>unsigned long argmin; /* rlimit marker for copy_strings() */
> >>unsigned int
> >>/*
> >> - * True if most recent call to cap_bprm_set_creds
> >> + * True if most recent call to security_bprm_set_creds
> >> * resulted in elevated privileges.
> >> */
> >> - cap_elevated:1,
> >> + active_secureexec:1,
> >
> > Also, I'd like it if this comment could be made more verbose as well, for
> > anyone trying to understand the binfmt execution flow for the first time.
> > Perhaps:
> >
> > /*
> > * Must be set True during the any call to
> > * bprm_set_creds hook where the execution would
> > * reuslt in elevated privileges. (The hook can be
> > * called multiple times during nested interpreter
> > * resolution across binfmt_script, binfmt_misc, etc).
> > */
> Well it is not during but after the call that it becomes true.
> I think most recent covers the case of multiple calls.
I'm thinking of an LSM writing reading these comments to decide what
they need to do to the flags, so it's a direction to them to set it to
true if they have determined that privilege was gained. (Though in
theory, this is all moot since only the commoncap hook cares.)
> I think having the loop explicitly in the code a few patches
> later makes it clear that there is a loop dealing with interpreters.
>
> Conciseness has a virtue in that it is easy to absorb. Seeing
> active says most recent and secureexec does not is enough to ask
> questions and look at the code.
I still think a hint about the nature of nested exec resolution would be
nice in here somewhere, especially given that this value is zeroed
before each call to the hook.
--
Kees Cook
Re: [PATCH v2 3/8] exec: Convert security_bprm_set_creds into security_bprm_repopulate_creds
Kees Cook writes:
> On Mon, May 18, 2020 at 07:31:14PM -0500, Eric W. Biederman wrote:
>>
>> Rename bprm->cap_elevated to bprm->active_secureexec and initialize it
>> in prepare_binprm instead of in cap_bprm_set_creds. Initializing
>> bprm->active_secureexec in prepare_binprm allows multiple
>> implementations of security_bprm_repopulate_creds to play nicely with
>> each other.
>>
>> Rename security_bprm_set_creds to security_bprm_reopulate_creds to
>> emphasize that this path recomputes part of bprm->cred. This
>> recomputation avoids the time of check vs time of use problems that
>> are inherent in unix #! interpreters.
>>
>> In short two renames and a move in the location of initializing
>> bprm->active_secureexec.
>
> I like this much better than the direct call to the capabilities hook.
> Thanks!
>
> Reviewed-by: Kees Cook
>
> One nit is a bikeshed on the name "active_secureexec", since
> the word "active" isn't really associated with any other part of the
> binfmt logic. It's supposed to be "latest state from the binfmt loop",
> so instead of "active", I considered these words that I also didn't
> like: "current", "this", "recent", and "now". Is "latest" better than
> "active"? Probably not.
I had pretty much the same problem. Active at least conveys that it
is still malleable and might change.
>> [...]
>> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
>> index d1217fcdedea..8605ab4a0f89 100644
>> --- a/include/linux/binfmts.h
>> +++ b/include/linux/binfmts.h
>> @@ -27,10 +27,10 @@ struct linux_binprm {
>> unsigned long argmin; /* rlimit marker for copy_strings() */
>> unsigned int
>> /*
>> - * True if most recent call to cap_bprm_set_creds
>> + * True if most recent call to security_bprm_set_creds
>> * resulted in elevated privileges.
>> */
>> -cap_elevated:1,
>> +active_secureexec:1,
>
> Also, I'd like it if this comment could be made more verbose as well, for
> anyone trying to understand the binfmt execution flow for the first time.
> Perhaps:
>
> /*
>* Must be set True during the any call to
>* bprm_set_creds hook where the execution would
>* reuslt in elevated privileges. (The hook can be
>* called multiple times during nested interpreter
>* resolution across binfmt_script, binfmt_misc, etc).
>*/
Well it is not during but after the call that it becomes true.
I think most recent covers the case of multiple calls.
I think having the loop explicitly in the code a few patches
later makes it clear that there is a loop dealing with interpreters.
Conciseness has a virtue in that it is easy to absorb. Seeing
active says most recent and secureexec does not is enough to ask
questions and look at the code.
Eric
Re: [PATCH v2 3/8] exec: Convert security_bprm_set_creds into security_bprm_repopulate_creds
On Mon, May 18, 2020 at 07:31:14PM -0500, Eric W. Biederman wrote:
>
> Rename bprm->cap_elevated to bprm->active_secureexec and initialize it
> in prepare_binprm instead of in cap_bprm_set_creds. Initializing
> bprm->active_secureexec in prepare_binprm allows multiple
> implementations of security_bprm_repopulate_creds to play nicely with
> each other.
>
> Rename security_bprm_set_creds to security_bprm_reopulate_creds to
> emphasize that this path recomputes part of bprm->cred. This
> recomputation avoids the time of check vs time of use problems that
> are inherent in unix #! interpreters.
>
> In short two renames and a move in the location of initializing
> bprm->active_secureexec.
I like this much better than the direct call to the capabilities hook.
Thanks!
Reviewed-by: Kees Cook
One nit is a bikeshed on the name "active_secureexec", since
the word "active" isn't really associated with any other part of the
binfmt logic. It's supposed to be "latest state from the binfmt loop",
so instead of "active", I considered these words that I also didn't
like: "current", "this", "recent", and "now". Is "latest" better than
"active"? Probably not.
> [...]
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index d1217fcdedea..8605ab4a0f89 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -27,10 +27,10 @@ struct linux_binprm {
> unsigned long argmin; /* rlimit marker for copy_strings() */
> unsigned int
> /*
> - * True if most recent call to cap_bprm_set_creds
> + * True if most recent call to security_bprm_set_creds
>* resulted in elevated privileges.
>*/
> - cap_elevated:1,
> + active_secureexec:1,
Also, I'd like it if this comment could be made more verbose as well, for
anyone trying to understand the binfmt execution flow for the first time.
Perhaps:
/*
* Must be set True during the any call to
* bprm_set_creds hook where the execution would
* reuslt in elevated privileges. (The hook can be
* called multiple times during nested interpreter
* resolution across binfmt_script, binfmt_misc, etc).
*/
--
Kees Cook
[PATCH v2 3/8] exec: Convert security_bprm_set_creds into security_bprm_repopulate_creds
Rename bprm->cap_elevated to bprm->active_secureexec and initialize it
in prepare_binprm instead of in cap_bprm_set_creds. Initializing
bprm->active_secureexec in prepare_binprm allows multiple
implementations of security_bprm_repopulate_creds to play nicely with
each other.
Rename security_bprm_set_creds to security_bprm_reopulate_creds to
emphasize that this path recomputes part of bprm->cred. This
recomputation avoids the time of check vs time of use problems that
are inherent in unix #! interpreters.
In short two renames and a move in the location of initializing
bprm->active_secureexec.
Signed-off-by: "Eric W. Biederman"
---
fs/exec.c | 8
include/linux/binfmts.h | 4 ++--
include/linux/lsm_hook_defs.h | 2 +-
include/linux/lsm_hooks.h | 4 ++--
include/linux/security.h | 8
security/commoncap.c | 9 -
security/security.c | 4 ++--
7 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 9e70da47f8d9..8e3b93d51d31 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1366,7 +1366,7 @@ int begin_new_exec(struct linux_binprm * bprm)
* the final state of setuid/setgid/fscaps can be merged into the
* secureexec flag.
*/
- bprm->secureexec |= bprm->cap_elevated;
+ bprm->secureexec |= bprm->active_secureexec;
if (bprm->secureexec) {
/* Make sure parent cannot signal privileged process. */
@@ -1634,10 +1634,10 @@ int prepare_binprm(struct linux_binprm *bprm)
int retval;
loff_t pos = 0;
+ /* Recompute parts of bprm->cred based on bprm->file */
+ bprm->active_secureexec = 0;
bprm_fill_uid(bprm);
-
- /* fill in binprm security blob */
- retval = security_bprm_set_creds(bprm);
+ retval = security_bprm_repopulate_creds(bprm);
if (retval)
return retval;
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index d1217fcdedea..8605ab4a0f89 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -27,10 +27,10 @@ struct linux_binprm {
unsigned long argmin; /* rlimit marker for copy_strings() */
unsigned int
/*
-* True if most recent call to cap_bprm_set_creds
+* True if most recent call to security_bprm_set_creds
* resulted in elevated privileges.
*/
- cap_elevated:1,
+ active_secureexec:1,
/*
* Set by bprm_creds_for_exec hook to indicate a
* privilege-gaining exec has happened. Used to set
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index aab0695f41df..1e295ba12c0d 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -50,7 +50,7 @@ LSM_HOOK(int, 0, settime, const struct timespec64 *ts,
const struct timezone *tz)
LSM_HOOK(int, 0, vm_enough_memory, struct mm_struct *mm, long pages)
LSM_HOOK(int, 0, bprm_creds_for_exec, struct linux_binprm *bprm)
-LSM_HOOK(int, 0, bprm_set_creds, struct linux_binprm *bprm)
+LSM_HOOK(int, 0, bprm_repopulate_creds, struct linux_binprm *bprm)
LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm)
LSM_HOOK(void, LSM_RET_VOID, bprm_committing_creds, struct linux_binprm *bprm)
LSM_HOOK(void, LSM_RET_VOID, bprm_committed_creds, struct linux_binprm *bprm)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index c719af37df20..d618ecc4d660 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -44,7 +44,7 @@
* request libc enable secure mode.
* @bprm contains the linux_binprm structure.
* Return 0 if the hook is successful and permission is granted.
- * @bprm_set_creds:
+ * @bprm_repopulate_creds:
* Assuming that the relevant bits of @bprm->cred->security have been
* previously set, examine @bprm->file and regenerate them. This is
* so that the credentials derived from the interpreter the code is
@@ -53,7 +53,7 @@
* reopen script, and may end up opening something completely different.
* This hook may also optionally check permissions (e.g. for
* transitions between security domains).
- * The hook must set @bprm->cap_elevated to 1 if AT_SECURE should be set to
+ * The hook must set @bprm->active_secureexec to 1 if AT_SECURE should be
set to
* request libc enable secure mode.
* @bprm contains the linux_binprm structure.
* Return 0 if the hook is successful and permission is granted.
diff --git a/include/linux/security.h b/include/linux/security.h
index 1bd7a6582775..d23f078eb589 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -140,7 +140,7 @@ extern int cap_capset(struct cred *new, const struct cred
*old,
const kernel_cap_t *effective,
const kernel_cap_t *inheritable,
