| Message ID | 20240904104824.1844082-7-ivanov.mikhail1@huawei-partners.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Support socket access-control | expand |
Hi! On Wed, Sep 04, 2024 at 06:48:11PM +0800, Mikhail Ivanov wrote: > Add test that validates behaviour of Landlock after rule with > unhandled access is added. > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> > --- > Changes since v2: > * Replaces EXPECT_EQ with ASSERT_EQ for close(). > * Refactors commit title and message. > > Changes since v1: > * Refactors commit message. > --- > .../testing/selftests/landlock/socket_test.c | 33 +++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c > index 811bdaa95a7a..d2fedfca7193 100644 > --- a/tools/testing/selftests/landlock/socket_test.c > +++ b/tools/testing/selftests/landlock/socket_test.c > @@ -351,4 +351,37 @@ TEST_F(protocol, rule_with_unknown_access) > ASSERT_EQ(0, close(ruleset_fd)); > } > > +TEST_F(protocol, rule_with_unhandled_access) > +{ > + struct landlock_ruleset_attr ruleset_attr = { > + .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE, > + }; > + struct landlock_socket_attr protocol = { > + .family = self->prot.family, > + .type = self->prot.type, > + }; > + int ruleset_fd; > + __u64 access; > + > + ruleset_fd = > + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); > + ASSERT_LE(0, ruleset_fd); > + > + for (access = 1; access > 0; access <<= 1) { > + int err; > + > + protocol.allowed_access = access; > + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET, > + &protocol, 0); > + if (access == ruleset_attr.handled_access_socket) { > + EXPECT_EQ(0, err); > + } else { > + EXPECT_EQ(-1, err); > + EXPECT_EQ(EINVAL, errno); > + } > + } > + > + ASSERT_EQ(0, close(ruleset_fd)); > +} > + I should probably have noticed this on the first review round; you are not actually exercising any scenario here where a rule with unhandled access is added. To clarify, the notion of an access right being "unhandled" means that the access right was not listed at ruleset creation time in the ruleset_attr's .handled_access_* field where it would have belonged. If that is the case, adding a ruleset with that access right is going to be denied. As an example: If the ruleset only handles LANDLOCK_ACCESS_FS_WRITE_FILE and nothing else, then, if the test tries to insert a rule for LANDLOCK_ACCESS_SOCKET_CREATE, that call is supposed to fail -- because the "socket creation" access right is not handled. IMHO the test would become more reasonable if it was more clearly "handling" something entirely unrelated at ruleset creation time, e.g. one of the file system access rights. (And we could do the same for the "net" and "fs" tests as well.) Your test is a copy of the same test for the "net" rights, which in turn is a copy of teh same test for the "fs" rights. When the "fs" test was written, the "fs" access rights were the only ones that could be used at all to create a ruleset, but this is not true any more. —Günther
On 9/10/2024 12:22 PM, Günther Noack wrote: > Hi! > > On Wed, Sep 04, 2024 at 06:48:11PM +0800, Mikhail Ivanov wrote: >> Add test that validates behaviour of Landlock after rule with >> unhandled access is added. >> >> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> >> --- >> Changes since v2: >> * Replaces EXPECT_EQ with ASSERT_EQ for close(). >> * Refactors commit title and message. >> >> Changes since v1: >> * Refactors commit message. >> --- >> .../testing/selftests/landlock/socket_test.c | 33 +++++++++++++++++++ >> 1 file changed, 33 insertions(+) >> >> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c >> index 811bdaa95a7a..d2fedfca7193 100644 >> --- a/tools/testing/selftests/landlock/socket_test.c >> +++ b/tools/testing/selftests/landlock/socket_test.c >> @@ -351,4 +351,37 @@ TEST_F(protocol, rule_with_unknown_access) >> ASSERT_EQ(0, close(ruleset_fd)); >> } >> >> +TEST_F(protocol, rule_with_unhandled_access) >> +{ >> + struct landlock_ruleset_attr ruleset_attr = { >> + .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE, >> + }; >> + struct landlock_socket_attr protocol = { >> + .family = self->prot.family, >> + .type = self->prot.type, >> + }; >> + int ruleset_fd; >> + __u64 access; >> + >> + ruleset_fd = >> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); >> + ASSERT_LE(0, ruleset_fd); >> + >> + for (access = 1; access > 0; access <<= 1) { >> + int err; >> + >> + protocol.allowed_access = access; >> + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET, >> + &protocol, 0); >> + if (access == ruleset_attr.handled_access_socket) { >> + EXPECT_EQ(0, err); >> + } else { >> + EXPECT_EQ(-1, err); >> + EXPECT_EQ(EINVAL, errno); >> + } >> + } >> + >> + ASSERT_EQ(0, close(ruleset_fd)); >> +} >> + > > I should probably have noticed this on the first review round; you are not > actually exercising any scenario here where a rule with unhandled access is > added. > > To clarify, the notion of an access right being "unhandled" means that the > access right was not listed at ruleset creation time in the ruleset_attr's > .handled_access_* field where it would have belonged. If that is the case, > adding a ruleset with that access right is going to be denied. > > As an example: > If the ruleset only handles LANDLOCK_ACCESS_FS_WRITE_FILE and nothing else, > then, if the test tries to insert a rule for LANDLOCK_ACCESS_SOCKET_CREATE, > that call is supposed to fail -- because the "socket creation" access right is > not handled. This test was added to exercise adding a rule with future possible "unhandled" access rights of "socket" type, but since this patch implements only one, this test is really meaningless. Thank you for this note! > > IMHO the test would become more reasonable if it was more clearly "handling" > something entirely unrelated at ruleset creation time, e.g. one of the file > system access rights. (And we could do the same for the "net" and "fs" tests as > well.) > > Your test is a copy of the same test for the "net" rights, which in turn is a > copy of teh same test for the "fs" rights. When the "fs" test was written, the > "fs" access rights were the only ones that could be used at all to create a > ruleset, but this is not true any more. Good idea! Can I implement such test in the current patchset? > > —Günther
On Wed, Sep 11, 2024 at 11:19:48AM +0300, Mikhail Ivanov wrote: > On 9/10/2024 12:22 PM, Günther Noack wrote: > > Hi! > > > > On Wed, Sep 04, 2024 at 06:48:11PM +0800, Mikhail Ivanov wrote: > > > Add test that validates behaviour of Landlock after rule with > > > unhandled access is added. > > > > > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> > > > --- > > > Changes since v2: > > > * Replaces EXPECT_EQ with ASSERT_EQ for close(). > > > * Refactors commit title and message. > > > > > > Changes since v1: > > > * Refactors commit message. > > > --- > > > .../testing/selftests/landlock/socket_test.c | 33 +++++++++++++++++++ > > > 1 file changed, 33 insertions(+) > > > > > > diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c > > > index 811bdaa95a7a..d2fedfca7193 100644 > > > --- a/tools/testing/selftests/landlock/socket_test.c > > > +++ b/tools/testing/selftests/landlock/socket_test.c > > > @@ -351,4 +351,37 @@ TEST_F(protocol, rule_with_unknown_access) > > > ASSERT_EQ(0, close(ruleset_fd)); > > > } > > > +TEST_F(protocol, rule_with_unhandled_access) > > > +{ > > > + struct landlock_ruleset_attr ruleset_attr = { > > > + .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE, > > > + }; > > > + struct landlock_socket_attr protocol = { > > > + .family = self->prot.family, > > > + .type = self->prot.type, > > > + }; > > > + int ruleset_fd; > > > + __u64 access; > > > + > > > + ruleset_fd = > > > + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); > > > + ASSERT_LE(0, ruleset_fd); > > > + > > > + for (access = 1; access > 0; access <<= 1) { > > > + int err; > > > + > > > + protocol.allowed_access = access; > > > + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET, > > > + &protocol, 0); > > > + if (access == ruleset_attr.handled_access_socket) { > > > + EXPECT_EQ(0, err); > > > + } else { > > > + EXPECT_EQ(-1, err); > > > + EXPECT_EQ(EINVAL, errno); > > > + } > > > + } > > > + > > > + ASSERT_EQ(0, close(ruleset_fd)); > > > +} > > > + > > > > I should probably have noticed this on the first review round; you are not > > actually exercising any scenario here where a rule with unhandled access is > > added. > > > > To clarify, the notion of an access right being "unhandled" means that the > > access right was not listed at ruleset creation time in the ruleset_attr's > > .handled_access_* field where it would have belonged. If that is the case, > > adding a ruleset with that access right is going to be denied. > > > > As an example: > > If the ruleset only handles LANDLOCK_ACCESS_FS_WRITE_FILE and nothing else, > > then, if the test tries to insert a rule for LANDLOCK_ACCESS_SOCKET_CREATE, > > that call is supposed to fail -- because the "socket creation" access right is > > not handled. > > This test was added to exercise adding a rule with future possible > "unhandled" access rights of "socket" type, but since this patch > implements only one, this test is really meaningless. Thank you for > this note! > > > > > IMHO the test would become more reasonable if it was more clearly "handling" > > something entirely unrelated at ruleset creation time, e.g. one of the file > > system access rights. (And we could do the same for the "net" and "fs" tests as > > well.) > > > > Your test is a copy of the same test for the "net" rights, which in turn is a > > copy of teh same test for the "fs" rights. When the "fs" test was written, the > > "fs" access rights were the only ones that could be used at all to create a > > ruleset, but this is not true any more. > > Good idea! Can I implement such test in the current patchset? Yes, I think it would be a good idea. I would, in fact, recommend to turn the rule_with_unhandled_access test into that test. The test traces its roots clearly to TEST_F(mini, rule_with_unhandled_access) from net_test.c and to TEST_F_FORK(layout1, rule_with_unhandled_access) from fs_test.c and I think all three variants would better be advised to create a ruleset with struct landlock_ruleset_attr ruleset_attr = { .handled_access_something_entirely_different = LANDLOCK_ACCESS_WHATEVER, } and then check their corresponding fs, net and socket access rights using a landlock_add_rule() call for the access rights that belong to the respective module, so that it exercises the scenario where userspace attempts to use the access right in a rule, but the surrounding ruleset did not restrict the same access right (it was "unhandled"). In spirit, it would be nicest if we could create a ruleset where nothing at all is handled, but I believe in that case, the landlock_create_ruleset() call would already fail. —Günther P.S.: I am starting to grow a bit uncomfortable with the amount of duplicated test code that we start having across the different types of access rights. If you see a way to keep this more in check, while still keeping the tests expressive and not over-frameworking them, let's try to move in that direction if we can. :)
On 9/13/2024 6:04 PM, Günther Noack wrote: > On Wed, Sep 11, 2024 at 11:19:48AM +0300, Mikhail Ivanov wrote: >> On 9/10/2024 12:22 PM, Günther Noack wrote: >>> Hi! >>> >>> On Wed, Sep 04, 2024 at 06:48:11PM +0800, Mikhail Ivanov wrote: >>>> Add test that validates behaviour of Landlock after rule with >>>> unhandled access is added. >>>> >>>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> >>>> --- >>>> Changes since v2: >>>> * Replaces EXPECT_EQ with ASSERT_EQ for close(). >>>> * Refactors commit title and message. >>>> >>>> Changes since v1: >>>> * Refactors commit message. >>>> --- >>>> .../testing/selftests/landlock/socket_test.c | 33 +++++++++++++++++++ >>>> 1 file changed, 33 insertions(+) >>>> >>>> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c >>>> index 811bdaa95a7a..d2fedfca7193 100644 >>>> --- a/tools/testing/selftests/landlock/socket_test.c >>>> +++ b/tools/testing/selftests/landlock/socket_test.c >>>> @@ -351,4 +351,37 @@ TEST_F(protocol, rule_with_unknown_access) >>>> ASSERT_EQ(0, close(ruleset_fd)); >>>> } >>>> +TEST_F(protocol, rule_with_unhandled_access) >>>> +{ >>>> + struct landlock_ruleset_attr ruleset_attr = { >>>> + .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE, >>>> + }; >>>> + struct landlock_socket_attr protocol = { >>>> + .family = self->prot.family, >>>> + .type = self->prot.type, >>>> + }; >>>> + int ruleset_fd; >>>> + __u64 access; >>>> + >>>> + ruleset_fd = >>>> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); >>>> + ASSERT_LE(0, ruleset_fd); >>>> + >>>> + for (access = 1; access > 0; access <<= 1) { >>>> + int err; >>>> + >>>> + protocol.allowed_access = access; >>>> + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET, >>>> + &protocol, 0); >>>> + if (access == ruleset_attr.handled_access_socket) { >>>> + EXPECT_EQ(0, err); >>>> + } else { >>>> + EXPECT_EQ(-1, err); >>>> + EXPECT_EQ(EINVAL, errno); >>>> + } >>>> + } >>>> + >>>> + ASSERT_EQ(0, close(ruleset_fd)); >>>> +} >>>> + >>> >>> I should probably have noticed this on the first review round; you are not >>> actually exercising any scenario here where a rule with unhandled access is >>> added. >>> >>> To clarify, the notion of an access right being "unhandled" means that the >>> access right was not listed at ruleset creation time in the ruleset_attr's >>> .handled_access_* field where it would have belonged. If that is the case, >>> adding a ruleset with that access right is going to be denied. >>> >>> As an example: >>> If the ruleset only handles LANDLOCK_ACCESS_FS_WRITE_FILE and nothing else, >>> then, if the test tries to insert a rule for LANDLOCK_ACCESS_SOCKET_CREATE, >>> that call is supposed to fail -- because the "socket creation" access right is >>> not handled. >> >> This test was added to exercise adding a rule with future possible >> "unhandled" access rights of "socket" type, but since this patch >> implements only one, this test is really meaningless. Thank you for >> this note! >> >>> >>> IMHO the test would become more reasonable if it was more clearly "handling" >>> something entirely unrelated at ruleset creation time, e.g. one of the file >>> system access rights. (And we could do the same for the "net" and "fs" tests as >>> well.) >>> >>> Your test is a copy of the same test for the "net" rights, which in turn is a >>> copy of teh same test for the "fs" rights. When the "fs" test was written, the >>> "fs" access rights were the only ones that could be used at all to create a >>> ruleset, but this is not true any more. >> >> Good idea! Can I implement such test in the current patchset? > > Yes, I think it would be a good idea. > > I would, in fact, recommend to turn the rule_with_unhandled_access test into that test. > > The test traces its roots clearly to > > TEST_F(mini, rule_with_unhandled_access) from net_test.c > > and to > > TEST_F_FORK(layout1, rule_with_unhandled_access) from fs_test.c > > > and I think all three variants would better be advised to create a ruleset with > > struct landlock_ruleset_attr ruleset_attr = { > .handled_access_something_entirely_different = LANDLOCK_ACCESS_WHATEVER, > } > > and then check their corresponding fs, net and socket access rights using a > landlock_add_rule() call for the access rights that belong to the respective > module, so that it exercises the scenario where userspace attempts to use the > access right in a rule, but the surrounding ruleset did not restrict the same > access right (it was "unhandled"). Agreed, thanks for the recommendation! > > In spirit, it would be nicest if we could create a ruleset where nothing at all > is handled, but I believe in that case, the landlock_create_ruleset() call would > already fail. > > —Günther > > P.S.: I am starting to grow a bit uncomfortable with the amount of duplicated > test code that we start having across the different types of access rights. If > you see a way to keep this more in check, while still keeping the tests > expressive and not over-frameworking them, let's try to move in that direction > if we can. :) Yeah, I really want to see patchset dedicated to tests refactoring. I'll try to finish the description of corresponding issue [1] ASAP. [1] https://github.com/landlock-lsm/linux/issues/34
diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c index 811bdaa95a7a..d2fedfca7193 100644 --- a/tools/testing/selftests/landlock/socket_test.c +++ b/tools/testing/selftests/landlock/socket_test.c @@ -351,4 +351,37 @@ TEST_F(protocol, rule_with_unknown_access) ASSERT_EQ(0, close(ruleset_fd)); } +TEST_F(protocol, rule_with_unhandled_access) +{ + struct landlock_ruleset_attr ruleset_attr = { + .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE, + }; + struct landlock_socket_attr protocol = { + .family = self->prot.family, + .type = self->prot.type, + }; + int ruleset_fd; + __u64 access; + + ruleset_fd = + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); + ASSERT_LE(0, ruleset_fd); + + for (access = 1; access > 0; access <<= 1) { + int err; + + protocol.allowed_access = access; + err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET, + &protocol, 0); + if (access == ruleset_attr.handled_access_socket) { + EXPECT_EQ(0, err); + } else { + EXPECT_EQ(-1, err); + EXPECT_EQ(EINVAL, errno); + } + } + + ASSERT_EQ(0, close(ruleset_fd)); +} + TEST_HARNESS_MAIN
Add test that validates behaviour of Landlock after rule with unhandled access is added. Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> --- Changes since v2: * Replaces EXPECT_EQ with ASSERT_EQ for close(). * Refactors commit title and message. Changes since v1: * Refactors commit message. --- .../testing/selftests/landlock/socket_test.c | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+)