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

[Mikhail Ivanov]

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 at buffet.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,