linux-kernel - Re: [PATCH v2] hv/hv_kvp_daemon: Handle IPv4 and Ipv6 combination for keyfile format

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]

Date: Fri, 15 Mar 2024 09:56:22 -0700
From: Easwar Hariharan <eahariha@...ux.microsoft.com>
To: Shradha Gupta <shradhagupta@...ux.microsoft.com>
Cc: linux-kernel@...r.kernel.org, linux-hyperv@...r.kernel.org,
 "K. Y. Srinivasan" <kys@...rosoft.com>,
 Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu <wei.liu@...nel.org>,
 Dexuan Cui <decui@...rosoft.com>, Long Li <longli@...rosoft.com>,
 Michael Kelley <mikelley@...rosoft.com>, Olaf Hering <olaf@...fle.de>,
 Ani Sinha <anisinha@...hat.com>, Shradha Gupta <shradhagupta@...rosoft.com>
Subject: Re: [PATCH v2] hv/hv_kvp_daemon: Handle IPv4 and Ipv6 combination for
 keyfile format

On 3/15/2024 7:09 AM, Shradha Gupta wrote:

<snip>

>>>>  }
>>>>  
>>>> +static int process_dns_gateway_nm(FILE *f, char *ip_string, int type,
>>>> +				  int ip_sec)
>>>> +{
>>>> +	char addr[INET6_ADDRSTRLEN], *output_str;
>>>> +	int ip_offset = 0, error = 0, ip_ver;
>>>> +	char *param_name;
>>>> +
>>>> +	output_str = (char *)calloc(INET6_ADDRSTRLEN * MAX_IP_ENTRIES,
>>>> +				    sizeof(char));
>>>> +
>>>> +	if (!output_str)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	memset(addr, 0, sizeof(addr));
>>>> +
>>>> +	if (type == DNS) {
>>>> +		param_name = "dns";
>>>> +	} else if (type == GATEWAY) {
>>>> +		param_name = "gateway";
>>>> +	} else {
>>>> +		error = -EINVAL;
>>>> +		goto cleanup;
>>>> +	}
>>>> +
>>>> +	while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
>>>> +				   (MAX_IP_ADDR_SIZE * 2))) {
>>>> +		ip_ver = ip_version_check(addr);
>>>> +		if (ip_ver < 0)
>>>> +			continue;
>>>> +
>>>> +		if ((ip_ver == IPV4 && ip_sec == IPV4) ||
>>>> +		    (ip_ver == IPV6 && ip_sec == IPV6)) {
>>>> +			if (((INET6_ADDRSTRLEN * MAX_IP_ENTRIES) - strlen(output_str)) >
>>>> +			    (strlen(addr))) {
>>>> +				strcat(output_str, addr);
>>>> +				strcat(output_str, ",");
>>>
>>> Prefer strncat() here
> Is this needed with the bound check above. I am trying to keep parity with the rest of the 
> file.
>>>
<snip>

I missed this earlier because my comment was more of a general best practice comment.

Note that in the worst case where your bounds check (INET6_ADDRSTRLEN*MAX_IP_ENTRIES) - strlen(output_str) equals (strlen(addr) + 1),
you will be adding strlen(addr)+1(","), and end up with no ASCII NUL '\0' delimiter.

If you're going to rely on the bounds check to ensure correctness, you'll need to correct that. Generally speaking, strncat would still
be helpful in case the bounds check changes in the future.

Thanks,
Easwar