netdev - Re: [RFC PATCH v3 17/19] samples/landlock: Replace atoi() with strtoull() in populate_ruleset

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: <bc7afd55-f560-7439-2806-a1f9e73307a0@huawei-partners.com>
Date: Thu, 3 Oct 2024 15:59:42 +0300
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: Günther Noack <gnoack@...gle.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>,
	<konstantin.meskhidze@...wei.com>
Subject: Re: [RFC PATCH v3 17/19] samples/landlock: Replace atoi() with
 strtoull() in populate_ruleset_net()

On 9/27/2024 6:12 PM, Günther Noack wrote:
> On Wed, Sep 04, 2024 at 06:48:22PM +0800, Mikhail Ivanov wrote:
>> Add str2num() helper and replace atoi() with it. atoi() does not provide
>> overflow checks, checks of invalid characters in a string and it is
>> recommended to use strtol-like functions (Cf. atoi() manpage).
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
>> ---
>>   samples/landlock/sandboxer.c | 27 ++++++++++++++++++++++++++-
>>   1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
>> index e8223c3e781a..d4dba9e4ce89 100644
>> --- a/samples/landlock/sandboxer.c
>> +++ b/samples/landlock/sandboxer.c
>> @@ -150,6 +150,26 @@ static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,
>>   	return ret;
>>   }
>>   
>> +static int str2num(const char *numstr, unsigned long long *num_dst)
>> +{
>> +	char *endptr = NULL;
>> +	int err = 1;
>> +	unsigned long long num;
>> +
>> +	errno = 0;
>> +	num = strtoull(numstr, &endptr, 0);
>> +	if (errno != 0)
>> +		goto out;
>> +
>> +	if (*endptr != '\0')
>> +		goto out;
>> +
>> +	*num_dst = num;
>> +	err = 0;
>> +out:
>> +	return err;
>> +}
> 
> I believe if numstr is the empty string, str2num would return success and set
> num_dst to 0, which looks unintentional to me.

Yeap.. I'll fix this

> 
> Do we not have a better helper for this that we can link from here?

I've checked how such convertion is performed in selftests by another
subsystems and it seems that most common practise is to implement static
helper or inline convertion in the needed place (e.g. safe_int() in
selftests/net/af_unix/scm_pidfd.c).

> 
>> +
>>   static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
>>   				const __u64 allowed_access)
>>   {
>> @@ -168,7 +188,12 @@ 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);
>> +		if (str2num(strport, &net_port.port)) {
>> +			fprintf(stderr,
>> +				"Failed to convert \"%s\" into a number\n",
>> +				strport);
>> +			goto out_free_name;
>> +		}
>>   		if (landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
>>   				      &net_port, 0)) {
>>   			fprintf(stderr,
>> -- 
>> 2.34.1
>>