From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 19 May 2022 16:24:17 +0300 From: "Vladimir D. Seleznev" To: ALT Linux kernel packages development Message-ID: References: <20220518152458.2326124-1-vseleznv@altlinux.org> <20220518152458.2326124-2-vseleznv@altlinux.org> <20220519000923.wguielow262jpryr@altlinux.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220519000923.wguielow262jpryr@altlinux.org> Subject: Re: [d-kernel] [PATCH] AltHa: handle setcap binaries in the same way as setuid ones X-BeenThere: devel-kernel@lists.altlinux.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: ALT Linux kernel packages development List-Id: ALT Linux kernel packages development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 May 2022 13:24:20 -0000 Archived-At: List-Archive: List-Post: On Thu, May 19, 2022 at 03:09:23AM +0300, Vitaly Chikunov wrote: > Vladimir, > > On Wed, May 18, 2022 at 03:24:58PM +0000, Vladimir D. Seleznev wrote: > > altha.nosuid facility controls what binaries can raise user privilleges. > > Prior to this commit it only handled setuid binaries, but it was still > > possible to raise privilleges via setcaps. Now it handles both setuid > > and setcap binaries. > > > > Signed-off-by: Vladimir D. Seleznev > > --- > > You don't need to send cover letter for a single patch if you add your > comments here, after '---'. But you still need to add v5 next time. OK. > > Documentation/admin-guide/LSM/AltHa.rst | 6 ++-- > > security/altha/altha_lsm.c | 48 ++++++++++++++++++++----- > > 2 files changed, 43 insertions(+), 11 deletions(-) > > > > diff --git a/Documentation/admin-guide/LSM/AltHa.rst b/Documentation/admin-guide/LSM/AltHa.rst > > index be698709d3f0..beda40601c9e 100644 > > --- a/Documentation/admin-guide/LSM/AltHa.rst > > +++ b/Documentation/admin-guide/LSM/AltHa.rst > > @@ -3,7 +3,7 @@ AltHa > > ==== > > > > AltHa is a Linux Security Module currently has three userspace hardening options: > > - * ignore SUID on binaries (with exceptions possible); > > + * ignore SUID and setcaps on binaries (with exceptions possible); > > Perhaps, description of SECURITY_ALTHA in Kconfig should be > updated too, if I'm counting correctly. OK. > > * prevent running selected script interpreters in interactive mode; > > * disable open file unlinking in selected dirs. > > * enable kiosk mode > > @@ -15,12 +15,12 @@ through sysctls in ``/proc/sys/kernel/altha``. > > > > NoSUID > > ============ > > -Modern Linux systems can be used with minimal (or even zero at least for OWL and ALT) usage of SUID programms, but in many cases in full-featured desktop or server systems there are plenty of them: uncounted and sometimes unnecessary. Privileged programms are always an attack surface, but mounting filesystems with ``nosuid`` flag doesn't provide enough granularity in SUID binaries management. This LSM module provides a single control point for all SUID binaries. When this submodule is enabled, SUID bits on all binaries except explicitly listed are system-wide ignored. > > +Modern Linux systems can be used with minimal (or even zero at least for OWL and ALT) usage of SUID programms, but in many cases in full-featured desktop or server systems there are plenty of them: uncounted and sometimes unnecessary. Privileged programms are always an attack surface, but mounting filesystems with ``nosuid`` flag doesn't provide enough granularity in SUID binaries management. This LSM module provides a single control point for all SUID and setcap binaries. When this submodule is enabled, SUID and setcap bits on all binaries except explicitly listed are system-wide ignored. > > > > Sysctl parameters and defaults: > > > > * ``kernel.altha.nosuid.enabled = 0``, set to 1 to enable > > -* ``kernel.altha.nosuid.exceptions =``, colon-separated list of enabled SUID binaries, for example: ``/bin/su:/usr/libexec/hasher-priv/hasher-priv`` > > +* ``kernel.altha.nosuid.exceptions =``, colon-separated list of enabled SUID and setcap binaries, for example: ``/bin/su:/usr/libexec/hasher-priv/hasher-priv`` > > > > RestrScript > > ============ > > diff --git a/security/altha/altha_lsm.c b/security/altha/altha_lsm.c > > index c670ad7ed458..4f6b309445c0 100644 > > --- a/security/altha/altha_lsm.c > > +++ b/security/altha/altha_lsm.c > > @@ -11,6 +11,7 @@ > > > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -237,10 +238,19 @@ int is_olock_dir(struct inode *inode) > > return 0; > > } > > > > +static int has_any_caps(struct cred *cred) > > Why helper for a single use? Also, it checks definitely not for 'any' > caps. It makes code look cleaner. But OK I'll remove this helper. > > +{ > > + return !cap_isclear(cred->cap_permitted) || > > + !cap_isclear(cred->cap_effective); > > + > > + return 0; > > +} > > + > > /* Hooks */ > > static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * fi) > > { > > struct altha_list_struct *node; > > + char *setuidcap_srt = "setuid"; > > What is 'srt'? Please rename if it means 'str'. OK. > > /* when it's not a shebang issued script interpreter */ > > if (rstrscript_enabled && bprm->executable == bprm->interpreter) { > > char *path_p; > > @@ -267,11 +277,30 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f > > up_read(&interpreters_sem); > > kfree(path_buffer); > > } > > - if (unlikely(nosuid_enabled && > > - !uid_eq(bprm->cred->uid, bprm->cred->euid))) { > > + if (nosuid_enabled) { > > char *path_p; > > char *path_buffer; > > - uid_t cur_uid; > > + int is_setuid = 0, is_setcap = 0; > > + uid_t cur_uid, cur_euid; > > + > > + is_setuid = !uid_eq(bprm->cred->uid, bprm->cred->euid); > > It seems we want to restrict root to suid into user too, because this > way of switching users is never used. Perhaps, this decision should be > documented in comments. Or we can restrict only switching to superuser. What do you think would be a correct way? > > + > > + if (!is_setuid) { > > + cur_euid = from_kuid(bprm->cred->user_ns, bprm->cred->euid); > > + if (cur_euid != (uid_t) 0) > > + is_setcap = has_any_caps(bprm->cred); > > Perhaps, this should also be documented in comment why such complicated > logic of setting `is_setcap`. -- Because, exec by root always have > capabilities which does not imply setcap and you want to avoid this > situation and accidental drop of legitimate root capabilities. Isn't that obvious? > > + } > > + > > + /* > > + * If no suid but it has any caps, change message string from > > + * setuid to setcap. > > Isn't this comment is obvious? It is. > > + */ > > + if (is_setcap) > > + setuidcap_srt = "setcap"; > > Why not move this above when you set `is_setcap'. OK. > > + > > + /* If no suid and no caps detected, exit. */ > > + if (!is_setuid && !is_setcap) > > + return 0; > > > > path_buffer = kmalloc(PATH_MAX, GFP_KERNEL); > > if (!path_buffer) > > @@ -283,8 +312,8 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f > > list_for_each_entry(node, &nosuid_exceptions_list, list) { > > if (strcmp(path_p, node->spath) == 0) { > > pr_notice_ratelimited > > - ("AltHa/NoSUID: %s permitted to setuid from %d\n", > > - bprm->filename, cur_uid); > > + ("AltHa/NoSUID: %s permitted to %s from %d\n", > > + bprm->filename, setuidcap_srt, cur_uid); > > up_read(&nosuid_exceptions_sem); > > kfree(path_buffer); > > return 0; > > @@ -292,9 +321,12 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f > > } > > up_read(&nosuid_exceptions_sem); > > pr_notice_ratelimited > > - ("AltHa/NoSUID: %s prevented to setuid from %d\n", > > - bprm->filename, cur_uid); > > - bprm->cred->euid = bprm->cred->uid; > > + ("AltHa/NoSUID: %s prevented to %s from %d\n", > > + bprm->filename, setuidcap_srt, cur_uid); > > + if (is_setuid) > > + bprm->cred->euid = bprm->cred->uid; > > + cap_clear(bprm->cred->cap_permitted); > > + cap_clear(bprm->cred->cap_effective); > > Any exec under root will drop privileges, is it intended? No, this code does not run if there is no either setuid or setcap. Everything is fine. > I think it isn't. For example, run dmesg under root when > kernel.altha.nosuid.enabled=1 is set. > > I strongly suggest adding tests to this change. > > Thanks, > > > > kfree(path_buffer); > > } > > return 0; > > -- > > 2.33.3 > > -- WBR, Vladimir D. Seleznev