| Message ID | 87eer4ysm5.fsf_-_@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/11] exec: Reduce bprm->per_clear to a single bit | expand |
On Thu, May 28, 2020 at 8:45 AM Eric W. Biederman <ebiederm@xmission.com> wrote: > > - me->personality &= ~bprm->per_clear; > + if (bprm->per_clear) > + me->personality &= ~PER_CLEAR_ON_SETID;\ My only problem with this patch is that I find that 'per_clear' thing to be a horrid horrid name, Obviously the name didn't change, but the use *did* change, and as such the name got worse. It used do do things like bprm->per_clear |= PER_CLEAR_ON_SETID; and now it does bprm->per_clear = 1; and honestly, there's a lot more semantic context in the old code that is now missing entirely. At least you used to be able to grep for PER_CLEAR_ON_SETID and it would make you go "Ahh.." Put another way, I can kind of see what a line like bprm->per_clear |= PER_CLEAR_ON_SETID; does, simply because now it kind of hints at what is up. But what the heck does bprm->per_clear = 1; mean? Nothing. You have to really know the code. "per_clear" makes no sense, and now it's a short line that doesn't need to be that short. I think "bprm->clear_personality_bits" would maybe describe what the _effect_ of that field is. It doesn't explain _why_, but it at least explains "what" much better than "per_clear", which just makes me go "per what?". Alternatively, "bprm->creds_changed" would describe what the bit conceptually is about, and code like if (bprm->creds_changed) me->personality &= ~PER_CLEAR_ON_SETID;\ looks sensible to me and kind of matches the comment about the PER_CLEAR_ON_SETID bits are. So I think that using a bitfield is fine, but I'd really like it to be named something much better. Plus changing the name means that you can't have any code that now mistakenly uses the new semantics but expects the old bitmask. Generally when something changes semantics that radically, you want to make sure the type changes sufficiently that any out-of-tree patch that hasn't been merged yet will get a clear warning or error if people don't realize. Please? Linus
Linus Torvalds <torvalds@linux-foundation.org> writes: > On Thu, May 28, 2020 at 8:45 AM Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> - me->personality &= ~bprm->per_clear; >> + if (bprm->per_clear) >> + me->personality &= ~PER_CLEAR_ON_SETID;\ > > My only problem with this patch is that I find that 'per_clear' thing > to be a horrid horrid name, > > Obviously the name didn't change, but the use *did* change, and as > such the name got worse. It used do do things like > > bprm->per_clear |= PER_CLEAR_ON_SETID; > > and now it does > > bprm->per_clear = 1; > > and honestly, there's a lot more semantic context in the old code that > is now missing entirely. At least you used to be able to grep for > PER_CLEAR_ON_SETID and it would make you go "Ahh.." > > Put another way, I can kind of see what a line like > > bprm->per_clear |= PER_CLEAR_ON_SETID; > > does, simply because now it kind of hints at what is up. > > But what the heck does > > bprm->per_clear = 1; > > mean? Nothing. You have to really know the code. "per_clear" makes no > sense, and now it's a short line that doesn't need to be that short. > > I think "bprm->clear_personality_bits" would maybe describe what the > _effect_ of that field is. It doesn't explain _why_, but it at least > explains "what" much better than "per_clear", which just makes me go > "per what?". > > Alternatively, "bprm->creds_changed" would describe what the bit > conceptually is about, and code like > > if (bprm->creds_changed) > me->personality &= ~PER_CLEAR_ON_SETID;\ > > looks sensible to me and kind of matches the comment about the > PER_CLEAR_ON_SETID bits are. > > So I think that using a bitfield is fine, but I'd really like it to be > named something much better. > > Plus changing the name means that you can't have any code that now > mistakenly uses the new semantics but expects the old bitmask. > Generally when something changes semantics that radically, you want to > make sure the type changes sufficiently that any out-of-tree patch > that hasn't been merged yet will get a clear warning or error if > people don't realize. > > Please? Yes. That will make a very nice change to the patch. I think I will go with bprm->clear_unsafe_personality_bits or something to that effect. I would really love to have a bit that means creds_changes or privilegeds_elevated. But right now we have 2 of two fields that mean essentially that (per_clear and secureexec) and they don't agree on when they get set. I will make them agree as much as possible, and this patchset is a first step in that direction but until we can actually make them agree, I want to keep them both grounded in what they do. That way it is possible to have a reasonable discussion on when they should be set. Eric
diff --git a/fs/exec.c b/fs/exec.c index c3c879a55d65..51fab62b9fca 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1354,7 +1354,8 @@ int begin_new_exec(struct linux_binprm * bprm) me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | PF_NOFREEZE | PF_NO_SETAFFINITY); flush_thread(); - me->personality &= ~bprm->per_clear; + if (bprm->per_clear) + me->personality &= ~PER_CLEAR_ON_SETID; /* * We have to apply CLOEXEC before we change whether the process is @@ -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->per_clear = 1; bprm->cred->euid = uid; } if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) { - bprm->per_clear |= PER_CLEAR_ON_SETID; + bprm->per_clear = 1; bprm->cred->egid = gid; } } diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 7fc05929c967..e7959a6a895a 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -26,6 +26,9 @@ struct linux_binprm { unsigned long p; /* current top of mem */ unsigned long argmin; /* rlimit marker for copy_strings() */ unsigned int + /* Should unsafe personality bits be cleared? */ + per_clear:1, + /* Should an execfd be passed to userspace? */ have_execfd:1, @@ -55,7 +58,6 @@ struct linux_binprm { struct file * file; struct cred *cred; /* new credentials */ int unsafe; /* how unsafe this exec is (mask of LSM_UNSAFE_*) */ - unsigned int per_clear; /* bits to clear in current->personality */ int argc, envc; const char * filename; /* Name of binary as seen by procps */ const char * interp; /* Name of the binary really executed. Most diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index d618ecc4d660..0ca68ad53592 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -42,6 +42,8 @@ * (e.g. for transitions between security domains). * The hook must set @bprm->secureexec to 1 if AT_SECURE should be set to * request libc enable secure mode. + * The hook must set @bprm->per_clear to 1 if the dangerous personality + * bits must be cleared from current->personality. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. * @bprm_repopulate_creds: @@ -55,6 +57,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->per_clear to 1 if the dangerous personality + * bits must 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/apparmor/domain.c b/security/apparmor/domain.c index 0b870a647488..c6d00735a40a 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -962,7 +962,7 @@ int apparmor_bprm_creds_for_exec(struct linux_binprm *bprm) aa_label_printk(new, GFP_KERNEL); dbg_printk("\n"); } - bprm->per_clear |= PER_CLEAR_ON_SETID; + bprm->per_clear = 1; } aa_put_label(cred_label(bprm->cred)); /* transfer reference, released when cred is freed */ diff --git a/security/commoncap.c b/security/commoncap.c index 77b04cb6feac..48b556046483 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->per_clear = 1; /* Don't let someone trace a set[ug]id/setpcap binary with the revised * credentials unless they have the appropriate permit. diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 718345dd76bb..6bea1b879fdb 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2385,7 +2385,7 @@ static int selinux_bprm_creds_for_exec(struct linux_binprm *bprm) } /* Clear any possibly unsafe personality bits on exec: */ - bprm->per_clear |= PER_CLEAR_ON_SETID; + bprm->per_clear = 1; /* Enable secure mode for SIDs transitions unless the noatsecure permission is granted between diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 0ac8f4518d07..a0d2fad27b33 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -933,7 +933,7 @@ static int smack_bprm_creds_for_exec(struct linux_binprm *bprm) return -EPERM; bsp->smk_task = isp->smk_task; - bprm->per_clear |= PER_CLEAR_ON_SETID; + bprm->per_clear = 1; /* Decide if this is a secure exec. */ if (bsp->smk_task != bsp->smk_forked)
The bprm->per_clear field only takes the values 0 and PER_CLEAR_ON_SETID. Reduce the field to a signle bit to make it clear that the only question is should the dangerous personality bits be cleared or not. Update the documentation of the security lsm hooks. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/exec.c | 7 ++++--- include/linux/binfmts.h | 4 +++- include/linux/lsm_hooks.h | 4 ++++ security/apparmor/domain.c | 2 +- security/commoncap.c | 2 +- security/selinux/hooks.c | 2 +- security/smack/smack_lsm.c | 2 +- 7 files changed, 15 insertions(+), 8 deletions(-)