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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <ZH9OFyWZ1njI7VG9@google.com>
Date: Tue, 6 Jun 2023 17:17:43 +0200
From: "Günther Noack" <gnoack@...gle.com>
To: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
Cc: mic@...ikod.net, willemdebruijn.kernel@...il.com, gnoack3000@...il.com, 
	linux-security-module@...r.kernel.org, netdev@...r.kernel.org, 
	netfilter-devel@...r.kernel.org, yusongping@...wei.com, 
	artem.kuzin@...wei.com
Subject: Re: [PATCH v11 11/12] samples/landlock: Add network demo

Hi Konstantin!

Apologies if some of this was discussed before, in this case,
Mickaël's review overrules my opinions from the sidelines ;)

On Tue, May 16, 2023 at 12:13:38AM +0800, Konstantin Meskhidze wrote:
> This commit adds network demo. It's possible to allow a sandboxer to
> bind/connect to a list of particular ports restricting network
> actions to the rest of ports.
> 
> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>


> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> index e2056c8b902c..b0250edb6ccb 100644
> --- a/samples/landlock/sandboxer.c
> +++ b/samples/landlock/sandboxer.c

...

> +static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
> +				const __u64 allowed_access)
> +{
> +	int num_ports, i, ret = 1;

I thought the convention was normally to set ret = 0 initially and to
override it in case of error, rather than the other way around?

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

-- 
Sent using Mutt 🐕 Woof Woof