netdev - Re: [RFC PATCH v1 1/7] samples/landlock: Fix port parsing in sandboxer

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: <61ece1e6-50b0-55e5-985c-5db8090cf82b@huawei-partners.com>
Date: Mon, 23 Sep 2024 18:24:36 +0300
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: Matthieu Buffet <matthieu@...fet.re>, Mickaël Salaün
	<mic@...ikod.net>
CC: Günther Noack <gnoack@...gle.com>, Paul Moore
	<paul@...l-moore.com>, James Morris <jmorris@...ei.org>, "Serge E . Hallyn"
	<serge@...lyn.com>, <linux-security-module@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>, Konstantin
 Meskhidze <konstantin.meskhidze@...wei.com>, yusongping
	<yusongping@...wei.com>, <artem.kuzin@...wei.com>
Subject: Re: [RFC PATCH v1 1/7] samples/landlock: Fix port parsing in
 sandboxer

On 9/16/2024 3:22 PM, Matthieu Buffet wrote:
> Unlike LL_FS_RO and LL_FS_RW, LL_TCP_* are currently optional: either
> don't specify them and these access rights won't be in handled_accesses,
> or specify them and only the values passed are allowed.
> 
> If you want to specify that no port can be bind()ed, you would think
> (looking at the code quickly) that setting LL_TCP_BIND="" would do it.
> Due to a quirk in the parsing logic and the use of atoi() returning 0 with
> no error checking for empty strings, you end up allowing bind(0) (which
> means bind to any ephemeral port) without realising it. The same occurred
> when leaving a trailing/leading colon (e.g. "80:").
> 
> To reproduce:
> export LL_FS_RO="/" LL_FS_RW="" LL_TCP_BIND=""
> 
> ---8<----- Before this patch:
> ./sandboxer strace -e bind nc -n -vvv -l -p 0
> Executing the sandboxed command...
> bind(3, {sa_family=AF_INET, sin_port=htons(0),
>       sin_addr=inet_addr("0.0.0.0")}, 16) = 0
> Listening on 0.0.0.0 37629
> 
> ---8<----- Expected:
> ./sandboxer strace -e bind nc -n -vvv -l -p 0
> Executing the sandboxed command...
> bind(3, {sa_family=AF_INET, sin_port=htons(0),
>       sin_addr=inet_addr("0.0.0.0")}, 16) = -1 EACCES (Permission denied)
> nc: Permission denied
> 
> Signed-off-by: Matthieu Buffet <matthieu@...fet.re>
> ---
>   samples/landlock/sandboxer.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> index e8223c3e781a..a84ae3a15482 100644
> --- a/samples/landlock/sandboxer.c
> +++ b/samples/landlock/sandboxer.c
> @@ -168,7 +168,18 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
>   
>   	env_port_name_next = env_port_name;
>   	while ((strport = strsep(&env_port_name_next, ENV_DELIMITER))) {
> -		net_port.port = atoi(strport);
> +		char *strport_num_end = NULL;
> +
> +		if (strcmp(strport, "") == 0)
> +			continue;
> +
> +		errno = 0;
> +		net_port.port = strtol(strport, &strport_num_end, 0);
> +		if (errno != 0 || strport_num_end == strport) {
> +			fprintf(stderr,
> +				"Failed to parse port at \"%s\"\n", strport);
> +			goto out_free_name;
> +		}

Probably it'll be better to make a separate function for strtol
conversion (e.g. [1])? It might be needed for the socket type
control patchset [2].

[1] 
https://lore.kernel.org/all/20240904104824.1844082-18-ivanov.mikhail1@huawei-partners.com/
[2] 
https://lore.kernel.org/all/20240904104824.1844082-19-ivanov.mikhail1@huawei-partners.com/

>   		if (landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
>   				      &net_port, 0)) {
>   			fprintf(stderr,