Re: [PATCH 1/2] exec: Add a per bprm->file version of per_clear
On Fri, May 29, 2020 at 10:23:58PM -0500, Eric W. Biederman wrote: > Kees Cook writes: > > I wish we had more robust execve tests. :( > > I think you have more skill at writing automated tests than I do. So > feel free to write some. Yeah, my limiting factor is available time. No worries; I didn't mean it as a request to you -- it was more a commiseration. :) -- Kees Cook
Re: [PATCH 1/2] exec: Add a per bprm->file version of per_clear
Kees Cook writes:
> 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"
>
> 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! :)
That plus it is very much true that active_ isn't a particularly good
prefix. pf_ for per_file seems slightly better.
The only time I can imagine this patch seeing the light of day is if
someone happens to discover that this fixes a bug for them and just this
patch is backported. At which point pf_per_clear pairs with
cap_elevated. So I don't think it hurts.
*Shrug*
The next patch is my long term solution to the mess.
> Reviewed-by: Kees Cook
>
> I wish we had more robust execve tests. :(
I think you have more skill at writing automated tests than I do. So
feel free to write some.
Eric
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"
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
I wish we had more robust execve tests. :(
--
Kees Cook
[PATCH 1/2] exec: Add a per bprm->file version of per_clear
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"
---
fs/exec.c | 6 --
include/linux/binfmts.h | 5 +
include/linux/lsm_hooks.h | 2 ++
security/commoncap.c | 2 +-
4 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index c3c879a55d65..0f793536e393 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1354,6 +1354,7 @@ int begin_new_exec(struct linux_binprm * bprm)
me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
PF_NOFREEZE | PF_NO_SETAFFINITY);
flush_thread();
+ bprm->per_clear |= bprm->pf_per_clear;
me->personality &= ~bprm->per_clear;
/*
@@ -1628,12 +1629,12 @@ static void bprm_fill_uid(struct linux_binprm *bprm)
return;
if (mode & S_ISUID) {
- bprm->per_clear |= PER_CLEAR_ON_SETID;
+ bprm->pf_per_clear |= PER_CLEAR_ON_SETID;
bprm->cred->euid = uid;
}
if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
- bprm->per_clear |= PER_CLEAR_ON_SETID;
+ bprm->pf_per_clear |= PER_CLEAR_ON_SETID;
bprm->cred->egid = gid;
}
}
@@ -1654,6 +1655,7 @@ static int prepare_binprm(struct linux_binprm *bprm)
/* Recompute parts of bprm->cred based on bprm->file */
bprm->active_secureexec = 0;
+ bprm->pf_per_clear = 0;
bprm_fill_uid(bprm);
retval = security_bprm_repopulate_creds(bprm);
if (retval)
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 7fc05929c967..50025ead0b72 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -55,6 +55,11 @@ struct linux_binprm {
struct file * file;
struct cred *cred; /* new credentials */
int unsafe; /* how unsafe this exec is (mask of
LSM_UNSAFE_*) */
+ /*
+* bits to clear in current->personality
+* recalculated for each bprm->file.
+*/
+ unsigned int pf_per_clear;
unsigned int per_clear; /* bits to clear in current->personality */
int argc, envc;
const char * filename; /* Name of binary as seen by procps */
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index d618ecc4d660..cd3dd0afceb5 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -55,6 +55,8 @@
* 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.
+ * The hook must set @bprm->pf_per_clear to the personality flags that
+ * should be cleared from current->personality.
* @bprm contains the linux_binprm structure.
* Return 0 if the hook is successful and permission is granted.
* @bprm_check_security:
diff --git a/security/commoncap.c b/security/commoncap.c
index 77b04cb6feac..6de72d22dc6c 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -826,7 +826,7 @@ int cap_bprm_repopulate_creds(struct linux_binprm *bprm)
/* if we have fs caps, clear dangerous personality flags */
if (__cap_gained(permitted, new, old))
- bprm->per_clear |= PER_CLEAR_ON_SETID;
+ bprm->pf_per_clear |= PER_CLEAR_ON_SETID;
/* Don't let someone trace a set[ug]id/setpcap binary with the revised
* credentials unless they have the appropriate permit.
--
2.25.0
