mbox series

[RFC,v2,00/12] Socket type control for Landlock

Message ID 20240524093015.2402952-1-ivanov.mikhail1@huawei-partners.com (mailing list archive)
Headers show
Series Socket type control for Landlock | expand

Message

Mikhail Ivanov May 24, 2024, 9:30 a.m. UTC
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

Comments

Günther Noack June 4, 2024, 8:22 p.m. UTC | #1
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
Mikhail Ivanov June 6, 2024, 11:44 a.m. UTC | #2
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
>
Günther Noack June 6, 2024, 1:32 p.m. UTC | #3
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
Günther Noack June 6, 2024, 7:32 p.m. UTC | #4
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
Mikhail Ivanov June 7, 2024, 1:58 p.m. UTC | #5
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
Günther Noack June 10, 2024, 8:03 a.m. UTC | #6
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
Mikhail Ivanov June 11, 2024, 11:35 a.m. UTC | #7
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?