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

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

[Micah Morton]

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@chromium.org>
---
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)) {
+			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

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@chromium.org> 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@chromium.org>
> ---
> 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
>

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

[Kees Cook]

On Mon, Jun 13, 2022 at 02:00:03PM -0700, Micah Morton wrote:
> On Mon, Jun 13, 2022 at 1:28 PM Micah Morton <mortonm@chromium.org> wrote:
> [...]
> > +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

This begs the question: are there self-tests for this LSM somewhere?
It'd be really nice to add them to tool/testing/selftests ...

-- 
Kees Cook

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

[kernel test robot]

Hi Micah,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on jmorris-security/next-testing kees/for-next/pstore v5.19-rc2 next-20220610]
[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]

url:    https://github.com/intel-lab-lkp/linux/commits/Micah-Morton/security-Add-LSM-hook-to-setgroups-syscall/20220614-050341
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git b13baccc3850ca8b8cccbf8ed9912dbaa0fdf7f3
config: arc-randconfig-r043-20220613 (https://download.01.org/0day-ci/archive/20220614/202206141217.8YUKCl5p-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/248aa1aeef5c49d4af78b9c3d09e896413258c76
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Micah-Morton/security-Add-LSM-hook-to-setgroups-syscall/20220614-050341
        git checkout 248aa1aeef5c49d4af78b9c3d09e896413258c76
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   security/safesetid/lsm.c: In function 'safesetid_task_fix_setgroups':
>> security/safesetid/lsm.c:248:64: error: 'group_info' undeclared (first use in this function)
     248 |                 if (!id_permitted_for_cred(old, (kid_t){.gid = group_info->gid[i]}, GID)) {
         |                                                                ^~~~~~~~~~
   security/safesetid/lsm.c:248:64: note: each undeclared identifier is reported only once for each function it appears in


vim +/group_info +248 security/safesetid/lsm.c

   237	
   238	static int safesetid_task_fix_setgroups(struct cred *new, const struct cred *old)
   239	{
   240		int i;
   241	
   242		/* Do nothing if there are no setgid restrictions for our old RGID. */
   243		if (setid_policy_lookup((kid_t){.gid = old->gid}, INVALID_ID, GID) == SIDPOL_DEFAULT)
   244			return 0;
   245	
   246		get_group_info(new->group_info);
   247		for (i = 0; i < new->group_info->ngroups; i++) {
 > 248			if (!id_permitted_for_cred(old, (kid_t){.gid = group_info->gid[i]}, GID)) {
   249				put_group_info(new->group_info);
   250				/*
   251				 * Kill this process to avoid potential security vulnerabilities
   252				 * that could arise from a missing allowlist entry preventing a
   253				 * privileged process from dropping to a lesser-privileged one.
   254				 */
   255				force_sig(SIGKILL);
   256				return -EACCES;
   257			}
   258		}
   259	
   260		put_group_info(new->group_info);
   261		return 0;
   262	}
   263	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

Hi Micah,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on jmorris-security/next-testing kees/for-next/pstore v5.19-rc2 next-20220610]
[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]

url:    https://github.com/intel-lab-lkp/linux/commits/Micah-Morton/security-Add-LSM-hook-to-setgroups-syscall/20220614-050341
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git b13baccc3850ca8b8cccbf8ed9912dbaa0fdf7f3
config: x86_64-randconfig-a001-20220613 (https://download.01.org/0day-ci/archive/20220614/202206141555.zswTLROZ-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c97436f8b6e2718286e8496faf53a2c800e281cf)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/248aa1aeef5c49d4af78b9c3d09e896413258c76
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Micah-Morton/security-Add-LSM-hook-to-setgroups-syscall/20220614-050341
        git checkout 248aa1aeef5c49d4af78b9c3d09e896413258c76
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> security/safesetid/lsm.c:248:50: error: use of undeclared identifier 'group_info'
                   if (!id_permitted_for_cred(old, (kid_t){.gid = group_info->gid[i]}, GID)) {
                                                                  ^
   1 error generated.


vim +/group_info +248 security/safesetid/lsm.c

   237	
   238	static int safesetid_task_fix_setgroups(struct cred *new, const struct cred *old)
   239	{
   240		int i;
   241	
   242		/* Do nothing if there are no setgid restrictions for our old RGID. */
   243		if (setid_policy_lookup((kid_t){.gid = old->gid}, INVALID_ID, GID) == SIDPOL_DEFAULT)
   244			return 0;
   245	
   246		get_group_info(new->group_info);
   247		for (i = 0; i < new->group_info->ngroups; i++) {
 > 248			if (!id_permitted_for_cred(old, (kid_t){.gid = group_info->gid[i]}, GID)) {
   249				put_group_info(new->group_info);
   250				/*
   251				 * Kill this process to avoid potential security vulnerabilities
   252				 * that could arise from a missing allowlist entry preventing a
   253				 * privileged process from dropping to a lesser-privileged one.
   254				 */
   255				force_sig(SIGKILL);
   256				return -EACCES;
   257			}
   258		}
   259	
   260		put_group_info(new->group_info);
   261		return 0;
   262	}
   263	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[Micah Morton]

On Mon, Jun 13, 2022 at 4:46 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Jun 13, 2022 at 02:00:03PM -0700, Micah Morton wrote:
> > On Mon, Jun 13, 2022 at 1:28 PM Micah Morton <mortonm@chromium.org> wrote:
> > [...]
> > > +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
>
> This begs the question: are there self-tests for this LSM somewhere?
> It'd be really nice to add them to tool/testing/selftests ...

There actually is tools/testing/selftests/safesetid/ but I haven't
updated it since v1 of SafeSetID that only accommodated UIDs. I've
been relying on integration testing we have out of tree on the Chrome
OS side but I suppose its reasonable to bring the selftest up to date
as well :)

Also both patches are a couple lines off from the ones I was finished
developing/testing with.. some kind of screw up happened when I copied
from my dev machine to another where I could get git-send-email
working properly. I'll just resend these 2 patches along with the
update to the selftest.

Thanks

>
> --
> Kees Cook

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

[Micah Morton]

On Tue, Jun 14, 2022 at 10:36 AM Micah Morton <mortonm@chromium.org> wrote:
>
> On Mon, Jun 13, 2022 at 4:46 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, Jun 13, 2022 at 02:00:03PM -0700, Micah Morton wrote:
> > > On Mon, Jun 13, 2022 at 1:28 PM Micah Morton <mortonm@chromium.org> wrote:
> > > [...]
> > > > +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
> >
> > This begs the question: are there self-tests for this LSM somewhere?
> > It'd be really nice to add them to tool/testing/selftests ...
>
> There actually is tools/testing/selftests/safesetid/ but I haven't
> updated it since v1 of SafeSetID that only accommodated UIDs. I've
> been relying on integration testing we have out of tree on the Chrome
> OS side but I suppose its reasonable to bring the selftest up to date
> as well :)
>
> Also both patches are a couple lines off from the ones I was finished
> developing/testing with.. some kind of screw up happened when I copied
> from my dev machine to another where I could get git-send-email
> working properly. I'll just resend these 2 patches along with the
> update to the selftest.

Just sent the updated patches and selftest patch.

>
> Thanks
>
> >
> > --
> > Kees Cook