avoid buffer overflow in sunrpc clnt_create (BZ #22542)

Annotating sockaddr_un::sun_path with attribute nonstring as
suggested by Carlos in [1] triggers a warning for call to strlen()
with the array as an argument in the clntunix_create() function.

The only caller of the function in Glibc, clnt_create(), calls
strcpy() to copy the string passed to it as the hostname argument
to the sun_path array member of a local struct sockadd_un object.
This causes a buffer overflow when the string is longer than
the size of the sun_path member array.

The attached patch tries to fix both of these problems.

It annotates the sun_path array member of struct sockaddr_un
with attribute nonstring.

It then changes clntunix_create() to use strnlen() instead of
strlen() to read as most as many characters from the sun_path
array as it has elements.

Finally, the patch changes the clnt_create() function to reject
hostnames longer than sizeof (sun_path) to avoid the overflow.

I considered changing clnt_create() to dynamically allocate
a larger object than sizeof (struct sockaddr_un) to fit the
whole hostname but decided to keep it simple.  Longer names
aren't supported now (they cause a buffer overflow) and since
no one complained, it seems good enough.  However, if it is
considered important to handle longer hostnames, it can be
changed.

Given the approach I chose in clnt_create(), using strnlen()
in clntunix_create() isn't really necessary to avoid reading
past the end of sun_path because it's nul-terminated, but it
is necessary to avoid the warning.

If it's decided that clnt_create() should handle overlong
hostnames then clntunix_create() will need to change back
to use either strlen(sun_path) and the warning will need
to be suppressed by some other means, or to use memchr().

Thanks
Martin

[1] https://sourceware.org/ml/libc-alpha/2017-11/msg00932.html

>  struct sockaddr_un
>    {
>      __SOCKADDR_COMMON (sun_);
> -    char sun_path[108];		/* Path name.  */
> +    char sun_path[108]
> +      __attribute_nonstring__;	/* Path name.  */
>    };

This says "sun_path uses strncpy format", but....

> +      if (strlen (hostname) >= sizeof sun.sun_path)
> +	{
> +	  struct rpc_createerr *ce = &get_rpc_createerr ();
> +	  ce->cf_stat = RPC_UNKNOWNHOST;
> +	  ce->cf_error.re_errno = EINVAL;
> +	  return NULL;
> +	}

... this says "sun_path uses ordinary string format", which isn't consistent.

I suggest that sun_path should use ordinary string format, since that's what 
people expect. In other words, do not add __attribute_nonstring__ or change 
clntunix_create, but instead just add the strlen check to clnt_create.

You might also consider using "__strnlen (hostname, sizeof sun.sun_path)" 
instead of "strlen (hostname)" to avoid bad asymptotic behavior if HOSTNAME is long.

On Sun, Dec 03, 2017 at 05:36:50PM -0800, Paul Eggert wrote:
[...]
> I suggest that sun_path should use ordinary string format, since that's what 
> people expect.

Do people really expect that?  Assuming that people are aware
of linux kernel behaviour, why would they expect that?

Dmitry V. Levin wrote:
> Do people really expect that?  Assuming that people are aware
> of linux kernel behaviour, why would they expect that?

These days, it's because strncpy format is obsolete and is not something 
programmers are ordinarily aware of. When in doubt (which there seems to be 
here), glibc should use null-terminated strings rather than strncpy format.

On Mon, Dec 04, 2017 at 12:04:12AM -0800, Paul Eggert wrote:
> Dmitry V. Levin wrote:
> > Do people really expect that?  Assuming that people are aware
> > of linux kernel behaviour, why would they expect that?
> 
> These days, it's because strncpy format is obsolete and is not something 
> programmers are ordinarily aware of. When in doubt (which there seems to be 
> here), glibc should use null-terminated strings rather than strncpy format.

Is there any statistics what programmers are ordinarily aware of?

I have no doubts that some valid code[1] no longer compiles with
-Werror=stringop-truncation, and the only plausible fix is to mark
struct sockaddr_un.sun_path with __attribute_nonstring__.

I think we should revisit the patch submitted by Martin.

[1] strace HEAD's tests no longer build in Fedora Rawhide with the following
diagnostics:
net-accept-connect.c: In function ‘main’:
net-accept-connect.c:57:2: error: ‘strncpy’ specified bound 108 equals destination size [-Werror=stringop-truncation]
  strncpy(addr.sun_path, av[1], sizeof(addr.sun_path));

On 02/06/2018 01:06 PM, Dmitry V. Levin wrote:
> On Mon, Dec 04, 2017 at 12:04:12AM -0800, Paul Eggert wrote:  >> Dmitry V. Levin wrote: >>> Do people really expect that? Assuming 
that people are aware >>> of linux kernel behaviour, why would they 
expect that? >> >> These days, it's because strncpy format is obsolete 
and is not something >> programmers are ordinarily aware of. When in 
doubt (which there seems to be >> here), glibc should use 
null-terminated strings rather than strncpy format. > > Is there any 
statistics what programmers are ordinarily aware of? > > I have no 
doubts that some valid code[1] no longer compiles with > 
-Werror=stringop-truncation, and the only plausible fix is to mark > 
struct sockaddr_un.sun_path with __attribute_nonstring__. If memory 
serves, a problem is that some glibc-related uses of struct 
sockaddr_un.sun_path assume strncpy format and others do not. If so, 
marking it with __attribute_nonstring__ will pacify uses in some cases 
(such as the one you mention), while masking bugs in others. If there's 
some doubt about the intended format of this buffer, then it's safer for 
everybody to stick to null-terminated strings in it.

For unusual programs such as the test program you mention, it's easy 
enough to use memcpy instead of strncpy to test buffers that are not 
null-terminated. Using memcpy would be better for this kind of test 
anyway, since it would allow a test case where the null byte at the end 
of the string is not followed by nulls all the way to the end of the buffer.

On Tue, Feb 06, 2018 at 04:54:45PM -0800, Paul Eggert wrote:
> On 02/06/2018 01:06 PM, Dmitry V. Levin wrote:
> > On Mon, Dec 04, 2017 at 12:04:12AM -0800, Paul Eggert wrote:  >> Dmitry V. Levin wrote: >>> Do people really expect that? Assuming 
> that people are aware >>> of linux kernel behaviour, why would they 
> expect that? >> >> These days, it's because strncpy format is obsolete 
> and is not something >> programmers are ordinarily aware of. When in 
> doubt (which there seems to be >> here), glibc should use 
> null-terminated strings rather than strncpy format. > > Is there any 
> statistics what programmers are ordinarily aware of? > > I have no 
> doubts that some valid code[1] no longer compiles with > 
> -Werror=stringop-truncation, and the only plausible fix is to mark > 
> struct sockaddr_un.sun_path with __attribute_nonstring__. If memory 
> serves, a problem is that some glibc-related uses of struct 
> sockaddr_un.sun_path assume strncpy format and others do not. If so, 
> marking it with __attribute_nonstring__ will pacify uses in some cases 
> (such as the one you mention), while masking bugs in others. If there's 
> some doubt about the intended format of this buffer, then it's safer for 
> everybody to stick to null-terminated strings in it.
> 
> For unusual programs such as the test program you mention, it's easy 
> enough to use memcpy instead of strncpy to test buffers that are not 
> null-terminated. Using memcpy would be better for this kind of test 
> anyway, since it would allow a test case where the null byte at the end 
> of the string is not followed by nulls all the way to the end of the buffer.

I don't see why a valid code has to be maimed to cater for broken code.
If anything, it's broken code that has to be changed, not otherwise.

I hope you don't really suggest people to apply changes like this
to their valid code:
-	strncpy(addr.sun_path, av[1], sizeof(addr.sun_path));
+	memcpy(addr.sun_path, av[1], strlen(av[1]) < sizeof(addr.sun_path) ? strlen(av[1]) + 1 : sizeof(addr.sun_path));

On 02/06/2018 02:06 PM, Dmitry V. Levin wrote:
> On Mon, Dec 04, 2017 at 12:04:12AM -0800, Paul Eggert wrote:
>> Dmitry V. Levin wrote:
>>> Do people really expect that?  Assuming that people are aware
>>> of linux kernel behaviour, why would they expect that?
>>
>> These days, it's because strncpy format is obsolete and is not something
>> programmers are ordinarily aware of. When in doubt (which there seems to be
>> here), glibc should use null-terminated strings rather than strncpy format.
>
> Is there any statistics what programmers are ordinarily aware of?
>
> I have no doubts that some valid code[1] no longer compiles with
> -Werror=stringop-truncation, and the only plausible fix is to mark
> struct sockaddr_un.sun_path with __attribute_nonstring__.
>
> I think we should revisit the patch submitted by Martin.
>
> [1] strace HEAD's tests no longer build in Fedora Rawhide with the following
> diagnostics:
> net-accept-connect.c: In function ‘main’:
> net-accept-connect.c:57:2: error: ‘strncpy’ specified bound 108 equals destination size [-Werror=stringop-truncation]
>   strncpy(addr.sun_path, av[1], sizeof(addr.sun_path));

I was going to follow up on the original thread but got stuck
trying to come up with a test case showing the kernel creating
a path with no terminating nul, and I've been too busy with
GCC work to get back to it.

I'm also worried that annotating the member nonstring will
lead to many more false positives for the canonical use case
of computing the path length/size using strlen even on input
(to the kernel/library) than true positives for the elusive
cases when there is no nul on output.  (Attribute nonstring
causes warnings when an array or pointer declared with is
passed to strlen or other functions that expect a nul-
terminated string.)

Martin

On 02/07/2018 02:53 AM, Dmitry V. Levin wrote:
> I don't see why a valid code has to be maimed to cater for broken code.
The problem here is that there is a dispute over what is "valid" and 
what is "broken". It's only a minor disagreement and shouldn't affect 
typical usage, but it is a disagreement. A good course here is to follow 
the usual advice about being strict about what you generate and generous 
about what you accept. We don't want to penalize code that is taking 
such an approach, in an attempt to slightly simplify code that is 
zealously enforcing one side of the dispute.

> I hope you don't really suggest people to apply changes like this  > to their valid code:
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]);

and once you do that, the problem goes away regardless of whether 
sun_path uses strncpy format.

In short, this particular test case should be changed to be strict about 
what it generates, and glibc should support this sort of usage instead 
of introducing artificial roadblocks against it.

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.

On Wed, Feb 07, 2018 at 08:57:46AM -0700, Martin Sebor wrote:
> On 02/06/2018 02:06 PM, Dmitry V. Levin wrote:
> > On Mon, Dec 04, 2017 at 12:04:12AM -0800, Paul Eggert wrote:
> >> Dmitry V. Levin wrote:
> >>> Do people really expect that?  Assuming that people are aware
> >>> of linux kernel behaviour, why would they expect that?
> >>
> >> These days, it's because strncpy format is obsolete and is not something
> >> programmers are ordinarily aware of. When in doubt (which there seems to be
> >> here), glibc should use null-terminated strings rather than strncpy format.
> >
> > Is there any statistics what programmers are ordinarily aware of?
> >
> > I have no doubts that some valid code[1] no longer compiles with
> > -Werror=stringop-truncation, and the only plausible fix is to mark
> > struct sockaddr_un.sun_path with __attribute_nonstring__.
> >
> > I think we should revisit the patch submitted by Martin.
> >
> > [1] strace HEAD's tests no longer build in Fedora Rawhide with the following
> > diagnostics:
> > net-accept-connect.c: In function ‘main’:
> > net-accept-connect.c:57:2: error: ‘strncpy’ specified bound 108 equals destination size [-Werror=stringop-truncation]
> >   strncpy(addr.sun_path, av[1], sizeof(addr.sun_path));
> 
> I was going to follow up on the original thread but got stuck
> trying to come up with a test case showing the kernel creating
> a path with no terminating nul, and I've been too busy with
> GCC work to get back to it.
> 
> I'm also worried that annotating the member nonstring will
> lead to many more false positives for the canonical use case
> of computing the path length/size using strlen even on input
> (to the kernel/library) than true positives for the elusive
> cases when there is no nul on output.  (Attribute nonstring
> causes warnings when an array or pointer declared with is
> passed to strlen or other functions that expect a nul-
> terminated string.)

struct sockaddr_un.sun_path is not a nul-terminated string,
one has to use strnlen instead of strlen.

On 02/07/2018 01:25 PM, Dmitry V. Levin wrote:
> If strncpy starts generating a compilation error,  > then the only available choice seems to be memcpy:
Yes, memcpy is typically the way to go here.

> 	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]);
Yes, that should also work and it'll fix the unlink bug that I 
mentioned. You might also want to replace the "if" statement with "len 
+= len < sizeof(addr.sun_path);", as that's simpler.

> 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.
There's nothing wrong with using memcpy for this test. On the contrary, 
memcpy improves the test by not unnecessarily initializing the part of 
addr.sun_path that doesn't need initializing. Programs like valgrind can 
use this information to catch bugs that the strncpy version would mask.

On 02/07/2018 02:31 PM, Dmitry V. Levin wrote:
> On Wed, Feb 07, 2018 at 08:57:46AM -0700, Martin Sebor wrote:
>> On 02/06/2018 02:06 PM, Dmitry V. Levin wrote:
>>> On Mon, Dec 04, 2017 at 12:04:12AM -0800, Paul Eggert wrote:
>>>> Dmitry V. Levin wrote:
>>>>> Do people really expect that?  Assuming that people are aware
>>>>> of linux kernel behaviour, why would they expect that?
>>>>
>>>> These days, it's because strncpy format is obsolete and is not something
>>>> programmers are ordinarily aware of. When in doubt (which there seems to be
>>>> here), glibc should use null-terminated strings rather than strncpy format.
>>>
>>> Is there any statistics what programmers are ordinarily aware of?
>>>
>>> I have no doubts that some valid code[1] no longer compiles with
>>> -Werror=stringop-truncation, and the only plausible fix is to mark
>>> struct sockaddr_un.sun_path with __attribute_nonstring__.
>>>
>>> I think we should revisit the patch submitted by Martin.
>>>
>>> [1] strace HEAD's tests no longer build in Fedora Rawhide with the following
>>> diagnostics:
>>> net-accept-connect.c: In function ‘main’:
>>> net-accept-connect.c:57:2: error: ‘strncpy’ specified bound 108 equals destination size [-Werror=stringop-truncation]
>>>   strncpy(addr.sun_path, av[1], sizeof(addr.sun_path));
>>
>> I was going to follow up on the original thread but got stuck
>> trying to come up with a test case showing the kernel creating
>> a path with no terminating nul, and I've been too busy with
>> GCC work to get back to it.
>>
>> I'm also worried that annotating the member nonstring will
>> lead to many more false positives for the canonical use case
>> of computing the path length/size using strlen even on input
>> (to the kernel/library) than true positives for the elusive
>> cases when there is no nul on output.  (Attribute nonstring
>> causes warnings when an array or pointer declared with is
>> passed to strlen or other functions that expect a nul-
>> terminated string.)
>
> struct sockaddr_un.sun_path is not a nul-terminated string,
> one has to use strnlen instead of strlen.

It may not be guaranteed to be nul-terminated, but it is nul
terminated most of the time (when the path is shorter than
the array), otherwise there wouldn't be so much working code
that depends on it and so much documentation with examples
that assume it is, including Glibc's own manual and the Linux
man page:
   http://man7.org/linux/man-pages/man7/unix.7.html

Once sun_path is declared nonstring all code that follows
these examples and in which GCC cannot prove the string is
nul-terminated will trigger the same warning.  The only
difference will be that the warning will point to sun_path
being passed to strlen or as the second argument to strcpy.

The question on my mind is: will the true positives/bugs
the warning finds outnumber the false positives by enough
of a margin that the warning will be viewed as useful.
To be able to even start to answer the question I'd need
to see an example where the sun_path isn't nul-terminated
and get a sense of how frequently that might happen in
practice.

Martin

* Paul Eggert:

>>  struct sockaddr_un
>>    {
>>      __SOCKADDR_COMMON (sun_);
>> -    char sun_path[108];		/* Path name.  */
>> +    char sun_path[108]
>> +      __attribute_nonstring__;	/* Path name.  */
>>    };
>
> This says "sun_path uses strncpy format", but....
>
>> +      if (strlen (hostname) >= sizeof sun.sun_path)
>> +	{
>> +	  struct rpc_createerr *ce = &get_rpc_createerr ();
>> +	  ce->cf_stat = RPC_UNKNOWNHOST;
>> +	  ce->cf_error.re_errno = EINVAL;
>> +	  return NULL;
>> +	}
>
> ... this says "sun_path uses ordinary string format", which isn't consistent.
>
> I suggest that sun_path should use ordinary string format, since
> that's what people expect. In other words, do not add
> __attribute_nonstring__ or change clntunix_create, but instead just
> add the strlen check to clnt_create.

There is a definition of SUN_LEN in <sys/un.h> that uses strlen, not
strnlen:

/* Evaluate to actual length of the `sockaddr_un' structure.  */
# define SUN_LEN(ptr) ((size_t) (((struct sockaddr_un *) 0)->sun_path)        \
                      + strlen ((ptr)->sun_path))

The kernel adds its own terminator, after the user-specified struct
length, so it should be fine with a pathname that is 108 bytes long.

Thanks,
Florian