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

[Günther Noack]

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