| Message ID | 20220613202852.447738-1-mortonm@chromium.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [1/2] security: Add LSM hook to setgroups() syscall | expand |
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 >
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 ...
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
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
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
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
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) };
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(-)