| Message ID | 1576577571-3668-4-git-send-email-xuyang2018.jy@cn.fujitsu.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | cleanup capset testcase | expand |
Hi! > +static void setup(void) > +{ > + pid_t pid; > + > + pid = getpid(); > + header.pid = pid; > + if (geteuid() == 0) { > + TEST(tst_syscall(__NR_capset, &header, data)); > + if (TST_RET == -1) > + tst_brk(TBROK | TTERRNO, "capset data failed"); > + } Please don't do that. If tests needs root (even for a subset of the test) just set the .needs_root flag. > +} > + > +static struct tst_test test = { > + .setup = setup, > + .test_all = verify_capset, > + .caps = (struct tst_cap []) { > + TST_CAP(TST_CAP_DROP, CAP_SETPCAP), > + {} > + }, > +}; > -- > 2.18.0 > > > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
Hi > Hi! >> +static void setup(void) >> +{ >> + pid_t pid; >> + >> + pid = getpid(); >> + header.pid = pid; >> + if (geteuid() == 0) { >> + TEST(tst_syscall(__NR_capset, &header, data)); >> + if (TST_RET == -1) >> + tst_brk(TBROK | TTERRNO, "capset data failed"); >> + } > > Please don't do that. If tests needs root (even for a subset of the > test) just set the .needs_root flag. > This test doesn't need root. These code is designed to create a envrionment for root user to generate this type EPERM error（new_Inheritable is not a subset of old_Inheritable and old_Permitted without CAP_SETPCAP）. root user: old pI: CAP_KILL old pP: CAP_KILL new pI: CAP_KILL + CAP_NET_RAW other user: old pI: 0 old pP: 0 new pI: CAP_KILL + CAP_NET_RAW other user also met condition and can generate this EPERM error. ps: In capset03, getpid() is useless, we can use pid = 0 to replace. Also, if we can use pid =0 in error test, maybe we don't need to test pid =0 in capget01/capset01.c . What do you think about it? >> +} >> + >> +static struct tst_test test = { >> + .setup = setup, >> + .test_all = verify_capset, >> + .caps = (struct tst_cap []) { >> + TST_CAP(TST_CAP_DROP, CAP_SETPCAP), >> + {} >> + }, >> +}; >> -- >> 2.18.0 >> >> >> >> >> -- >> Mailing list info: https://lists.linux.it/listinfo/ltp >
Hi! > >> +static void setup(void) > >> +{ > >> + pid_t pid; > >> + > >> + pid = getpid(); > >> + header.pid = pid; > >> + if (geteuid() == 0) { > >> + TEST(tst_syscall(__NR_capset, &header, data)); > >> + if (TST_RET == -1) > >> + tst_brk(TBROK | TTERRNO, "capset data failed"); > >> + } > > > > Please don't do that. If tests needs root (even for a subset of the > > test) just set the .needs_root flag. > > > This test doesn't need root. These code is designed to create a > envrionment for root user to generate this type EPERM > error??new_Inheritable is not a subset of old_Inheritable and > old_Permitted without CAP_SETPCAP??. > root user: > old pI: CAP_KILL > old pP: CAP_KILL > new pI: CAP_KILL + CAP_NET_RAW > > other user: > old pI: 0 > old pP: 0 > new pI: CAP_KILL + CAP_NET_RAW > > other user also met condition and can generate this EPERM error. Hmm, we are testing different things under root and non-root then. When the test is executed under a regular user we assert that the system default is sane + the capset assertion, while under the root we test only capset. It would make sense to run the test only under root to make sure that we are consistent. Also the CAP_DROP in the tst_test structure seems to be useless to me. Looking at man 7 capabilities, there are also transitions defined for what is supposed to happen when we change user id. It would make sense to write tests that capabilities are correctly dropped when UID changes from 0 to nonzero. Which is what this test is testing when executed as non-root, since the transition from 0 to nonzero must have happened somewhere when user has logged in. > ps: In capset03, getpid() is useless, we can use pid = 0 to replace. > Also, if we can use pid =0 in error test, maybe we don't need to test > pid =0 in capget01/capset01.c . What do you think about it? Sure. We can use 0 in all tests if we have a least one for each syscall that tests it with pid != 0.
Hi! > Hi! >>>> +static void setup(void) >>>> +{ >>>> + pid_t pid; >>>> + >>>> + pid = getpid(); >>>> + header.pid = pid; >>>> + if (geteuid() == 0) { >>>> + TEST(tst_syscall(__NR_capset, &header, data)); >>>> + if (TST_RET == -1) >>>> + tst_brk(TBROK | TTERRNO, "capset data failed"); >>>> + } >>> >>> Please don't do that. If tests needs root (even for a subset of the >>> test) just set the .needs_root flag. >>> >> This test doesn't need root. These code is designed to create a >> envrionment for root user to generate this type EPERM >> error??new_Inheritable is not a subset of old_Inheritable and >> old_Permitted without CAP_SETPCAP??. >> root user: >> old pI: CAP_KILL >> old pP: CAP_KILL >> new pI: CAP_KILL + CAP_NET_RAW >> >> other user: >> old pI: 0 >> old pP: 0 >> new pI: CAP_KILL + CAP_NET_RAW >> >> other user also met condition and can generate this EPERM error. > > Hmm, we are testing different things under root and non-root then. When > the test is executed under a regular user we assert that the system > default is sane + the capset assertion, while under the root we test > only capset. > > It would make sense to run the test only under root to make sure that we > are consistent. > Ok. I will make this case consistent and add .need_root flag. > Also the CAP_DROP in the tst_test structure seems to be useless to me. > > > Looking at man 7 capabilities, there are also transitions defined for > what is supposed to happen when we change user id. It would make sense > to write tests that capabilities are correctly dropped when UID changes > from 0 to nonzero. Which is what this test is testing when executed as > non-root, since the transition from 0 to nonzero must have happened > somewhere when user has logged in. In man 7 capabilities " Effect of user ID changes on capabilities", I see transitions between 0 and nonzero user IDs. But it is about capabilities，not about capset syscall. I think we should add these cases(user ID changes on capabilities) into kernel/security (such as cap_bound or filecaps). In capset, we can only test capset various EPERM error as kernel sercurity/commoncap.c cap_capset function. --------------------------------- if (cap_inh_is_capped() && !cap_issubset(*inheritable, cap_combine(old->cap_inheritable, old->cap_permitted))) /* incapable of using this inheritable set */ return -EPERM; if (!cap_issubset(*inheritable, cap_combine(old->cap_inheritable, old->cap_bset))) /* no new pI capabilities outside bounding set */ return -EPERM; /* verify restrictions on target's new Permitted set */ if (!cap_issubset(*permitted, old->cap_permitted)) return -EPERM; /* verify the _new_Effective_ is a subset of the _new_Permitted_ */ if (!cap_issubset(*effective, *permitted)) return -EPERM; --------------------------------- Also, if we only run under root, CAP_DROP(CAP_SETPCAP) is needed to reproduce this EPERM error. > >> ps: In capset03, getpid() is useless, we can use pid = 0 to replace. >> Also, if we can use pid =0 in error test, maybe we don't need to test >> pid =0 in capget01/capset01.c . What do you think about it? > > Sure. We can use 0 in all tests if we have a least one for each syscall > that tests it with pid != 0. Ok. I will do it. >
Hi! > > Also the CAP_DROP in the tst_test structure seems to be useless to me. > > > > > > Looking at man 7 capabilities, there are also transitions defined for > > what is supposed to happen when we change user id. It would make sense > > to write tests that capabilities are correctly dropped when UID changes > > from 0 to nonzero. Which is what this test is testing when executed as > > non-root, since the transition from 0 to nonzero must have happened > > somewhere when user has logged in. > In man 7 capabilities " Effect of user ID changes on capabilities", > I see transitions between 0 and nonzero user IDs. But it is about > capabilities??not about capset syscall. I think we should add these > cases(user ID changes on capabilities) into kernel/security (such as > cap_bound or filecaps). In capset, we can only test capset various EPERM > error as kernel sercurity/commoncap.c cap_capset function. > --------------------------------- > if (cap_inh_is_capped() && > !cap_issubset(*inheritable, > cap_combine(old->cap_inheritable, > old->cap_permitted))) > /* incapable of using this inheritable set */ > return -EPERM; > > if (!cap_issubset(*inheritable, > cap_combine(old->cap_inheritable, > old->cap_bset))) > /* no new pI capabilities outside bounding set */ > return -EPERM; > > /* verify restrictions on target's new Permitted set */ > if (!cap_issubset(*permitted, old->cap_permitted)) > return -EPERM; > > /* verify the _new_Effective_ is a subset of the _new_Permitted_ */ > if (!cap_issubset(*effective, *permitted)) > return -EPERM; > --------------------------------- Indeed these does not belog under setcap(). Maybe we could add these checks under setuid tests, since we are testing side efect of setuid. But having these under security/ would work as well. > Also, if we only run under root, CAP_DROP(CAP_SETPCAP) is needed to > reproduce this EPERM error. Isn't the first thing that the test does to remove all capabilities but CAP_KILL? Why do we need to drop anything beforehand?
Hi! > Hi! >>> Also the CAP_DROP in the tst_test structure seems to be useless to me. >>> >>> >>> Looking at man 7 capabilities, there are also transitions defined for >>> what is supposed to happen when we change user id. It would make sense >>> to write tests that capabilities are correctly dropped when UID changes >>> from 0 to nonzero. Which is what this test is testing when executed as >>> non-root, since the transition from 0 to nonzero must have happened >>> somewhere when user has logged in. >> In man 7 capabilities " Effect of user ID changes on capabilities", >> I see transitions between 0 and nonzero user IDs. But it is about >> capabilities??not about capset syscall. I think we should add these >> cases(user ID changes on capabilities) into kernel/security (such as >> cap_bound or filecaps). In capset, we can only test capset various EPERM >> error as kernel sercurity/commoncap.c cap_capset function. >> --------------------------------- >> if (cap_inh_is_capped() && >> !cap_issubset(*inheritable, >> cap_combine(old->cap_inheritable, >> old->cap_permitted))) >> /* incapable of using this inheritable set */ >> return -EPERM; >> >> if (!cap_issubset(*inheritable, >> cap_combine(old->cap_inheritable, >> old->cap_bset))) >> /* no new pI capabilities outside bounding set */ >> return -EPERM; >> >> /* verify restrictions on target's new Permitted set */ >> if (!cap_issubset(*permitted, old->cap_permitted)) >> return -EPERM; >> >> /* verify the _new_Effective_ is a subset of the _new_Permitted_ */ >> if (!cap_issubset(*effective, *permitted)) >> return -EPERM; >> --------------------------------- > > Indeed these does not belog under setcap(). Maybe we could add these > checks under setuid tests, since we are testing side efect of setuid. > But having these under security/ would work as well.Maybe put them in setuid is better because I don't know a good directory name for them in security(such as user_change_cap). Anyway, I will list them in my todo. > >> Also, if we only run under root, CAP_DROP(CAP_SETPCAP) is needed to >> reproduce this EPERM error. > > Isn't the first thing that the test does to remove all capabilities but > CAP_KILL? Why do we need to drop anything beforehand? Yes, you are right. I forgot it. I will remove this drop and also used guarded buffer for the other capset cases . >
diff --git a/runtest/syscalls b/runtest/syscalls index fa87ef63f..4f481be6d 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -44,6 +44,7 @@ capget02 capget02 capset01 capset01 capset02 capset02 +capset03 capset03 cacheflush01 cacheflush01 diff --git a/testcases/kernel/syscalls/capset/.gitignore b/testcases/kernel/syscalls/capset/.gitignore index 004ce7b3e..3f9a4d5e8 100644 --- a/testcases/kernel/syscalls/capset/.gitignore +++ b/testcases/kernel/syscalls/capset/.gitignore @@ -1,2 +1,3 @@ /capset01 /capset02 +/capset03 diff --git a/testcases/kernel/syscalls/capset/capset03.c b/testcases/kernel/syscalls/capset/capset03.c new file mode 100644 index 000000000..d973095a4 --- /dev/null +++ b/testcases/kernel/syscalls/capset/capset03.c @@ -0,0 +1,65 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2019 FUJITSU LIMITED. All rights reserved. + * Author: Yang Xu <xuyang2018.jy@cn.fujitsu.com + * + * capset() fails with errno set or EPERM if the new_Inheritable is + * not a subset of old_Inheritable and old_Permitted without CAP_SETPCAP. + */ +#include <stdlib.h> +#include <sys/types.h> +#include <unistd.h> +#include <sys/prctl.h> +#include "tst_test.h" +#include "lapi/syscalls.h" +#include <linux/capability.h> + +static struct __user_cap_header_struct header = { + .version = 0x20080522, + .pid = 0, +}; + +static struct __user_cap_data_struct data[2] = { + { + .effective = 1 << CAP_KILL, + .permitted = 1 << CAP_KILL, + .inheritable = 1 << CAP_KILL, + }, +}; + +static void verify_capset(void) +{ + tst_res(TINFO, "Test bad value data(when pI is not old pP or old pI without CAP_SETPCAP)"); + data[0].inheritable = (1 << CAP_KILL | 1 << CAP_NET_RAW); + TEST(tst_syscall(__NR_capset, &header, data)); + if (TST_RET == 0) { + tst_res(TFAIL, "capset succeed unexpectedly"); + return; + } + if (TST_ERR == EPERM) + tst_res(TPASS | TTERRNO, "capset() failed as expected"); + else + tst_res(TFAIL | TTERRNO, "capset expected EPERM, bug got"); +} + +static void setup(void) +{ + pid_t pid; + + pid = getpid(); + header.pid = pid; + if (geteuid() == 0) { + TEST(tst_syscall(__NR_capset, &header, data)); + if (TST_RET == -1) + tst_brk(TBROK | TTERRNO, "capset data failed"); + } +} + +static struct tst_test test = { + .setup = setup, + .test_all = verify_capset, + .caps = (struct tst_cap []) { + TST_CAP(TST_CAP_DROP, CAP_SETPCAP), + {} + }, +};
Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> --- runtest/syscalls | 1 + testcases/kernel/syscalls/capset/.gitignore | 1 + testcases/kernel/syscalls/capset/capset03.c | 65 +++++++++++++++++++++ 3 files changed, 67 insertions(+) create mode 100644 testcases/kernel/syscalls/capset/capset03.c