Message ID | 20250306082615.174777-1-max.kellermann@ionos.com (mailing list archive) |
---|---|
State | Handled Elsewhere |
Headers | show |
Series | security/commoncap: don't assume "setid" if all ids are identical | expand |
Hi Max, kernel test robot noticed the following build warnings: [auto build test WARNING on pcmoore-selinux/next] [also build test WARNING on linus/master v6.14-rc5 next-20250306] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Max-Kellermann/security-commoncap-don-t-assume-setid-if-all-ids-are-identical/20250306-162826 base: https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git next patch link: https://lore.kernel.org/r/20250306082615.174777-1-max.kellermann%40ionos.com patch subject: [PATCH] security/commoncap: don't assume "setid" if all ids are identical config: arc-allnoconfig (https://download.01.org/0day-ci/archive/20250307/202503071808.FE4vwUc4-lkp@intel.com/config) compiler: arc-elf-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250307/202503071808.FE4vwUc4-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202503071808.FE4vwUc4-lkp@intel.com/ All warnings (new ones prefixed by >>): >> security/commoncap.c:865: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst * Are all user/group ids in both cred instances identical? vim +865 security/commoncap.c 863 864 /** > 865 * Are all user/group ids in both cred instances identical? 866 * 867 * It can be used after __is_setuid() / __is_setgid() to check whether 868 * this is really a set*id operation or whether both processes just 869 * have differing real/effective ids. It is safe to keep differing 870 * real/effective ids in "unsafe" program execution. 871 */ 872 static bool has_identical_uids_gids(const struct cred *a, const struct cred *b) 873 { 874 return uid_eq(a->uid, b->uid) && 875 gid_eq(a->gid, b->gid) && 876 uid_eq(a->suid, b->suid) && 877 gid_eq(a->sgid, b->sgid) && 878 uid_eq(a->euid, b->euid) && 879 gid_eq(a->egid, b->egid) && 880 uid_eq(a->fsuid, b->fsuid) && 881 gid_eq(a->fsgid, b->fsgid); 882 } 883
On Thu, Mar 06, 2025 at 09:26:15AM +0100, Max Kellermann wrote: > If a program enables `NO_NEW_PRIVS` and sets up > differing real/effective/saved/fs ids, the effective ids are > downgraded during exec because the kernel believes it should "get no > more than they had, and maybe less". > > I believe it is safe to keep differing ids even if `NO_NEW_PRIVS` is > set. The newly executed program doesn't get any more, but there's no > reason to give it less. > > This is different from "set[ug]id/setpcap" execution where privileges > may be raised; here, the assumption that it's "set[ug]id" if > effective!=real is too broad. > > If we verify that all user/group ids remain as they were, we can > safely allow the new program to keep them. Thanks, it's an interesting point. Seems to mainly depend on what users of the feature have come to expect. Andy, what do you think? > Signed-off-by: Max Kellermann <max.kellermann@ionos.com> > --- > security/commoncap.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/security/commoncap.c b/security/commoncap.c > index 58a0c1c3e409..057a7400ef7d 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -861,6 +861,26 @@ static inline bool __is_setuid(struct cred *new, const struct cred *old) > static inline bool __is_setgid(struct cred *new, const struct cred *old) > { return !gid_eq(new->egid, old->gid); } > > +/** > + * Are all user/group ids in both cred instances identical? > + * > + * It can be used after __is_setuid() / __is_setgid() to check whether > + * this is really a set*id operation or whether both processes just > + * have differing real/effective ids. It is safe to keep differing > + * real/effective ids in "unsafe" program execution. > + */ > +static bool has_identical_uids_gids(const struct cred *a, const struct cred *b) > +{ > + return uid_eq(a->uid, b->uid) && > + gid_eq(a->gid, b->gid) && > + uid_eq(a->suid, b->suid) && > + gid_eq(a->sgid, b->sgid) && > + uid_eq(a->euid, b->euid) && > + gid_eq(a->egid, b->egid) && > + uid_eq(a->fsuid, b->fsuid) && > + gid_eq(a->fsgid, b->fsgid); > +} > + > /* > * 1) Audit candidate if current->cap_effective is set > * > @@ -940,7 +960,8 @@ 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); > + is_setid = (__is_setuid(new, old) || __is_setgid(new, old)) && > + !has_identical_uids_gids(new, old); > > if ((is_setid || __cap_gained(permitted, new, old)) && > ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) || > -- > 2.47.2
diff --git a/security/commoncap.c b/security/commoncap.c index 58a0c1c3e409..057a7400ef7d 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -861,6 +861,26 @@ static inline bool __is_setuid(struct cred *new, const struct cred *old) static inline bool __is_setgid(struct cred *new, const struct cred *old) { return !gid_eq(new->egid, old->gid); } +/** + * Are all user/group ids in both cred instances identical? + * + * It can be used after __is_setuid() / __is_setgid() to check whether + * this is really a set*id operation or whether both processes just + * have differing real/effective ids. It is safe to keep differing + * real/effective ids in "unsafe" program execution. + */ +static bool has_identical_uids_gids(const struct cred *a, const struct cred *b) +{ + return uid_eq(a->uid, b->uid) && + gid_eq(a->gid, b->gid) && + uid_eq(a->suid, b->suid) && + gid_eq(a->sgid, b->sgid) && + uid_eq(a->euid, b->euid) && + gid_eq(a->egid, b->egid) && + uid_eq(a->fsuid, b->fsuid) && + gid_eq(a->fsgid, b->fsgid); +} + /* * 1) Audit candidate if current->cap_effective is set * @@ -940,7 +960,8 @@ 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); + is_setid = (__is_setuid(new, old) || __is_setgid(new, old)) && + !has_identical_uids_gids(new, old); if ((is_setid || __cap_gained(permitted, new, old)) && ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
If a program enables `NO_NEW_PRIVS` and sets up differing real/effective/saved/fs ids, the effective ids are downgraded during exec because the kernel believes it should "get no more than they had, and maybe less". I believe it is safe to keep differing ids even if `NO_NEW_PRIVS` is set. The newly executed program doesn't get any more, but there's no reason to give it less. This is different from "set[ug]id/setpcap" execution where privileges may be raised; here, the assumption that it's "set[ug]id" if effective!=real is too broad. If we verify that all user/group ids remain as they were, we can safely allow the new program to keep them. Signed-off-by: Max Kellermann <max.kellermann@ionos.com> --- security/commoncap.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)