Skip to content

Commit a786832

Browse files
committed
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: 1bb0fa1 ("[PATCH] NX: clean up legacy binary support") Reviewed-by: Kees Cook <[email protected]> Signed-off-by: "Eric W. Biederman" <[email protected]>
1 parent e32f887 commit a786832

File tree

4 files changed

+12
-3
lines changed

4 files changed

+12
-3
lines changed

fs/exec.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1354,6 +1354,7 @@ int begin_new_exec(struct linux_binprm * bprm)
13541354
me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
13551355
PF_NOFREEZE | PF_NO_SETAFFINITY);
13561356
flush_thread();
1357+
bprm->per_clear |= bprm->pf_per_clear;
13571358
me->personality &= ~bprm->per_clear;
13581359

13591360
/*
@@ -1628,12 +1629,12 @@ static void bprm_fill_uid(struct linux_binprm *bprm)
16281629
return;
16291630

16301631
if (mode & S_ISUID) {
1631-
bprm->per_clear |= PER_CLEAR_ON_SETID;
1632+
bprm->pf_per_clear |= PER_CLEAR_ON_SETID;
16321633
bprm->cred->euid = uid;
16331634
}
16341635

16351636
if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
1636-
bprm->per_clear |= PER_CLEAR_ON_SETID;
1637+
bprm->pf_per_clear |= PER_CLEAR_ON_SETID;
16371638
bprm->cred->egid = gid;
16381639
}
16391640
}
@@ -1654,6 +1655,7 @@ static int prepare_binprm(struct linux_binprm *bprm)
16541655

16551656
/* Recompute parts of bprm->cred based on bprm->file */
16561657
bprm->active_secureexec = 0;
1658+
bprm->pf_per_clear = 0;
16571659
bprm_fill_uid(bprm);
16581660
retval = security_bprm_repopulate_creds(bprm);
16591661
if (retval)

include/linux/binfmts.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,11 @@ struct linux_binprm {
5555
struct file * file;
5656
struct cred *cred; /* new credentials */
5757
int unsafe; /* how unsafe this exec is (mask of LSM_UNSAFE_*) */
58+
/*
59+
* bits to clear in current->personality
60+
* recalculated for each bprm->file.
61+
*/
62+
unsigned int pf_per_clear;
5863
unsigned int per_clear; /* bits to clear in current->personality */
5964
int argc, envc;
6065
const char * filename; /* Name of binary as seen by procps */

include/linux/lsm_hooks.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@
5555
* transitions between security domains).
5656
* The hook must set @bprm->active_secureexec to 1 if AT_SECURE should be set to
5757
* request libc enable secure mode.
58+
* The hook must add to @bprm->pf_per_clear any personality flags that
59+
* should be cleared from current->personality.
5860
* @bprm contains the linux_binprm structure.
5961
* Return 0 if the hook is successful and permission is granted.
6062
* @bprm_check_security:

security/commoncap.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -826,7 +826,7 @@ int cap_bprm_repopulate_creds(struct linux_binprm *bprm)
826826

827827
/* if we have fs caps, clear dangerous personality flags */
828828
if (__cap_gained(permitted, new, old))
829-
bprm->per_clear |= PER_CLEAR_ON_SETID;
829+
bprm->pf_per_clear |= PER_CLEAR_ON_SETID;
830830

831831
/* Don't let someone trace a set[ug]id/setpcap binary with the revised
832832
* credentials unless they have the appropriate permit.

0 commit comments

Comments
 (0)