Re: [PATCH v3 1/3] samples/landlock: Fix port parsing in sandboxer

["Mickaël Salaün" <mic@digikod.net>]

Thanks! I pushed the three patches in my -next branch with minor
changes.

On Sat, Oct 19, 2024 at 05:15:32PM +0200, Matthieu Buffet wrote:
> If you want to specify that no port can be bind()ed, you would think
> (looking quickly at both help message and code) that setting LL_TCP_BIND=""
> would do it.
> 
> However the code splits on ":" then applies atoi(), which does not allow
> checking for errors. Passing an empty string returns 0, which is
> interpreted as "allow bind(0)", which means bind to any ephemeral port.
> This bug occurs whenever passing an empty string or when leaving a
> trailing/leading colon, making it impossible to completely deny bind().
> 
> To reproduce:
> export LL_FS_RO="/" LL_FS_RW="" LL_TCP_BIND=""
> ./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
> 
> Use strtoull(3) instead, which allows error checking. Check that the entire
> string has been parsed correctly without overflows/underflows, but not
> that the __u64 (the type of struct landlock_net_port_attr.port)
> is a valid __u16 port: that is already done by the kernel.
> 
> Fixes: 5e990dcef12e ("samples/landlock: Support TCP restrictions")
> Signed-off-by: Matthieu Buffet <matthieu@buffet.re>
> ---
>  samples/landlock/sandboxer.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
> index f847e832ba14..4cbef9d2f15b 100644
> --- a/samples/landlock/sandboxer.c
> +++ b/samples/landlock/sandboxer.c
> @@ -60,6 +60,25 @@ static inline int landlock_restrict_self(const int ruleset_fd,
>  #define ENV_SCOPED_NAME "LL_SCOPED"
>  #define ENV_DELIMITER ":"
>  
> +static int str2num(const char *numstr, __u64 *num_dst)
> +{
> +	char *endptr = NULL;
> +	int err = 0;
> +	__u64 num;
> +
> +	errno = 0;
> +	num = strtoull(numstr, &endptr, 10);
> +	if (errno != 0)
> +		err = errno;
> +	/* Was the string empty, or not entirely parsed successfully? */
> +	else if ((*numstr == '\0') || (*endptr != '\0'))

We cannot pass "0 " but we can still pass " 0".  I'm good with that
though.

> +		err = EINVAL;
> +	else
> +		*num_dst = num;
> +
> +	return err;
> +}
> +
>  static int parse_path(char *env_path, const char ***const path_list)
>  {
>  	int i, num_paths = 0;
> @@ -160,7 +179,6 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
>  	char *env_port_name, *env_port_name_next, *strport;
>  	struct landlock_net_port_attr net_port = {
>  		.allowed_access = allowed_access,
> -		.port = 0,
>  	};
>  
>  	env_port_name = getenv(env_var);
> @@ -171,7 +189,17 @@ 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);
> +		__u64 port;
> +
> +		if (strcmp(strport, "") == 0)
> +			continue;
> +
> +		if (str2num(strport, &port)) {
> +			fprintf(stderr, "Failed to parse port at \"%s\"\n",
> +				strport);
> +			goto out_free_name;
> +		}
> +		net_port.port = port;
>  		if (landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
>  				      &net_port, 0)) {
>  			fprintf(stderr,
> -- 
> 2.39.5
> 
>