Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

exec: Correct the permission check for unsafe exec

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 introdused 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 varaiable is_setid is renamed to id_changed (it's Linux-2.4.0-test12)
as the old name describes what 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(&current->fs->count) > 1
+ || atomic_read(&current->files->count) > 1
+ || atomic_read(&current->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 condenced 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

To minimize behavioural changes the code continues to set secureexec
when euid != uid or when egid != gid.

[1] https://lkml.kernel.org/r/20250306082615.174777-1-max.kellermann@ionos.com

Reported-by: Max Kellermann <max.kellermann@ionos.com>
Fixes: 64444d3d0d7f ("Linux version 2.4.0-prerelease")
v1: https://lkml.kernel.org/r/878qmxsuy8.fsf@email.froward.int.ebiederm.org
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
Reviewed-by: Jann Horn <jannh@google.com>
Acked-by: Kees Cook <kees@kernel.org>

authored by

Eric W. Biederman and committed by
Serge Hallyn
337490f0 19272b37

+8 -12
+8 -12
security/commoncap.c
··· 856 856 #define __cap_full(field, cred) \ 857 857 cap_issubset(CAP_FULL_SET, cred->cap_##field) 858 858 859 - static inline bool __is_setuid(struct cred *new, const struct cred *old) 860 - { return !uid_eq(new->euid, old->uid); } 861 - 862 - static inline bool __is_setgid(struct cred *new, const struct cred *old) 863 - { return !gid_eq(new->egid, old->gid); } 864 - 865 859 /* 866 860 * 1) Audit candidate if current->cap_effective is set 867 861 * ··· 885 891 (root_privileged() && 886 892 __is_suid(root, new) && 887 893 !__cap_full(effective, new)) || 888 - (!__is_setuid(new, old) && 894 + (uid_eq(new->euid, old->euid) && 889 895 ((has_fcap && 890 896 __cap_gained(permitted, new, old)) || 891 897 __cap_gained(ambient, new, old)))) ··· 911 917 /* Process setpcap binaries and capabilities for uid 0 */ 912 918 const struct cred *old = current_cred(); 913 919 struct cred *new = bprm->cred; 914 - bool effective = false, has_fcap = false, is_setid; 920 + bool effective = false, has_fcap = false, id_changed; 915 921 int ret; 916 922 kuid_t root_uid; 917 923 ··· 935 941 * 936 942 * In addition, if NO_NEW_PRIVS, then ensure we get no new privs. 937 943 */ 938 - is_setid = __is_setuid(new, old) || __is_setgid(new, old); 944 + id_changed = !uid_eq(new->euid, old->euid) || !in_group_p(new->egid); 939 945 940 - if ((is_setid || __cap_gained(permitted, new, old)) && 946 + if ((id_changed || __cap_gained(permitted, new, old)) && 941 947 ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) || 942 948 !ptracer_capable(current, new->user_ns))) { 943 949 /* downgrade; they get no more than they had, and maybe less */ ··· 954 960 new->sgid = new->fsgid = new->egid; 955 961 956 962 /* File caps or setid cancels ambient. */ 957 - if (has_fcap || is_setid) 963 + if (has_fcap || id_changed) 958 964 cap_clear(new->cap_ambient); 959 965 960 966 /* ··· 987 993 return -EPERM; 988 994 989 995 /* Check for privilege-elevated exec. */ 990 - if (is_setid || 996 + if (id_changed || 997 + !uid_eq(new->euid, old->uid) || 998 + !gid_eq(new->egid, old->gid) || 991 999 (!__is_real(root_uid, new) && 992 1000 (effective || 993 1001 __cap_grew(permitted, ambient, new))))