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

Commit Message

Konstantin Meskhidze (A) May 15, 2023, 4:13 p.m. UTC

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 <[e-mail]>
---

Changes since v10:
* Refactors populate_ruleset_net() helper.
* Code style minor fix.

Changes since v9:
* Deletes ports converting.
* Minor fixes.

Changes since v8:
* Convert ports to __be16.
* Minor fixes.

Changes since v7:
* Removes network support if ABI < 4.
* Removes network support if not set by a user.

Changes since v6:
* Removes network support if ABI < 3.

Changes since v5:
* Makes network ports sandboxing optional.
* Fixes some logic errors.
* Formats code with clang-format-14.

Changes since v4:
* Adds ENV_TCP_BIND_NAME "LL_TCP_BIND" and
ENV_TCP_CONNECT_NAME "LL_TCP_CONNECT" variables
to insert TCP ports.
* Renames populate_ruleset() to populate_ruleset_fs().
* Adds populate_ruleset_net() and parse_port_num() helpers.
* Refactors main() to support network sandboxing.

---
 samples/landlock/sandboxer.c | 128 +++++++++++++++++++++++++++++++----
 1 file changed, 116 insertions(+), 12 deletions(-)

--
2.25.1

Comments

Günther Noack June 6, 2023, 3:17 p.m. UTC | #1

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 <[e-mail]>


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

> +	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 */
}

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

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


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

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

—Günther

Konstantin Meskhidze (A) June 13, 2023, 10:54 a.m. UTC | #2

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 <[e-mail]>
> 
> 
>> 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?
> 
   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?

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

>> +		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?)
> 
> 
>> +	/* 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/[e-mail]/

https://lore.kernel.org/linux-security-module/[e-mail]/
> 
> —Günther
>

Mickaël Salaün June 13, 2023, 8:38 p.m. UTC | #3

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 <[e-mail]>
>>
>>
>>> 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/[e-mail]/

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/[e-mail]/
>>
>> —Günther
>>

Konstantin Meskhidze (A) June 19, 2023, 2:24 p.m. UTC | #4

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 <[e-mail]>
>>>
>>>
>>>> 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/[e-mail]/
> 
> 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/[e-mail]/
>>>
>>> —Günther
>>>
> .

Mickaël Salaün June 19, 2023, 6:19 p.m. UTC | #5

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 <[e-mail]>
>>>>
>>>>
>>>>> 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).

What is your status on syzkaller? Do you need some help? I can write the 
tests if it's too much.


>     - I will fix documentation and sandbox demo and sent patch v12?

Yes please. Let me a few days to send more reviews.

> 
>>
>>
>>>
>>>>
>>>>> +
>>>>> +	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/[e-mail]/
>>
>> 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/[e-mail]/
>>>>
>>>> —Günther
>>>>
>> .

Konstantin Meskhidze (A) June 22, 2023, 8 a.m. UTC | #6

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 <[e-mail]>
>>>>>
>>>>>
>>>>>> 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???

> 
> 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.
> 
>>     - 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/[e-mail]/
>>>
>>> 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/[e-mail]/
>>>>>
>>>>> —Günther
>>>>>
>>> .
> .

Mickaël Salaün June 22, 2023, 10:18 a.m. UTC | #7

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 <[e-mail]>
>>>>>>
>>>>>>
>>>>>>> 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.


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


>>
>>>      - 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/[e-mail]/
>>>>
>>>> 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/[e-mail]/
>>>>>>
>>>>>> —Günther
>>>>>>
>>>> .
>> .

Konstantin Meskhidze (A) July 3, 2023, 12:50 p.m. UTC | #8

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 <[e-mail]>
>>>>>>>
>>>>>>>
>>>>>>>> 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?
> 
> 
>> 
>>>
>>> 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/[e-mail]/
>>>>>
>>>>> 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/[e-mail]/
>>>>>>>
>>>>>>> —Günther
>>>>>>>
>>>>> .
>>> .
> .

Mickaël Salaün July 3, 2023, 5:09 p.m. UTC | #9

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 <[e-mail]>
>>>>>>>>
>>>>>>>>
>>>>>>>>> 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?

I'm almost done with the test, I revamped code and I'll send that tomorrow.

>>
>>
>>>
>>>>
>>>> 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/[e-mail]/
>>>>>>
>>>>>> 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/[e-mail]/
>>>>>>>>
>>>>>>>> —Günther
>>>>>>>>
>>>>>> .
>>>> .
>> .

Konstantin Meskhidze (A) July 4, 2023, 12:33 p.m. UTC | #10

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 <[e-mail]>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> 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/[e-mail] 

      2. 
https://lore.kernel.org/linux-next/[e-mail]/
      3. 
https://lore.kernel.org/linux-next/[e-mail]/

> 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/[e-mail]/
>>>>>>>
>>>>>>> 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/[e-mail]/
>>>>>>>>>
>>>>>>>>> —Günther
>>>>>>>>>
>>>>>>> .
>>>>> .
>>> .
> .

Mickaël Salaün July 6, 2023, 2:35 p.m. UTC | #11

On 04/07/2023 14:33, Konstantin Meskhidze (A) wrote:
> 
> 
> 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 пишет:


[...]

>>>>>>>>>         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/[e-mail]

This issue is because the WARN_ON_ONCE() is triggered by any 
non-landlocked process, so removing the WARN_ON_ONCE() will fix that.


> 
>        2.
> https://lore.kernel.org/linux-next/[e-mail]/

Wrong printf format.


>        3.
> https://lore.kernel.org/linux-next/[e-mail]/

It looks like htmldocs doesn't like #if in enum definition. Anyway, I 
think it should be better to not conditionally define an enum. I've 
pushed this change here: https://git.kernel.org/mic/c/8c96c7eee3ff 
(landlock-net-v11 branch)


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

[...]

Konstantin Meskhidze (A) July 10, 2023, 12:26 p.m. UTC | #12

7/6/2023 5:35 PM, Mickaël Salaün пишет:
> 
> On 04/07/2023 14:33, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 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 пишет:
> 
> 
> [...]
> 
>>>>>>>>>>         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/[e-mail]
> 
> This issue is because the WARN_ON_ONCE() is triggered by any
> non-landlocked process, so removing the WARN_ON_ONCE() will fix that.
> 
   Got it. Will be fixed. Thanks.
> 
>> 
>>        2.
>> https://lore.kernel.org/linux-next/[e-mail]/
> 
> Wrong printf format.
> 
   Ok. I will fix it.
> 
>>        3.
>> https://lore.kernel.org/linux-next/[e-mail]/
> 
> It looks like htmldocs doesn't like #if in enum definition. Anyway, I
> think it should be better to not conditionally define an enum. I've
> pushed this change here: https://git.kernel.org/mic/c/8c96c7eee3ff
> (landlock-net-v11 branch)
> 
   Ok. Thank you.
> 
>> 
>>> 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.
> 
> [...]
> .

Patch

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
@@ -8,6 +8,7 @@ 
  */

 #define _GNU_SOURCE
+#include <arpa/inet.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <linux/landlock.h>
@@ -51,6 +52,8 @@  static inline int landlock_restrict_self(const int ruleset_fd,

 #define ENV_FS_RO_NAME "LL_FS_RO"
 #define ENV_FS_RW_NAME "LL_FS_RW"
+#define ENV_TCP_BIND_NAME "LL_TCP_BIND"
+#define ENV_TCP_CONNECT_NAME "LL_TCP_CONNECT"
 #define ENV_PATH_TOKEN ":"

 static int parse_path(char *env_path, const char ***const path_list)
@@ -71,6 +74,20 @@  static int parse_path(char *env_path, const char ***const path_list)
 	return num_paths;
 }

+static int parse_port_num(char *env_port)
+{
+	int i, num_ports = 0;
+
+	if (env_port) {
+		num_ports++;
+		for (i = 0; env_port[i]; i++) {
+			if (env_port[i] == ENV_PATH_TOKEN[0])
+				num_ports++;
+		}
+	}
+	return num_ports;
+}
+
 /* clang-format off */

 #define ACCESS_FILE ( \
@@ -81,8 +98,8 @@  static int parse_path(char *env_path, const char ***const path_list)

 /* clang-format on */

-static int populate_ruleset(const char *const env_var, const int ruleset_fd,
-			    const __u64 allowed_access)
+static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,
+			       const __u64 allowed_access)
 {
 	int num_paths, i, ret = 1;
 	char *env_path_name;
@@ -143,6 +160,45 @@  static int populate_ruleset(const char *const env_var, const int ruleset_fd,
 	return ret;
 }

+static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
+				const __u64 allowed_access)
+{
+	int num_ports, i, ret = 1;
+	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;
+	}
+
+	for (i = 0; i < num_ports; i++) {
+		net_service.port = atoi(strsep(&env_port_name, ENV_PATH_TOKEN));
+		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;
+}
+
 /* clang-format off */

 #define ACCESS_FS_ROUGHLY_READ ( \
@@ -166,39 +222,58 @@  static int populate_ruleset(const char *const env_var, const int ruleset_fd,

 /* clang-format on */

-#define LANDLOCK_ABI_LAST 3
+#define LANDLOCK_ABI_LAST 4

 int main(const int argc, char *const argv[], char *const *const envp)
 {
 	const char *cmd_path;
 	char *const *cmd_argv;
 	int ruleset_fd, abi;
+	char *env_port_name;
 	__u64 access_fs_ro = ACCESS_FS_ROUGHLY_READ,
 	      access_fs_rw = ACCESS_FS_ROUGHLY_READ | ACCESS_FS_ROUGHLY_WRITE;
+
 	struct landlock_ruleset_attr ruleset_attr = {
 		.handled_access_fs = access_fs_rw,
+		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
+				      LANDLOCK_ACCESS_NET_CONNECT_TCP,
 	};

 	if (argc < 2) {
 		fprintf(stderr,
-			"usage: %s=\"...\" %s=\"...\" %s <cmd> [args]...\n\n",
-			ENV_FS_RO_NAME, ENV_FS_RW_NAME, argv[0]);
+			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\"%s "
+			"<cmd> [args]...\n\n",
+			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
+			ENV_TCP_CONNECT_NAME, argv[0]);
 		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");
+		fprintf(stderr,
+			"* %s: list of ports allowed to bind (server).\n",
+			ENV_TCP_BIND_NAME);
+		fprintf(stderr,
+			"* %s: list of ports allowed to connect (client).\n",
+			ENV_TCP_CONNECT_NAME);
 		fprintf(stderr,
 			"\nexample:\n"
 			"%s=\"/bin:/lib:/usr:/proc:/etc:/dev/urandom\" "
 			"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
+			"%s=\"9418\" "
+			"%s=\"80:443\" "
 			"%s bash -i\n\n",
-			ENV_FS_RO_NAME, ENV_FS_RW_NAME, argv[0]);
+			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
+			ENV_TCP_CONNECT_NAME, argv[0]);
 		fprintf(stderr,
 			"This sandboxer can use Landlock features "
 			"up to ABI version %d.\n",
@@ -255,7 +330,12 @@  int main(const int argc, char *const argv[], char *const *const envp)
 	case 2:
 		/* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
 		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
-
+		__attribute__((fallthrough));
+	case 3:
+		/* Removes network support for ABI < 4 */
+		ruleset_attr.handled_access_net &=
+			~(LANDLOCK_ACCESS_NET_BIND_TCP |
+			  LANDLOCK_ACCESS_NET_CONNECT_TCP);
 		fprintf(stderr,
 			"Hint: You should update the running kernel "
 			"to leverage Landlock features "
@@ -274,18 +354,42 @@  int main(const int argc, char *const argv[], char *const *const envp)
 	access_fs_ro &= ruleset_attr.handled_access_fs;
 	access_fs_rw &= ruleset_attr.handled_access_fs;

+	/* 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;
+	}
+
 	ruleset_fd =
 		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
 	if (ruleset_fd < 0) {
 		perror("Failed to create a ruleset");
 		return 1;
 	}
-	if (populate_ruleset(ENV_FS_RO_NAME, ruleset_fd, access_fs_ro)) {
+
+	if (populate_ruleset_fs(ENV_FS_RO_NAME, ruleset_fd, access_fs_ro)) {
+		goto err_close_ruleset;
+	}
+	if (populate_ruleset_fs(ENV_FS_RW_NAME, ruleset_fd, access_fs_rw)) {
 		goto err_close_ruleset;
 	}
-	if (populate_ruleset(ENV_FS_RW_NAME, ruleset_fd, access_fs_rw)) {
+
+	if (populate_ruleset_net(ENV_TCP_BIND_NAME, ruleset_fd,
+				 LANDLOCK_ACCESS_NET_BIND_TCP)) {
 		goto err_close_ruleset;
 	}
+	if (populate_ruleset_net(ENV_TCP_CONNECT_NAME, ruleset_fd,
+				 LANDLOCK_ACCESS_NET_CONNECT_TCP)) {
+		goto err_close_ruleset;
+	}
+
 	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
 		perror("Failed to restrict privileges");
 		goto err_close_ruleset;