| Message ID | 20240524093015.2402952-1-ivanov.mikhail1@huawei-partners.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Socket type control for Landlock | expand |
On Fri, May 24, 2024 at 05:30:03PM +0800, Mikhail Ivanov wrote: > Hello! This is v2 RFC patch dedicated to socket protocols restriction. > > It is based on the landlock's mic-next branch on top of v6.9 kernel > version. Hello Mikhail! I patched in your patchset and tried to use the feature with a small demo tool, but I ran into what I think is a bug -- do you happen to know what this might be? I used 6.10-rc1 as a base and patched your patches on top. The code is a small tool called "nonet", which does the following: - Disable socket creation with a Landlock ruleset with the following attributes: struct landlock_ruleset_attr attr = { .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE, }; - open("/dev/null", O_WRONLY) Expected result: - open() should work Observed result: - open() fails with EACCES. I traced this with perf, and found that the open() gets rejected from Landlock's hook_file_open, whereas hook_socket_create does not get invoked. This is surprising to me -- Enabling a policy for socket creation should not influence the outcome of opening files! Tracing commands: sudo perf probe hook_socket_create '$params' sudo perf probe 'hook_file_open%return $retval' sudo perf record -e 'probe:*' -g -- ./nonet sudo perf report You can find the tool in my landlock-examples repo in the nonet_bug branch: https://github.com/gnoack/landlock-examples/blob/nonet_bug/nonet.c Landlock is enabled like this: https://github.com/gnoack/landlock-examples/blob/nonet_bug/sandbox_socket.c Do you have a hunch what might be going on? Thanks, –Günther
6/4/2024 11:22 PM, Günther Noack wrote: > On Fri, May 24, 2024 at 05:30:03PM +0800, Mikhail Ivanov wrote: >> Hello! This is v2 RFC patch dedicated to socket protocols restriction. >> >> It is based on the landlock's mic-next branch on top of v6.9 kernel >> version. > > Hello Mikhail! > > I patched in your patchset and tried to use the feature with a small > demo tool, but I ran into what I think is a bug -- do you happen to > know what this might be? > > I used 6.10-rc1 as a base and patched your patches on top. > > The code is a small tool called "nonet", which does the following: > > - Disable socket creation with a Landlock ruleset with the following > attributes: > > struct landlock_ruleset_attr attr = { > .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE, > }; > > - open("/dev/null", O_WRONLY) > > Expected result: > > - open() should work > > Observed result: > > - open() fails with EACCES. > > I traced this with perf, and found that the open() gets rejected from > Landlock's hook_file_open, whereas hook_socket_create does not get > invoked. This is surprising to me -- Enabling a policy for socket > creation should not influence the outcome of opening files! > > Tracing commands: > > sudo perf probe hook_socket_create '$params' > sudo perf probe 'hook_file_open%return $retval' > sudo perf record -e 'probe:*' -g -- ./nonet > sudo perf report > > You can find the tool in my landlock-examples repo in the nonet_bug branch: > https://github.com/gnoack/landlock-examples/blob/nonet_bug/nonet.c > > Landlock is enabled like this: > https://github.com/gnoack/landlock-examples/blob/nonet_bug/sandbox_socket.c > > Do you have a hunch what might be going on? Hello Günther! Big thanks for this research! I figured out that I define LANDLOCK_SHIFT_ACCESS_SOCKET macro in really strange way (see landlock/limits.h): #define LANDLOCK_SHIFT_ACCESS_SOCKET LANDLOCK_NUM_ACCESS_SOCKET With this definition, socket access mask overlaps the fs access mask in ruleset->access_masks[layer_level]. That's why landlock_get_fs_access_mask() returns non-zero mask in hook_file_open(). So, the macro must be defined in this way: #define LANDLOCK_SHIFT_ACCESS_SOCKET (LANDLOCK_NUM_ACCESS_NET + LANDLOCK_NUM_ACCESS_FS) With this fix, open() doesn't fail in your example. I'm really sorry that I somehow made such a stupid typo. I will try my best to make sure this doesn't happen again. > > Thanks, > –Günther >
Hello Mikhail! On Thu, Jun 06, 2024 at 02:44:23PM +0300, Mikhail Ivanov wrote: > 6/4/2024 11:22 PM, Günther Noack wrote: > > On Fri, May 24, 2024 at 05:30:03PM +0800, Mikhail Ivanov wrote: > > > Hello! This is v2 RFC patch dedicated to socket protocols restriction. > > > > > > It is based on the landlock's mic-next branch on top of v6.9 kernel > > > version. > > > > Hello Mikhail! > > > > I patched in your patchset and tried to use the feature with a small > > demo tool, but I ran into what I think is a bug -- do you happen to > > know what this might be? > > > > I used 6.10-rc1 as a base and patched your patches on top. > > > > The code is a small tool called "nonet", which does the following: > > > > - Disable socket creation with a Landlock ruleset with the following > > attributes: > > struct landlock_ruleset_attr attr = { > > .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE, > > }; > > > > - open("/dev/null", O_WRONLY) > > > > Expected result: > > > > - open() should work > > > > Observed result: > > > > - open() fails with EACCES. > > > > I traced this with perf, and found that the open() gets rejected from > > Landlock's hook_file_open, whereas hook_socket_create does not get > > invoked. This is surprising to me -- Enabling a policy for socket > > creation should not influence the outcome of opening files! > > > > Tracing commands: > > > > sudo perf probe hook_socket_create '$params' > > sudo perf probe 'hook_file_open%return $retval' > > sudo perf record -e 'probe:*' -g -- ./nonet > > sudo perf report > > You can find the tool in my landlock-examples repo in the nonet_bug branch: > > https://github.com/gnoack/landlock-examples/blob/nonet_bug/nonet.c > > > > Landlock is enabled like this: > > https://github.com/gnoack/landlock-examples/blob/nonet_bug/sandbox_socket.c > > > > Do you have a hunch what might be going on? > > Hello Günther! > Big thanks for this research! > > I figured out that I define LANDLOCK_SHIFT_ACCESS_SOCKET macro in > really strange way (see landlock/limits.h): > > #define LANDLOCK_SHIFT_ACCESS_SOCKET LANDLOCK_NUM_ACCESS_SOCKET > > With this definition, socket access mask overlaps the fs access > mask in ruleset->access_masks[layer_level]. That's why > landlock_get_fs_access_mask() returns non-zero mask in hook_file_open(). > > So, the macro must be defined in this way: > > #define LANDLOCK_SHIFT_ACCESS_SOCKET (LANDLOCK_NUM_ACCESS_NET + > LANDLOCK_NUM_ACCESS_FS) > > With this fix, open() doesn't fail in your example. > > I'm really sorry that I somehow made such a stupid typo. I will try my > best to make sure this doesn't happen again. Thanks for figuring it out so quickly. With that change, I'm getting some compilation errors (some bit shifts are becoming too wide for the underlying types), but I'm sure you can address that easily for the next version of the patch set. IMHO this shows that our reliance on bit manipulation is probably getting in the way of code clarity. :-/ I hope we can simplify these internal structures at some point. Once we have a better way to check for performance changes [1], we can try to change this and measure whether these comprehensibility/performance tradeoff is really worth it. [1] https://github.com/landlock-lsm/linux/issues/24 The other takeaway in my mind is, we should probably have some tests for that, to check that the enablement of one kind of policy does not affect the operations that belong to other kinds of policies. Like this, for instance (I was about to send this test to help debugging): TEST_F(mini, restricting_socket_does_not_affect_fs_actions) { const struct landlock_ruleset_attr ruleset_attr = { .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE, }; int ruleset_fd, fd; ruleset_fd = landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); ASSERT_LE(0, ruleset_fd); enforce_ruleset(_metadata, ruleset_fd); ASSERT_EQ(0, close(ruleset_fd)); /* * Accessing /dev/null for writing should be permitted, * because we did not add any file system restrictions. */ fd = open("/dev/null", O_WRONLY); EXPECT_LE(0, fd); ASSERT_EQ(0, close(fd)); } Since these kinds of tests are a bit at the intersection between the fs/net/socket tests, maybe they could go into a separate test file? The next time we add a new kind of Landlock restriction, it would come more naturally to add the matching test there and spot such issues earlier. Would you volunteer to add such a test as part of your patch set? :) Thanks, Günther
On Thu, Jun 06, 2024 at 03:32:47PM +0200, Günther Noack wrote: > Thanks for figuring it out so quickly. With that change, I'm getting some > compilation errors (some bit shifts are becoming too wide for the underlying > types), but I'm sure you can address that easily for the next version of the > patch set. Addendum, please ignore the remark about me getting compilation errors - I made a typo myself, and it worked in the way you suggested without warnings or errors. —Günther
6/6/2024 4:32 PM, Günther Noack wrote: > Hello Mikhail! > > On Thu, Jun 06, 2024 at 02:44:23PM +0300, Mikhail Ivanov wrote: >> 6/4/2024 11:22 PM, Günther Noack wrote: >>> On Fri, May 24, 2024 at 05:30:03PM +0800, Mikhail Ivanov wrote: >>>> Hello! This is v2 RFC patch dedicated to socket protocols restriction. >>>> >>>> It is based on the landlock's mic-next branch on top of v6.9 kernel >>>> version. >>> >>> Hello Mikhail! >>> >>> I patched in your patchset and tried to use the feature with a small >>> demo tool, but I ran into what I think is a bug -- do you happen to >>> know what this might be? >>> >>> I used 6.10-rc1 as a base and patched your patches on top. >>> >>> The code is a small tool called "nonet", which does the following: >>> >>> - Disable socket creation with a Landlock ruleset with the following >>> attributes: >>> struct landlock_ruleset_attr attr = { >>> .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE, >>> }; >>> >>> - open("/dev/null", O_WRONLY) >>> >>> Expected result: >>> >>> - open() should work >>> >>> Observed result: >>> >>> - open() fails with EACCES. >>> >>> I traced this with perf, and found that the open() gets rejected from >>> Landlock's hook_file_open, whereas hook_socket_create does not get >>> invoked. This is surprising to me -- Enabling a policy for socket >>> creation should not influence the outcome of opening files! >>> >>> Tracing commands: >>> >>> sudo perf probe hook_socket_create '$params' >>> sudo perf probe 'hook_file_open%return $retval' >>> sudo perf record -e 'probe:*' -g -- ./nonet >>> sudo perf report >>> You can find the tool in my landlock-examples repo in the nonet_bug branch: >>> https://github.com/gnoack/landlock-examples/blob/nonet_bug/nonet.c >>> >>> Landlock is enabled like this: >>> https://github.com/gnoack/landlock-examples/blob/nonet_bug/sandbox_socket.c >>> >>> Do you have a hunch what might be going on? >> >> Hello Günther! >> Big thanks for this research! >> >> I figured out that I define LANDLOCK_SHIFT_ACCESS_SOCKET macro in >> really strange way (see landlock/limits.h): >> >> #define LANDLOCK_SHIFT_ACCESS_SOCKET LANDLOCK_NUM_ACCESS_SOCKET >> >> With this definition, socket access mask overlaps the fs access >> mask in ruleset->access_masks[layer_level]. That's why >> landlock_get_fs_access_mask() returns non-zero mask in hook_file_open(). >> >> So, the macro must be defined in this way: >> >> #define LANDLOCK_SHIFT_ACCESS_SOCKET (LANDLOCK_NUM_ACCESS_NET + >> LANDLOCK_NUM_ACCESS_FS) >> >> With this fix, open() doesn't fail in your example. >> >> I'm really sorry that I somehow made such a stupid typo. I will try my >> best to make sure this doesn't happen again. > > Thanks for figuring it out so quickly. With that change, I'm getting some > compilation errors (some bit shifts are becoming too wide for the underlying > types), but I'm sure you can address that easily for the next version of the > patch set. > > IMHO this shows that our reliance on bit manipulation is probably getting in the > way of code clarity. :-/ I hope we can simplify these internal structures at > some point. Once we have a better way to check for performance changes [1], we > can try to change this and measure whether these comprehensibility/performance > tradeoff is really worth it. > > [1] https://github.com/landlock-lsm/linux/issues/24 Sounds great, probably this idea should be added to this issue [1]. [1] https://github.com/landlock-lsm/linux/issues/34 > > The other takeaway in my mind is, we should probably have some tests for that, > to check that the enablement of one kind of policy does not affect the > operations that belong to other kinds of policies. Like this, for instance (I > was about to send this test to help debugging): > > TEST_F(mini, restricting_socket_does_not_affect_fs_actions) > { > const struct landlock_ruleset_attr ruleset_attr = { > .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE, > }; > int ruleset_fd, fd; > > ruleset_fd = landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); > ASSERT_LE(0, ruleset_fd); > > enforce_ruleset(_metadata, ruleset_fd); > ASSERT_EQ(0, close(ruleset_fd)); > > /* > * Accessing /dev/null for writing should be permitted, > * because we did not add any file system restrictions. > */ > fd = open("/dev/null", O_WRONLY); > EXPECT_LE(0, fd); > > ASSERT_EQ(0, close(fd)); > } > > Since these kinds of tests are a bit at the intersection between the > fs/net/socket tests, maybe they could go into a separate test file? The next > time we add a new kind of Landlock restriction, it would come more naturally to > add the matching test there and spot such issues earlier. Would you volunteer > to add such a test as part of your patch set? :) Good idea! This test should probably be a part of the patch I mentioned here [1]. WDYT? (Btw, [1] should also be a part of the issue mentioned above). [1] https://lore.kernel.org/all/f4b5e2b9-e960-fd08-fdf4-328bb475e2ef@huawei-partners.com/ > > Thanks, > Günther
On Thu, Jun 06, 2024 at 02:44:23PM +0300, Mikhail Ivanov wrote: > 6/4/2024 11:22 PM, Günther Noack wrote: > I figured out that I define LANDLOCK_SHIFT_ACCESS_SOCKET macro in > really strange way (see landlock/limits.h): > > #define LANDLOCK_SHIFT_ACCESS_SOCKET LANDLOCK_NUM_ACCESS_SOCKET > > With this definition, socket access mask overlaps the fs access > mask in ruleset->access_masks[layer_level]. That's why > landlock_get_fs_access_mask() returns non-zero mask in hook_file_open(). > > So, the macro must be defined in this way: > > #define LANDLOCK_SHIFT_ACCESS_SOCKET (LANDLOCK_NUM_ACCESS_NET + > LANDLOCK_NUM_ACCESS_FS) > > With this fix, open() doesn't fail in your example. > > I'm really sorry that I somehow made such a stupid typo. I will try my > best to make sure this doesn't happen again. I found that we had the exact same bug with a wrongly defined "SHIFT" value in [1]. Maybe we should define access_masks_t as a bit-field rather than doing the bit-shifts by hand. Then the compiler would keep track of the bit-offsets automatically. Bit-fields have a bad reputation, but in my understanding, this is largely because they make it hard to control the exact bit-by-bit layout. In our case, we do not need such an exact control though, and it would be fine. To quote Linus Torvalds on [2], Bitfields are fine if you don't actually care about the underlying format, and want gcc to just randomly assign bits, and want things to be convenient in that situation. Let me send you a proposal patch which replaces access_masks_t with a bit-field and removes the need for the "SHIFT" definition, which we already got wrong in two patch sets now. It has the additional benefit of making the code a bit shorter and also removing a few static_assert()s which are now guaranteed by the compiler. —Günther [1] https://lore.kernel.org/all/ZmLEoBfHyUR3nKAV@google.com/ [2] https://yarchive.net/comp/linux/bitfields.html
6/10/2024 11:03 AM, Günther Noack wrote: > On Thu, Jun 06, 2024 at 02:44:23PM +0300, Mikhail Ivanov wrote: >> 6/4/2024 11:22 PM, Günther Noack wrote: >> I figured out that I define LANDLOCK_SHIFT_ACCESS_SOCKET macro in >> really strange way (see landlock/limits.h): >> >> #define LANDLOCK_SHIFT_ACCESS_SOCKET LANDLOCK_NUM_ACCESS_SOCKET >> >> With this definition, socket access mask overlaps the fs access >> mask in ruleset->access_masks[layer_level]. That's why >> landlock_get_fs_access_mask() returns non-zero mask in hook_file_open(). >> >> So, the macro must be defined in this way: >> >> #define LANDLOCK_SHIFT_ACCESS_SOCKET (LANDLOCK_NUM_ACCESS_NET + >> LANDLOCK_NUM_ACCESS_FS) >> >> With this fix, open() doesn't fail in your example. >> >> I'm really sorry that I somehow made such a stupid typo. I will try my >> best to make sure this doesn't happen again. > > I found that we had the exact same bug with a wrongly defined "SHIFT" value in > [1]. > > Maybe we should define access_masks_t as a bit-field rather than doing the > bit-shifts by hand. Then the compiler would keep track of the bit-offsets > automatically. > > Bit-fields have a bad reputation, but in my understanding, this is largely > because they make it hard to control the exact bit-by-bit layout. In our case, > we do not need such an exact control though, and it would be fine. > > To quote Linus Torvalds on [2], > > Bitfields are fine if you don't actually care about the underlying format, > and want gcc to just randomly assign bits, and want things to be > convenient in that situation. > > Let me send you a proposal patch which replaces access_masks_t with a bit-field > and removes the need for the "SHIFT" definition, which we already got wrong in > two patch sets now. It has the additional benefit of making the code a bit > shorter and also removing a few static_assert()s which are now guaranteed by the > compiler. > > —Günther > > [1] https://lore.kernel.org/all/ZmLEoBfHyUR3nKAV@google.com/ > [2] https://yarchive.net/comp/linux/bitfields.html Thank you, Günther! It really looks more clear. This patch should be applied to Landlock separately, right?
Hello! This is v2 RFC patch dedicated to socket protocols restriction. It is based on the landlock's mic-next branch on top of v6.9 kernel version. Description =========== Patchset implements new type of Landlock rule, that restricts socket protocols used in the sandboxed process. This restriction does not affect socket actions such as bind(2) or send(2), only those actions that result in a socket with unwanted protocol (e.g. creating socket with socket(2)). Such restriction would be useful to ensure that a sandboxed process uses only necessary protocols. For example sandboxed TCP server may want to permit only TCP sockets and deny any others. See [1] for more cases. The rules store information about the socket family and type. Thus, any protocol that can be defined by a family-type pair can be restricted by Landlock. struct landlock_socket_attr { __u64 allowed_access; int family; // same as domain in socket(2) int type; // see socket(2) } Patchset currently implements rule only for socket creation, but other necessary rules will also be impemented. [2] [1] https://lore.kernel.org/all/ZJvy2SViorgc+cZI@google.com/ [2] https://lore.kernel.org/all/b8a2045a-e7e8-d141-7c01-bf47874c7930@digikod.net/ Code coverage ============= Code coverage(gcov) report with the launch of all the landlock selftests: * security/landlock: lines......: 93.3% (795 of 852 lines) functions..: 95.5% (106 of 111 functions) * security/landlock/socket.c: lines......: 100.0% (33 of 33 lines) functions..: 100.0% (5 of 5 functions) General changes =============== * Rebases on mic-next (landlock-6.10-rc1). * Refactors code and commits. * Renames `family` into `domain` in landlock_socket_attr. * Changes ABI version from 5 to 6. * Reverts landlock_key.data type from u64 to uinptr_t. * Adds mini.socket_overflow, mini.socket_invalid_type tests. Previous versions ================= v1: https://lore.kernel.org/all/20240408093927.1759381-1-ivanov.mikhail1@huawei-partners.com/ Mikhail Ivanov (12): landlock: Support socket access-control landlock: Add hook on socket creation selftests/landlock: Add protocol.create to socket tests selftests/landlock: Add protocol.socket_access_rights to socket tests selftests/landlock: Add protocol.rule_with_unknown_access to socket tests selftests/landlock: Add protocol.rule_with_unhandled_access to socket tests selftests/landlock: Add protocol.inval to socket tests selftests/landlock: Add tcp_layers.ruleset_overlap to socket tests selftests/landlock: Add mini.ruleset_with_unknown_access to socket tests selftests/landlock: Add mini.socket_overflow to socket tests selftests/landlock: Add mini.socket_invalid_type to socket tests samples/landlock: Support socket protocol restrictions include/uapi/linux/landlock.h | 53 +- samples/landlock/sandboxer.c | 141 ++++- security/landlock/Makefile | 2 +- security/landlock/limits.h | 5 + security/landlock/ruleset.c | 37 +- security/landlock/ruleset.h | 41 +- security/landlock/setup.c | 2 + security/landlock/socket.c | 130 ++++ security/landlock/socket.h | 19 + security/landlock/syscalls.c | 66 +- tools/testing/selftests/landlock/base_test.c | 2 +- tools/testing/selftests/landlock/common.h | 1 + tools/testing/selftests/landlock/config | 1 + .../testing/selftests/landlock/socket_test.c | 581 ++++++++++++++++++ 14 files changed, 1056 insertions(+), 25 deletions(-) create mode 100644 security/landlock/socket.c create mode 100644 security/landlock/socket.h create mode 100644 tools/testing/selftests/landlock/socket_test.c