Skip to content

Commit

Permalink
exec: Add a per bprm->file version of per_clear
Browse files Browse the repository at this point in the history
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")
Reviewed-by: Kees Cook <[email protected]>
Signed-off-by: "Eric W. Biederman" <[email protected]>
  • Loading branch information
ebiederm committed May 30, 2020
1 parent e32f887 commit a786832
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 3 deletions.
6 changes: 4 additions & 2 deletions fs/exec.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/*
Expand Down Expand Up @@ -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;
}
}
Expand All @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions include/linux/binfmts.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
2 changes: 2 additions & 0 deletions include/linux/lsm_hooks.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 add to @bprm->pf_per_clear any 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:
Expand Down
2 changes: 1 addition & 1 deletion security/commoncap.c
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit a786832

Please sign in to comment.