| Message ID | 20230502171755.9788-4-gnoack3000@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Landlock: ioctl support | expand |
On 02/05/2023 19:17, Günther Noack wrote: > Exercise the use of Landlock's ioctl restriction: If ioctl is > restricted, the use of ioctl fails with a freshly opened /dev/tty > file. > > Signed-off-by: Günther Noack <gnoack3000@gmail.com> > --- > tools/testing/selftests/landlock/fs_test.c | 62 ++++++++++++++++++++++ > 1 file changed, 62 insertions(+) > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c > index fdd7d439ce4..1f827604374 100644 > --- a/tools/testing/selftests/landlock/fs_test.c > +++ b/tools/testing/selftests/landlock/fs_test.c > @@ -3655,6 +3655,68 @@ TEST(memfd_ftruncate) > ASSERT_EQ(0, close(fd)); > } > > +/* > + * Invokes ioctl(2) and returns its errno or 0. > + * The provided fd needs to be a tty for this to work. > + */ > +static int test_tty_ioctl(int fd) > +{ > + struct winsize ws; > + > + if (ioctl(fd, TIOCGWINSZ, &ws) < 0) > + return errno; > + return 0; > +} > + > +/* > + * Attempt ioctl on /dev/tty0 and /dev/tty1, > + * with file descriptors opened before and after landlocking. > + */ > +TEST_F_FORK(layout1, ioctl) > +{ > + const struct rule rules[] = { > + { > + .path = "/dev/tty1", > + .access = LANDLOCK_ACCESS_FS_IOCTL, > + }, > + /* Implicitly: No ioctl access on /dev/tty0. */ We should create a new PTS mount point, create a new session, and use that for tests to limit the dependency on the test environment and not mess with it. > + {}, > + }; > + const __u64 handled = LANDLOCK_ACCESS_FS_IOCTL; > + int ruleset_fd; > + int old_tty0_fd, tty0_fd, tty1_fd; > + > + old_tty0_fd = open("/dev/tty0", O_RDWR); > + ASSERT_LE(0, old_tty0_fd); > + > + /* Checks that ioctl works before landlocking. */ > + EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd)); > + > + /* Enable Landlock. */ Enable*s* > + ruleset_fd = create_ruleset(_metadata, handled, rules); > + ASSERT_LE(0, ruleset_fd); > + enforce_ruleset(_metadata, ruleset_fd); > + ASSERT_EQ(0, close(ruleset_fd)); > + > + /* Checks that ioctl with existing FD works after landlocking. */ > + EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd)); > + > + /* Checks that same ioctl fails when file is opened after landlocking. */ > + tty0_fd = open("/dev/tty0", O_RDWR); > + ASSERT_LE(0, tty0_fd); > + EXPECT_EQ(EACCES, test_tty_ioctl(tty0_fd)); > + > + /* Checks that same ioctl fails when file is opened after landlocking. */ > + tty1_fd = open("/dev/tty1", O_RDWR); > + ASSERT_LE(0, tty1_fd); > + EXPECT_EQ(0, test_tty_ioctl(tty1_fd)); /dev, or rather the test PTS mount point, and its parent, should also be tested. We can use three layers in the same test for that. > + > + /* Close all TTY file descriptors. */ > + ASSERT_EQ(0, close(old_tty0_fd)); > + ASSERT_EQ(0, close(tty0_fd)); > + ASSERT_EQ(0, close(tty1_fd)); > +} > + > /* clang-format off */ > FIXTURE(layout1_bind) {}; > /* clang-format on */
Hello! Thanks for the review! On Mon, Jun 19, 2023 at 04:42:17PM +0200, Mickaël Salaün wrote: > On 02/05/2023 19:17, Günther Noack wrote: > > Exercise the use of Landlock's ioctl restriction: If ioctl is > > restricted, the use of ioctl fails with a freshly opened /dev/tty > > file. > > > > Signed-off-by: Günther Noack <gnoack3000@gmail.com> > > --- > > tools/testing/selftests/landlock/fs_test.c | 62 ++++++++++++++++++++++ > > 1 file changed, 62 insertions(+) > > > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c > > index fdd7d439ce4..1f827604374 100644 > > --- a/tools/testing/selftests/landlock/fs_test.c > > +++ b/tools/testing/selftests/landlock/fs_test.c > > @@ -3655,6 +3655,68 @@ TEST(memfd_ftruncate) > > ASSERT_EQ(0, close(fd)); > > } > > +/* > > + * Invokes ioctl(2) and returns its errno or 0. > > + * The provided fd needs to be a tty for this to work. > > + */ > > +static int test_tty_ioctl(int fd) > > +{ > > + struct winsize ws; > > + > > + if (ioctl(fd, TIOCGWINSZ, &ws) < 0) > > + return errno; > > + return 0; > > +} > > + > > +/* > > + * Attempt ioctl on /dev/tty0 and /dev/tty1, > > + * with file descriptors opened before and after landlocking. > > + */ > > +TEST_F_FORK(layout1, ioctl) > > +{ > > + const struct rule rules[] = { > > + { > > + .path = "/dev/tty1", > > + .access = LANDLOCK_ACCESS_FS_IOCTL, > > + }, > > + /* Implicitly: No ioctl access on /dev/tty0. */ > > We should create a new PTS mount point, create a new session, and use that > for tests to limit the dependency on the test environment and not mess with > it. I have pondered this, and I feel that this is unnecessarily complicating the test. The mechanism that I intend to test here is just the general filtering of IOCTL commands, but not TTYs specifically. TTYs are a common use case for IOCTLs, but they are not the only one. If you are not strongly opposed to it, I would rather look for a different IOCTL command that works on a different file, where we don't need any special set up? That would simplify the test and exercise the same functionality in the end. Does that sounds reasonable? > > + {}, > > + }; > > + const __u64 handled = LANDLOCK_ACCESS_FS_IOCTL; > > + int ruleset_fd; > > + int old_tty0_fd, tty0_fd, tty1_fd; > > + > > + old_tty0_fd = open("/dev/tty0", O_RDWR); > > + ASSERT_LE(0, old_tty0_fd); > > + > > + /* Checks that ioctl works before landlocking. */ > > + EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd)); > > + > > + /* Enable Landlock. */ > > Enable*s* Done. > > + ruleset_fd = create_ruleset(_metadata, handled, rules); > > + ASSERT_LE(0, ruleset_fd); > > + enforce_ruleset(_metadata, ruleset_fd); > > + ASSERT_EQ(0, close(ruleset_fd)); > > + > > + /* Checks that ioctl with existing FD works after landlocking. */ > > + EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd)); > > + > > + /* Checks that same ioctl fails when file is opened after landlocking. */ > > + tty0_fd = open("/dev/tty0", O_RDWR); > > + ASSERT_LE(0, tty0_fd); > > + EXPECT_EQ(EACCES, test_tty_ioctl(tty0_fd)); > > + > > + /* Checks that same ioctl fails when file is opened after landlocking. */ > > + tty1_fd = open("/dev/tty1", O_RDWR); > > + ASSERT_LE(0, tty1_fd); > > + EXPECT_EQ(0, test_tty_ioctl(tty1_fd)); > > /dev, or rather the test PTS mount point, and its parent, should also be > tested. We can use three layers in the same test for that. We've already tested the inheritance of access rights across different directories and mount points in other tests. I feel that exercising it in all combinations of access rights and inheritance mechanisms makes the tests harder to understand and maintain, and does not give us much additional confidence on top of what we already have. What balance do you want to find there? Thanks, —Günther
On Mon, Aug 07, 2023 at 09:39:50AM +0200, Günther Noack wrote: > Hello! > > Thanks for the review! > > On Mon, Jun 19, 2023 at 04:42:17PM +0200, Mickaël Salaün wrote: > > On 02/05/2023 19:17, Günther Noack wrote: > > > Exercise the use of Landlock's ioctl restriction: If ioctl is > > > restricted, the use of ioctl fails with a freshly opened /dev/tty > > > file. > > > > > > Signed-off-by: Günther Noack <gnoack3000@gmail.com> > > > --- > > > tools/testing/selftests/landlock/fs_test.c | 62 ++++++++++++++++++++++ > > > 1 file changed, 62 insertions(+) > > > > > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c > > > index fdd7d439ce4..1f827604374 100644 > > > --- a/tools/testing/selftests/landlock/fs_test.c > > > +++ b/tools/testing/selftests/landlock/fs_test.c > > > @@ -3655,6 +3655,68 @@ TEST(memfd_ftruncate) > > > ASSERT_EQ(0, close(fd)); > > > } > > > +/* > > > + * Invokes ioctl(2) and returns its errno or 0. > > > + * The provided fd needs to be a tty for this to work. > > > + */ > > > +static int test_tty_ioctl(int fd) > > > +{ > > > + struct winsize ws; > > > + > > > + if (ioctl(fd, TIOCGWINSZ, &ws) < 0) > > > + return errno; > > > + return 0; > > > +} > > > + > > > +/* > > > + * Attempt ioctl on /dev/tty0 and /dev/tty1, > > > + * with file descriptors opened before and after landlocking. > > > + */ > > > +TEST_F_FORK(layout1, ioctl) > > > +{ > > > + const struct rule rules[] = { > > > + { > > > + .path = "/dev/tty1", > > > + .access = LANDLOCK_ACCESS_FS_IOCTL, > > > + }, > > > + /* Implicitly: No ioctl access on /dev/tty0. */ > > > > We should create a new PTS mount point, create a new session, and use that > > for tests to limit the dependency on the test environment and not mess with > > it. > > I have pondered this, and I feel that this is unnecessarily complicating the > test. The mechanism that I intend to test here is just the general filtering of > IOCTL commands, but not TTYs specifically. TTYs are a common use case for > IOCTLs, but they are not the only one. > > If you are not strongly opposed to it, I would rather look for a different IOCTL > command that works on a different file, where we don't need any special set up? > That would simplify the test and exercise the same functionality in the end. > Does that sounds reasonable? Yes, simpler is better. > > > > > + {}, > > > + }; > > > + const __u64 handled = LANDLOCK_ACCESS_FS_IOCTL; > > > + int ruleset_fd; > > > + int old_tty0_fd, tty0_fd, tty1_fd; > > > + > > > + old_tty0_fd = open("/dev/tty0", O_RDWR); > > > + ASSERT_LE(0, old_tty0_fd); > > > + > > > + /* Checks that ioctl works before landlocking. */ > > > + EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd)); > > > + > > > + /* Enable Landlock. */ > > > > Enable*s* > > Done. > > > > > + ruleset_fd = create_ruleset(_metadata, handled, rules); > > > + ASSERT_LE(0, ruleset_fd); > > > + enforce_ruleset(_metadata, ruleset_fd); > > > + ASSERT_EQ(0, close(ruleset_fd)); > > > + > > > + /* Checks that ioctl with existing FD works after landlocking. */ > > > + EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd)); > > > + > > > + /* Checks that same ioctl fails when file is opened after landlocking. */ > > > + tty0_fd = open("/dev/tty0", O_RDWR); > > > + ASSERT_LE(0, tty0_fd); > > > + EXPECT_EQ(EACCES, test_tty_ioctl(tty0_fd)); > > > + > > > + /* Checks that same ioctl fails when file is opened after landlocking. */ > > > + tty1_fd = open("/dev/tty1", O_RDWR); > > > + ASSERT_LE(0, tty1_fd); > > > + EXPECT_EQ(0, test_tty_ioctl(tty1_fd)); > > > > /dev, or rather the test PTS mount point, and its parent, should also be > > tested. We can use three layers in the same test for that. > > We've already tested the inheritance of access rights across different > directories and mount points in other tests. I feel that exercising it in all > combinations of access rights and inheritance mechanisms makes the tests harder > to understand and maintain, and does not give us much additional confidence on > top of what we already have. What balance do you want to find there? Indeed. It should be notted that this new IOCTL access right will be the first one to directly apply to both files and directories. It should then have the same scope as LANDLOCK_ACCESS_FS_READ i.e., apply to the target directory itself and files/directories beneath it. We then need to test a directory's IOCTL, for instance using FIOQSIZE. What about this two rules and related access checks, combined with already-opened FD? - dir_s1d1: always denied (negative test) - file1_s1d1: allowed with a rule (checks ACCESS_FILE) - dir_s2d1: allowed with a rule (checks directory right)
Hi! On Mon, Aug 07, 2023 at 11:41:48AM +0200, Mickaël Salaün wrote: > On Mon, Aug 07, 2023 at 09:39:50AM +0200, Günther Noack wrote: > > We've already tested the inheritance of access rights across different > > directories and mount points in other tests. I feel that exercising it in all > > combinations of access rights and inheritance mechanisms makes the tests harder > > to understand and maintain, and does not give us much additional confidence on > > top of what we already have. What balance do you want to find there? > > Indeed. It should be notted that this new IOCTL access right will be the > first one to directly apply to both files and directories. It should > then have the same scope as LANDLOCK_ACCESS_FS_READ i.e., apply to the > target directory itself and files/directories beneath it. > > We then need to test a directory's IOCTL, for instance using FIOQSIZE. > > What about this two rules and related access checks, combined with > already-opened FD? > - dir_s1d1: always denied (negative test) > - file1_s1d1: allowed with a rule (checks ACCESS_FILE) > - dir_s2d1: allowed with a rule (checks directory right) Ah, that's an excellent point - I had not realized yet that it is different to the other access rights in that way, and it makes a lot of sense to test that.
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index fdd7d439ce4..1f827604374 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -3655,6 +3655,68 @@ TEST(memfd_ftruncate) ASSERT_EQ(0, close(fd)); } +/* + * Invokes ioctl(2) and returns its errno or 0. + * The provided fd needs to be a tty for this to work. + */ +static int test_tty_ioctl(int fd) +{ + struct winsize ws; + + if (ioctl(fd, TIOCGWINSZ, &ws) < 0) + return errno; + return 0; +} + +/* + * Attempt ioctl on /dev/tty0 and /dev/tty1, + * with file descriptors opened before and after landlocking. + */ +TEST_F_FORK(layout1, ioctl) +{ + const struct rule rules[] = { + { + .path = "/dev/tty1", + .access = LANDLOCK_ACCESS_FS_IOCTL, + }, + /* Implicitly: No ioctl access on /dev/tty0. */ + {}, + }; + const __u64 handled = LANDLOCK_ACCESS_FS_IOCTL; + int ruleset_fd; + int old_tty0_fd, tty0_fd, tty1_fd; + + old_tty0_fd = open("/dev/tty0", O_RDWR); + ASSERT_LE(0, old_tty0_fd); + + /* Checks that ioctl works before landlocking. */ + EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd)); + + /* Enable Landlock. */ + ruleset_fd = create_ruleset(_metadata, handled, rules); + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + /* Checks that ioctl with existing FD works after landlocking. */ + EXPECT_EQ(0, test_tty_ioctl(old_tty0_fd)); + + /* Checks that same ioctl fails when file is opened after landlocking. */ + tty0_fd = open("/dev/tty0", O_RDWR); + ASSERT_LE(0, tty0_fd); + EXPECT_EQ(EACCES, test_tty_ioctl(tty0_fd)); + + /* Checks that same ioctl fails when file is opened after landlocking. */ + tty1_fd = open("/dev/tty1", O_RDWR); + ASSERT_LE(0, tty1_fd); + EXPECT_EQ(0, test_tty_ioctl(tty1_fd)); + + /* Close all TTY file descriptors. */ + ASSERT_EQ(0, close(old_tty0_fd)); + ASSERT_EQ(0, close(tty0_fd)); + ASSERT_EQ(0, close(tty1_fd)); +} + /* clang-format off */ FIXTURE(layout1_bind) {}; /* clang-format on */
Exercise the use of Landlock's ioctl restriction: If ioctl is restricted, the use of ioctl fails with a freshly opened /dev/tty file. Signed-off-by: Günther Noack <gnoack3000@gmail.com> --- tools/testing/selftests/landlock/fs_test.c | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+)