Date: Mon, 23 Oct 2023 11:56:06 +0300
From: "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com>
To: Mickaël Salaün <mic@...ikod.net>
CC: <willemdebruijn.kernel@...il.com>, <gnoack3000@...il.com>,
	<linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>,
	<netfilter-devel@...r.kernel.org>, <yusongping@...wei.com>,
	<artem.kuzin@...wei.com>
Subject: Re: [PATCH v13 08/12] landlock: Add network rules and TCP hooks
 support



10/23/2023 11:30 AM, Mickaël Salaün пишет:
> On Mon, Oct 23, 2023 at 10:23:35AM +0300, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 10/20/2023 6:41 PM, Mickaël Salaün пишет:
>> > On Fri, Oct 20, 2023 at 02:58:31PM +0300, Konstantin Meskhidze (A) wrote:
>> > > 
>> > > 
>> > > 10/20/2023 12:49 PM, Mickaël Salaün пишет:
>> > > > On Fri, Oct 20, 2023 at 07:08:33AM +0300, Konstantin Meskhidze (A) wrote:
>> > > > > > > > > 10/18/2023 3:29 PM, Mickaël Salaün пишет:
>> > > > > > On Mon, Oct 16, 2023 at 09:50:26AM +0800, Konstantin Meskhidze wrote:
>> > 
>> > > > > > > diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> > > > > > > index 4c209acee01e..1fe4298ff4a7 100644
>> > > > > > > --- a/security/landlock/ruleset.c
>> > > > > > > +++ b/security/landlock/ruleset.c
>> > > > > > > @@ -36,6 +36,11 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>> > > > > > >  	refcount_set(&new_ruleset->usage, 1);
>> > > > > > >  	mutex_init(&new_ruleset->lock);
>> > > > > > >  	new_ruleset->root_inode = RB_ROOT;
>> > > > > > > +
>> > > > > > > +#if IS_ENABLED(CONFIG_INET)
>> > > > > > > +	new_ruleset->root_net_port = RB_ROOT;
>> > > > > > > +#endif /* IS_ENABLED(CONFIG_INET) */
>> > > > > > > +
>> > > > > > >  	new_ruleset->num_layers = num_layers;
>> > > > > > >  	/*
>> > > > > > >  	 * hierarchy = NULL
>> > > > > > > @@ -46,16 +51,21 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>> > > > > > >  }
>> > > > > > > > >  struct landlock_ruleset *
>> > > > > > > -landlock_create_ruleset(const access_mask_t fs_access_mask)
>> > > > > > > +landlock_create_ruleset(const access_mask_t fs_access_mask,
>> > > > > > > +			const access_mask_t net_access_mask)
>> > > > > > >  {
>> > > > > > >  	struct landlock_ruleset *new_ruleset;
>> > > > > > > > >  	/* Informs about useless ruleset. */
>> > > > > > > -	if (!fs_access_mask)
>> > > > > > > +	if (!fs_access_mask && !net_access_mask)
>> > > > > > >  		return ERR_PTR(-ENOMSG);
>> > > > > > >  	new_ruleset = create_ruleset(1);
>> > > > > > > -	if (!IS_ERR(new_ruleset))
>> > > > > > > +	if (IS_ERR(new_ruleset))
>> > > > > > > +		return new_ruleset;
>> > > > > > > +	if (fs_access_mask)
>> > > > > > >  		landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
>> > > > > > > +	if (net_access_mask)
>> > > > > > > +		landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
>> > > > > > > This is good, but it is not tested: we need to add a test that
>> > > > > both
>> > > > > > handle FS and net restrictions. You can add one in net.c, just handling
>> > > > > > LANDLOCK_ACCESS_FS_READ_DIR and LANDLOCK_ACCESS_NET_BIND_TCP, add one
>> > > > > > rule with path_beneath (e.g. /dev) and another with net_port, and check
>> > > > > > that open("/") is denied, open("/dev") is allowed, and and only the
>> > > > > > allowed port is allowed with bind(). This test should be simple and can
>> > > > > > only check against an IPv4 socket, i.e. using ipv4_tcp fixture, just
>> > > > > > after port_endianness. fcntl.h should then be included by net.c
>> > > > > > >   Ok.
>> > > > > > > I guess that was the purpose of layout1.with_net (in fs_test.c)
>> > > > > but it
>> > > > > > >   Yep. I added this kind of nest in fs_test.c to test both
>> > > fs and network
>> > > > > rules together.
>> > > > > > is not complete. You can revamp this test and move it to net.c
>> > > > > > following the above suggestions, keeping it consistent with other tests
>> > > > > > in net.c . You don't need the test_open() nor create_ruleset() helpers.
>> > > > > > > This test must failed if we change
>> > > > > "ruleset->access_masks[layer_level] |="
>> > > > > > to "ruleset->access_masks[layer_level] =" in
>> > > > > > landlock_add_fs_access_mask() or landlock_add_net_access_mask().
>> > > > > > >   Do you want to change it? Why?
>> > > > > The kernel code is correct and must not be changed. However, if
>> > > by
>> > > > mistake we change it and remove the OR, a test should catch that. We
>> > > > need a test to assert this assumption.
>> > > >   OK. I will add additional assert simulating
>> > > "ruleset->access_masks[layer_level] =" kernel code.
>> > > > >   Fs and network masks are ORed to not intersect with each other.
>> > > > > Yes, they are ORed, and we need a test to check that. Noting is
>> > > > currently testing this OR (and the different rule type consistency).
>> > > > I'm suggesting to revamp the layout1.with_net test into
>> > > > ipv4_tcp.with_fs and make it check ruleset->access_masks[] and rule
>> > > > addition of different types.
>> > > 
>> > >   I will move layout1.with_net test into net.c and rename it. Looks like
>> > >   it just needed to add "ruleset->access_masks[layer_level] =" assert
>> > >   because the test already has rule addition with different types.
>> > 
>> > The with_net test doesn't have FS rules, which is the main missing part.
>> > You'll need to rely on the net.c helpers, use the hardcoded paths, and
>> > only handle one access right of each type as I suggested above.
>> > 
>> 
>>  This is with_net code:
>> 
>>   ....
>>   /* Adds a network rule. */
>> 	
>> ASSERT_EQ(0, landlock_add_rule(ruleset_fd_net, LANDLOCK_RULE_NET_PORT,
>> 				       &tcp_bind, 0));
>> 
>> 	enforce_ruleset(_metadata, ruleset_fd_net);
>> 	ASSERT_EQ(0, close(ruleset_fd_net));
>> 
>> 	ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
>> 
>> 	ASSERT_LE(0, ruleset_fd);
>> 	enforce_ruleset(_metadata, ruleset_fd);
>> 	ASSERT_EQ(0, close(ruleset_fd));
>> ....
>> 
>> It has FS rules - just after ruleset_fd_net rule inforced.
>> Or maybe I missed something?
> 
> ruleset_fd_net and ruleset_fd are two different rulesets, and then
> they create two different layers. We need to test support for FS and net
> with the same ruleset/layer to check ruleset->access_masks[layer_level].
> 
   Got your point here. Thanks.
>> 
>> > > 
>> > >   Do you have any more review updates so far?
>> > 
>> > That's all for this patch series. :)
>> 
>>   Ok. Thanks.
>> > .
> .

Powered by blists - more mailing lists