Re: [PATCH 2/2] LSM: SafeSetID: Add setgroups() security policy handling

[Micah Morton]

On Mon, Jun 13, 2022 at 1:28 PM Micah Morton <mortonm@xxxxxxxxxxxx> wrote:
>
> The SafeSetID LSM has functionality for restricting setuid()/setgid()
> syscalls based on its configured security policies. This patch adds the
> analogous functionality for the setgroups() syscall. Security policy
> for the setgroups() syscall follows the same policies that are
> installed on the system for setgid() syscalls.
>
> Signed-off-by: Micah Morton <mortonm@xxxxxxxxxxxx>
> ---
> NOTE: this code does nothing to prevent a SafeSetID-restricted process
> with CAP_SETGID from dropping supplementary groups. I don't anticipate
> supplementary groups ever being used to restrict a process' privileges
> (rather than grant privileges), so I think this is fine for the
> purposes of SafeSetID.
>
> Developed on 5.18
>
>  security/safesetid/lsm.c | 39 ++++++++++++++++++++++++++++++---------
>  1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> index 963f4ad9cb66..01c355e740aa 100644
> --- a/security/safesetid/lsm.c
> +++ b/security/safesetid/lsm.c
> @@ -97,15 +97,9 @@ static int safesetid_security_capable(const struct cred *cred,
>                 return 0;
>
>         /*
> -        * If CAP_SET{U/G}ID is currently used for a setid() syscall, we want to
> -        * let it go through here; the real security check happens later, in the
> -        * task_fix_set{u/g}id hook.
> -         *
> -         * NOTE:
> -         * Until we add support for restricting setgroups() calls, GID security
> -         * policies offer no meaningful security since we always return 0 here
> -         * when called from within the setgroups() syscall and there is no
> -         * additional hook later on to enforce security policies for setgroups().
> +        * If CAP_SET{U/G}ID is currently used for a setid or setgroups syscall, we
> +        * want to let it go through here; the real security check happens later, in
> +        * the task_fix_set{u/g}id or task_fix_setgroups hooks.
>          */
>         if ((opts & CAP_OPT_INSETID) != 0)
>                 return 0;
> @@ -241,9 +235,36 @@ static int safesetid_task_fix_setgid(struct cred *new,
>         return -EACCES;
>  }
>
> +static int safesetid_task_fix_setgroups(struct cred *new, const struct cred *old)
> +{
> +       int i;
> +
> +       /* Do nothing if there are no setgid restrictions for our old RGID. */
> +       if (setid_policy_lookup((kid_t){.gid = old->gid}, INVALID_ID, GID) == SIDPOL_DEFAULT)
> +               return 0;
> +
> +       get_group_info(new->group_info);
> +       for (i = 0; i < new->group_info->ngroups; i++) {
> +               if (!id_permitted_for_cred(old, (kid_t){.gid = group_info->gid[i]}, GID)) {

Oops, should be:

!id_permitted_for_cred(old, (kid_t){.gid = new->group_info->gid[i]}, GID)

Guess I won't send a whole new patch just for that one line

> +                       put_group_info(new->group_info);
> +                       /*
> +                        * Kill this process to avoid potential security vulnerabilities
> +                        * that could arise from a missing allowlist entry preventing a
> +                        * privileged process from dropping to a lesser-privileged one.
> +                        */
> +                       force_sig(SIGKILL);
> +                       return -EACCES;
> +               }
> +       }
> +
> +       put_group_info(new->group_info);
> +       return 0;
> +}
> +
>  static struct security_hook_list safesetid_security_hooks[] = {
>         LSM_HOOK_INIT(task_fix_setuid, safesetid_task_fix_setuid),
>         LSM_HOOK_INIT(task_fix_setgid, safesetid_task_fix_setgid),
> +       LSM_HOOK_INIT(task_fix_setgroups, safesetid_task_fix_setgroups),
>         LSM_HOOK_INIT(capable, safesetid_security_capable)
>  };
>
> --
> 2.36.1.476.g0c4daa206d-goog
>