Re: [PATCH] avoid buffer overflow in sunrpc clnt_create (BZ #22542)

["Dmitry V. Levin" <ldv at altlinux dot org>]

On Wed, Feb 07, 2018 at 11:52:56AM -0800, Paul Eggert wrote:
[...]
> No, because that particular code isn't valid and should be fixed anyway, 
> regardless of whether one assumes sun_path uses strncpy format. The 
> problem is that the code mistakenly assumes that 'socket' always creates 
> a file whose name is given by av[1] and then goes off and unlinks av[1] 
> to make room for the new file, but this is incorrect when av[1]'s length 
> exceeds sizeof(addr.sun_path). That is, this code:
> 
>      assert(strlen(av[1]) > 0);
> 
>      strncpy(addr.sun_path, av[1], sizeof(addr.sun_path));
>      len = offsetof(struct sockaddr_un, sun_path) + strlen(av[1]) + 1;
>      if (len > sizeof(addr))
>          len = sizeof(addr);
> 
>      unlink(av[1]);
> 
> should be replaced by something like this code:
> 
>      assert(0 < strlen(av[1]) && strlen(av[1]) < sizeof(addr.sun_path));
> 
>      strcpy(addr.sun_path, av[1]);
>      len = offsetof(struct sockaddr_un, sun_path) + strlen(av[1]) + 1;
> 
>      unlink(av[1]);

The main purpose of this test is to check that strace decodes sun_path properly,
in particular, that the last byte of non-NUL-terminated sun_path is not
lost (it's a regression test for strace commit v4.9-248-gf57bd11), so strcpy
is not applicable here.  If strncpy starts generating a compilation error,
then the only available choice seems to be memcpy:

	len = strlen(av[1]);
	assert(len > 0 && len <= sizeof(addr.sun_path));

	if (++len > sizeof(addr.sun_path))
		len = sizeof(addr.sun_path);

	memcpy(addr.sun_path, av[1], len);
	len += offsetof(struct sockaddr_un, sun_path);

	unlink(av[1]);

As struct sockaddr_un.sun_path is not necessarily a C string, pretending
that it is a C string would encourage users to replace strncpy with
memcpy.


-- 
ldv

Attachment: signature.asc
Description: PGP signature