Date: Tue, 4 Jul 2023 15:33:17 +0300
From: "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com>
To: Mickaël Salaün <mic@...ikod.net>,
	Günther Noack <gnoack@...gle.com>
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 v11 11/12] samples/landlock: Add network demo



7/3/2023 8:09 PM, Mickaël Salaün пишет:
> 
> On 03/07/2023 14:50, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 6/22/2023 1:18 PM, Mickaël Salaün пишет:
>>>
>>> On 22/06/2023 10:00, Konstantin Meskhidze (A) wrote:
>>>>
>>>>
>>>> 6/19/2023 9:19 PM, Mickaël Salaün пишет:
>>>>>
>>>>> On 19/06/2023 16:24, Konstantin Meskhidze (A) wrote:
>>>>>>
>>>>>>
>>>>>> 6/13/2023 11:38 PM, Mickaël Salaün пишет:
>>>>>>>
>>>>>>> On 13/06/2023 12:54, Konstantin Meskhidze (A) wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> 6/6/2023 6:17 PM, Günther Noack пишет:
>>>>>>>>> Hi Konstantin!
>>>>>>>>>
>>>>>>>>> Apologies if some of this was discussed before, in this case,
>>>>>>>>> Mickaël's review overrules my opinions from the sidelines ;)
>>>>>>>>>
>>>>>>>>> On Tue, May 16, 2023 at 12:13:38AM +0800, Konstantin Meskhidze wrote:
>>>>>>>>>> This commit adds network demo. It's possible to allow a sandboxer to
>>>>>>>>>> bind/connect to a list of particular ports restricting network
>>>>>>>>>> actions to the rest of ports.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
>>>>>>>>>> index e2056c8b902c..b0250edb6ccb 100644
>>>>>>>>>> --- a/samples/landlock/sandboxer.c
>>>>>>>>>> +++ b/samples/landlock/sandboxer.c
>>>>>>>>>
>>>>>>>>> ...
>>>>>>>>>
>>>>>>>>>> +static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
>>>>>>>>>> +				const __u64 allowed_access)
>>>>>>>>>> +{
>>>>>>>>>> +	int num_ports, i, ret = 1;
>>>>>>>>>
>>>>>>>>> I thought the convention was normally to set ret = 0 initially and to
>>>>>>>>> override it in case of error, rather than the other way around?
>>>>>>>
>>>>>>> Which convention? In this case, by default the return code is an error.
>>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>        Well, I just followed Mickaёl's way of logic here. >
>>>>>>>>
>>>>>>>>>> +	char *env_port_name;
>>>>>>>>>> +	struct landlock_net_service_attr net_service = {
>>>>>>>>>> +		.allowed_access = allowed_access,
>>>>>>>>>> +		.port = 0,
>>>>>>>>>> +	};
>>>>>>>>>> +
>>>>>>>>>> +	env_port_name = getenv(env_var);
>>>>>>>>>> +	if (!env_port_name)
>>>>>>>>>> +		return 0;
>>>>>>>>>> +	env_port_name = strdup(env_port_name);
>>>>>>>>>> +	unsetenv(env_var);
>>>>>>>>>> +	num_ports = parse_port_num(env_port_name);
>>>>>>>>>> +
>>>>>>>>>> +	if (num_ports == 1 && (strtok(env_port_name, ENV_PATH_TOKEN) == NULL)) {
>>>>>>>>>> +		ret = 0;
>>>>>>>>>> +		goto out_free_name;
>>>>>>>>>> +	}
>>>>>>>>>
>>>>>>>>> I don't understand why parse_port_num and strtok are necessary in this
>>>>>>>>> program. The man-page for strsep(3) describes it as a replacement to
>>>>>>>>> strtok(3) (in the HISTORY section), and it has a very short example
>>>>>>>>> for how it is used.
>>>>>>>>>
>>>>>>>>> Wouldn't it work like this as well?
>>>>>>>>>
>>>>>>>>> while ((strport = strsep(&env_port_name, ":"))) {
>>>>>>>>>        net_service.port = atoi(strport);
>>>>>>>>>        /* etc */
>>>>>>>>> }
>>>>>>>>
>>>>>>>>        Thanks for a tip. I think it's a better solution here. Now this
>>>>>>>> commit is in Mickaёl's -next branch. I could send a one-commit patch later.
>>>>>>>> Mickaёl, what do you think?
>>>>>>>
>>>>>>> I removed this series from -next because there is some issues (see the
>>>>>>> bot's emails), but anyway, this doesn't mean these patches don't need to
>>>>>>> be changed, they do. The goal of -next is to test more widely a patch
>>>>>>> series and get more feedbacks, especially from bots. When this series
>>>>>>> will be fully ready (and fuzzed with syzkaller), I'll push it to Linus
>>>>>>> Torvalds.
>>>>>>>
>>>>>>> I'll review the remaining tests and sample code this week, but you can
>>>>>>> still take into account the documentation review.
>>>>>>
>>>>>>      Hi, Mickaёl.
>>>>>>
>>>>>>      I have a few quetions?
>>>>>>       - Are you going to fix warnings for bots, meanwhile I run syzcaller?
>>>>>
>>>>> No, you need to fix that with the next series (except the Signed-off-by
>>>>> warnings).
>>>>
>>>>     Hi, Mickaёl.
>>>>      As I understand its possible to check bots warnings just after you
>>>> push the next V12 series again into your -next branch???
>>>
>>> Yes, we get bot warnings on the -next tree, but the command that
>>> generate it should be reproducible.
>> 
>>     Stephen Rothwell sent a few warnings he got with powerpc
>> pseries_le_defconfig. Do I need to fix it in V12 patch? How can I handle
>> it cause no warnings in current .config?
> 
> Yes, this need to be fixed in the next series. Could you point to the
> message?
> 
   Here you are please:
      1. 
https://lore.kernel.org/linux-next/20230607141044.1df56246@canb.auug.org.au 

      2. 
https://lore.kernel.org/linux-next/20230607135229.1f1e5c91@canb.auug.org.au/
      3. 
https://lore.kernel.org/linux-next/20230607124940.44af88bb@canb.auug.org.au/

> I'm almost done with the test, I revamped code and I'll send that tomorrow.
> 
   Ok.Thanks you. Please take your time. I will wait.
>>>
>>>
>>>>
>>>>>
>>>>> What is your status on syzkaller? Do you need some help? I can write the
>>>>> tests if it's too much.
>>>>>
>>>>      Sorry. To be honest I'm busy with another project. I dont know how
>>>> much time it will take for me to set up and run syzkaller. I need your
>>>> help here please, how you do this, some roadmap.
>>>
>>> Ok, no worries, I have it set up so I'll take care of it and keep you in
>>> the loop with your GitHub account.
>>>
>>    Thank you!!
>>>
>>>>>
>>>>>>       - I will fix documentation and sandbox demo and sent patch v12?
>>>>>
>>>>> Yes please. Let me a few days to send more reviews.
>>>>>
>>>>      Ok. Sure.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +	for (i = 0; i < num_ports; i++) {
>>>>>>>>>> +		net_service.port = atoi(strsep(&env_port_name, ENV_PATH_TOKEN));
>>>>>>>>>
>>>>>>>>> Naming of ENV_PATH_TOKEN:
>>>>>>>>> This usage is not related to paths, maybe rename the variable?
>>>>>>>>> It's also technically not the token, but the delimiter.
>>>>>>>>>
>>>>>>>>       What do you think of ENV_PORT_TOKEN or ENV_PORT_DELIMITER???
>>>>>>>
>>>>>>> You can rename ENV_PATH_TOKEN to ENV_DELIMITER for the FS and network parts.
>>>>>>>
>>>>>>        Ok. Got it.
>>>>>>>
>>>>>>>>
>>>>>>>>>> +		if (landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
>>>>>>>>>> +				      &net_service, 0)) {
>>>>>>>>>> +			fprintf(stderr,
>>>>>>>>>> +				"Failed to update the ruleset with port \"%lld\": %s\n",
>>>>>>>>>> +				net_service.port, strerror(errno));
>>>>>>>>>> +			goto out_free_name;
>>>>>>>>>> +		}
>>>>>>>>>> +	}
>>>>>>>>>> +	ret = 0;
>>>>>>>>>> +
>>>>>>>>>> +out_free_name:
>>>>>>>>>> +	free(env_port_name);
>>>>>>>>>> +	return ret;
>>>>>>>>>> +}
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>      		fprintf(stderr,
>>>>>>>>>>      			"Launch a command in a restricted environment.\n\n");
>>>>>>>>>> -		fprintf(stderr, "Environment variables containing paths, "
>>>>>>>>>> -				"each separated by a colon:\n");
>>>>>>>>>> +		fprintf(stderr,
>>>>>>>>>> +			"Environment variables containing paths and ports "
>>>>>>>>>> +			"each separated by a colon:\n");
>>>>>>>>>>      		fprintf(stderr,
>>>>>>>>>>      			"* %s: list of paths allowed to be used in a read-only way.\n",
>>>>>>>>>>      			ENV_FS_RO_NAME);
>>>>>>>>>>      		fprintf(stderr,
>>>>>>>>>> -			"* %s: list of paths allowed to be used in a read-write way.\n",
>>>>>>>>>> +			"* %s: list of paths allowed to be used in a read-write way.\n\n",
>>>>>>>>>>      			ENV_FS_RW_NAME);
>>>>>>>>>> +		fprintf(stderr,
>>>>>>>>>> +			"Environment variables containing ports are optional "
>>>>>>>>>> +			"and could be skipped.\n");
>>>>>>>>>
>>>>>>>>> As it is, I believe the program does something different when I'm
>>>>>>>>> setting these to the empty string (ENV_TCP_BIND_NAME=""), compared to
>>>>>>>>> when I'm unsetting them?
>>>>>>>>>
>>>>>>>>> I think the case where we want to forbid all handle-able networking is
>>>>>>>>> a legit and very common use case - it could be clearer in the
>>>>>>>>> documentation how this is done with the tool. (And maybe the interface
>>>>>>>>> could be something more explicit than setting the environment variable
>>>>>>>>> to empty?)
>>>>>>>
>>>>>>> I'd like to keep it simple, and it should be seen as an example code,
>>>>>>> not a full-feature sandboxer, but still a consistent and useful one.
>>>>>>> What would you suggest?
>>>>>>>
>>>>>>> This sandboxer tool relies on environment variables for its
>>>>>>> configuration. This is definitely not a good fit for all use cases, but
>>>>>>> I think it is simple and flexible enough. One use case might be to
>>>>>>> export a set of environment variables and simply call this tool. I'd
>>>>>>> prefer to not deal with argument parsing, but maybe that was too
>>>>>>> simplistic? We might want to revisit this approach but probably not with
>>>>>>> this series.
>>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +	/* Removes bind access attribute if not supported by a user. */
>>>>>>>>>> +	env_port_name = getenv(ENV_TCP_BIND_NAME);
>>>>>>>>>> +	if (!env_port_name) {
>>>>>>>>>> +		ruleset_attr.handled_access_net &=
>>>>>>>>>> +			~LANDLOCK_ACCESS_NET_BIND_TCP;
>>>>>>>>>> +	}
>>>>>>>>>> +	/* Removes connect access attribute if not supported by a user. */
>>>>>>>>>> +	env_port_name = getenv(ENV_TCP_CONNECT_NAME);
>>>>>>>>>> +	if (!env_port_name) {
>>>>>>>>>> +		ruleset_attr.handled_access_net &=
>>>>>>>>>> +			~LANDLOCK_ACCESS_NET_CONNECT_TCP;
>>>>>>>>>> +	}
>>>>>>>>>
>>>>>>>>> This is the code where the program does not restrict network usage,
>>>>>>>>> if the corresponding environment variable is not set.
>>>>>>>>
>>>>>>>>        Yep. Right.
>>>>>>>>>
>>>>>>>>> It's slightly inconsistent with what this tool does for filesystem
>>>>>>>>> paths. - If you don't specify any file paths, it will still restrict
>>>>>>>>> file operations there, independent of whether that env variable was
>>>>>>>>> set or not.  (Apologies if it was discussed before.)
>>>>>>>>
>>>>>>>>       Mickaёl wanted to make network ports optional here.
>>>>>>>>       Please check:
>>>>>>>>      
>>>>>>>> https://lore.kernel.org/linux-security-module/179ac2ee-37ff-92da-c381-c2c716725045@digikod.net/
>>>>>>>
>>>>>>> Right, the rationale is for compatibility with the previous version of
>>>>>>> this tool. We should not break compatibility when possible. A comment
>>>>>>> should explain the rationale though.
>>>>>>>
>>>>>>>>
>>>>>>>> https://lore.kernel.org/linux-security-module/fe3bc928-14f8-5e2b-359e-9a87d6cf5b01@digikod.net/
>>>>>>>>>
>>>>>>>>> —Günther
>>>>>>>>>
>>>>>>> .
>>>>> .
>>> .
> .

Powered by blists - more mailing lists