wild use of strlcpy() broke ipv6 support

[Matthieu Patou]

Hi Jeremy,

On 03/29/2012 11:49 AM, Jeremy Allison wrote:
> The branch, master has been updated
>         via  5df1c11 Start to add truncate checks on all uses of strlcpy(). Reading lwn has it's uses :-)
This patch  broke the internal DNS server if at least one interface has 
a local-link address (that is to say if enable IPv6).
Furthermore if bind interfaces only = yes and interfaces = xxx are 
specified in the configuration then all samba 4 services are broken.

>
> diff --git a/lib/util/util_net.c b/lib/util/util_net.c
> index 637c52b..69e5324 100644
> --- a/lib/util/util_net.c
> +++ b/lib/util/util_net.c
> @@ -107,9 +107,11 @@ static bool interpret_string_addr_pref(struct sockaddr_storage *pss,
>   		 */
>
>   		if (p&&  (p>  str)&&  ((scope_id = if_nametoindex(p+1)) != 0)) {
> -			strlcpy(addr, str,
> -				MIN(PTR_DIFF(p,str)+1,
> -					sizeof(addr)));
> +			size_t len = MIN(PTR_DIFF(p,str)+1, sizeof(addr));
> +			if (strlcpy(addr, str, len)>= len) {
> +				/* Truncate. */
> +				return false;
> +			}
>   			str = addr;
>   		}
>   	}
> @@ -332,9 +334,11 @@ bool is_ipaddress_v6(const char *str)
>   		 */
>
>   		if (p&&  (p>  str)&&  (if_nametoindex(p+1) != 0)) {
> -			strlcpy(addr, str,
> -				MIN(PTR_DIFF(p,str)+1,
> -					sizeof(addr)));
> +			size_t len = MIN(PTR_DIFF(p,str)+1, sizeof(addr));
> +			if (strlcpy(addr, str, len)>= len) {
> +				/* Truncate. */
> +				return false;
> +			}
>   			sp = addr;
>   		}
>   		ret = inet_pton(AF_INET6, sp,&dest6);
At least for this two changes you didn't get completely the sense of the 
strlcpy(), the idea is that if you have fe80::221:ccff:fe5f:7e51%eth0 to 
get the number of the interface and remove what is after '%'.
so we should in this case always truncate and ihmo it's not a problem.
> @@ -723,7 +727,10 @@ static const char *get_socket_addr(int fd, char *addr_buf, size_t addr_len)
>   	 * zero IPv6 address. No good choice here.
>   	 */
>
> -	strlcpy(addr_buf, "0.0.0.0", addr_len);
> +	if (strlcpy(addr_buf, "0.0.0.0", addr_len)>= addr_len) {
> +		/* Truncate ! */
> +		return NULL;
> +	}
>   
Here it seems also wrong if you have an address like 1.2.3.4 as addr_len 
will be the same as strlen("0.0.0.0"), didn't had a closer look.

I just focused on the IPv6 issues, maybe we should revert your patch 
while you had a closer look on the different use cases for strlcpy and 
then push a valid change ?

Matthieu.


-- 
Matthieu Patou
Samba Team
http://samba.org