| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aCey42ACEr4h3fmH@lei>
Date: Fri, 16 May 2025 21:49:23 +0000
From: sergeh@...nel.org
To: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: Kees Cook <keescook@...omium.org>,
Max Kellermann <max.kellermann@...os.com>,
"Serge E. Hallyn" <serge@...lyn.com>, paul@...l-moore.com,
jmorris@...ei.org, Andy Lutomirski <luto@...nel.org>,
morgan@...nel.org, Christian Brauner <christian@...uner.io>,
linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] exec: Correct the permission check for unsafe exec
On Thu, May 15, 2025 at 11:24:47AM -0500, Eric W. Biederman wrote:
>
> Max Kellerman recently experienced a problem[1] when calling exec with
> differing uid and euid's and he triggered the logic that is supposed
> to only handle setuid executables.
>
> When exec isn't changing anything in struct cred it doesn't make sense
> to go into the code that is there to handle the case when the
> credentials change.
>
> When looking into the history of the code I discovered that this issue
> was not present in Linux-2.4.0-test12 and was introduced in
> Linux-2.4.0-prerelease when the logic for handling this case was moved
> from prepare_binprm to compute_creds in fs/exec.c.
>
> The bug introduced was to comparing euid in the new credentials with
> uid instead of euid in the old credentials, when testing if setuid
> had changed the euid.
>
> Since triggering the keep ptrace limping along case for setuid
> executables makes no sense when it was not a setuid exec revert back
> to the logic present in Linux-2.4.0-test12.
>
> This removes the confusingly named and subtlety incorrect helpers
> is_setuid and is_setgid, that helped this bug to persist.
>
> The variable is_setid is renamed to id_changed (it's Linux-2.4.0-test12
> name) as the old name describes matters rather than it's cause.
>
> The code removed in Linux-2.4.0-prerelease was:
> - /* Set-uid? */
> - if (mode & S_ISUID) {
> - bprm->e_uid = inode->i_uid;
> - if (bprm->e_uid != current->euid)
> - id_change = 1;
> - }
> -
> - /* Set-gid? */
> - /*
> - * If setgid is set but no group execute bit then this
> - * is a candidate for mandatory locking, not a setgid
> - * executable.
> - */
> - if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
> - bprm->e_gid = inode->i_gid;
> - if (!in_group_p(bprm->e_gid))
> - id_change = 1;
>
> Linux-2.4.0-prerelease added the current logic as:
> + if (bprm->e_uid != current->uid || bprm->e_gid != current->gid ||
> + !cap_issubset(new_permitted, current->cap_permitted)) {
> + current->dumpable = 0;
> +
> + lock_kernel();
> + if (must_not_trace_exec(current)
> + || atomic_read(¤t->fs->count) > 1
> + || atomic_read(¤t->files->count) > 1
> + || atomic_read(¤t->sig->count) > 1) {
> + if(!capable(CAP_SETUID)) {
> + bprm->e_uid = current->uid;
> + bprm->e_gid = current->gid;
> + }
> + if(!capable(CAP_SETPCAP)) {
> + new_permitted = cap_intersect(new_permitted,
> + current->cap_permitted);
> + }
> + }
> + do_unlock = 1;
> + }
>
> I have condensed the logic from Linux-2.4.0-test12 to just:
> id_changed = !uid_eq(new->euid, old->euid) || !in_group_p(new->egid);
>
>
> This change is userspace visible, but I don't expect anyone to care.
>
> For the bug that is being fixed to trigger bprm->unsafe has to be set.
> The variable bprm->unsafe is set when ptracing an executable, when
> sharing a working directory, or when no_new_privs is set. Properly
> testing for cases that are safe even in those conditions and doing
> nothing special should not affect anyone. Especially if they were
> previously ok with their credentials getting munged
>
> Reported-by: Max Kellermann <max.kellermann@...os.com>
> Fixes: 64444d3d0d7f ("Linux version 2.4.0-prerelease")
> [1] https://lkml.kernel.org/r/20250306082615.174777-1-max.kellermann@ionos.com
> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com>
Thanks, Eric.
Obviously we should continue to discuss Jann's concern and the
potential change in behavior, but I'm hugely in favor of this patch.
Reviewed-by: Serge Hallyn <serge@...lyn.com>
> ---
> security/commoncap.c | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 28d4248bf001..96c7654f2012 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -856,12 +856,6 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap,
> #define __cap_full(field, cred) \
> cap_issubset(CAP_FULL_SET, cred->cap_##field)
>
> -static inline bool __is_setuid(struct cred *new, const struct cred *old)
> -{ return !uid_eq(new->euid, old->uid); }
> -
> -static inline bool __is_setgid(struct cred *new, const struct cred *old)
> -{ return !gid_eq(new->egid, old->gid); }
> -
> /*
> * 1) Audit candidate if current->cap_effective is set
> *
> @@ -891,7 +885,7 @@ static inline bool nonroot_raised_pE(struct cred *new, const struct cred *old,
> (root_privileged() &&
> __is_suid(root, new) &&
> !__cap_full(effective, new)) ||
> - (!__is_setuid(new, old) &&
> + (uid_eq(new->euid, old->euid) &&
> ((has_fcap &&
> __cap_gained(permitted, new, old)) ||
> __cap_gained(ambient, new, old))))
> @@ -917,7 +911,7 @@ int cap_bprm_creds_from_file(struct linux_binprm *bprm, const struct file *file)
> /* Process setpcap binaries and capabilities for uid 0 */
> const struct cred *old = current_cred();
> struct cred *new = bprm->cred;
> - bool effective = false, has_fcap = false, is_setid;
> + bool effective = false, has_fcap = false, id_changed;
> int ret;
> kuid_t root_uid;
>
> @@ -941,9 +935,9 @@ int cap_bprm_creds_from_file(struct linux_binprm *bprm, const struct file *file)
> *
> * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
> */
> - is_setid = __is_setuid(new, old) || __is_setgid(new, old);
> + id_changed = !uid_eq(new->euid, old->euid) || !in_group_p(new->egid);
>
> - if ((is_setid || __cap_gained(permitted, new, old)) &&
> + if ((id_changed || __cap_gained(permitted, new, old)) &&
> ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
> !ptracer_capable(current, new->user_ns))) {
> /* downgrade; they get no more than they had, and maybe less */
> @@ -960,7 +954,7 @@ int cap_bprm_creds_from_file(struct linux_binprm *bprm, const struct file *file)
> new->sgid = new->fsgid = new->egid;
>
> /* File caps or setid cancels ambient. */
> - if (has_fcap || is_setid)
> + if (has_fcap || id_changed)
> cap_clear(new->cap_ambient);
>
> /*
> @@ -993,7 +987,7 @@ int cap_bprm_creds_from_file(struct linux_binprm *bprm, const struct file *file)
> return -EPERM;
>
> /* Check for privilege-elevated exec. */
> - if (is_setid ||
> + if (id_changed ||
> (!__is_real(root_uid, new) &&
> (effective ||
> __cap_grew(permitted, ambient, new))))
> --
> 2.41.0
>
Powered by blists - more mailing lists