Re: [PATCH v11 11/12] samples/landlock: Add network demo

["Konstantin Meskhidze (A)" <konstantin.meskhidze@huawei.com>]

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@huawei.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?
   - I will fix documentation and sandbox demo and sent patch v12?

> 
> 
>> 
>>>
>>>> +
>>>> +	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
>>>
> .