Date: Tue, 13 Jun 2023 22:38:04 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com>,
 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


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.


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


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