Re: nonstrings in Glibc

[Carlos O'Donell <carlos at redhat dot com>]

On 11/27/2017 08:24 AM, Martin Sebor wrote:
> On 11/21/2017 05:14 PM, Dmitry V. Levin wrote:
>> On Tue, Nov 21, 2017 at 04:41:27PM -0700, Martin Sebor wrote:
>>> On 11/20/2017 11:20 AM, Carlos O'Donell wrote:
>>>> On 11/20/2017 08:54 AM, Martin Sebor wrote:
>>>>> I'm done testing my update to the -Wstringop-truncation GCC patch
>>>>> to find misuses of non-string arrays.  With the very limited use
>>>>> of attribute nonstring it only found one potential bug (22447).
>>>>> I've been looking at other uses of strncpy in Glibc to see if there
>>>>> are other arrays that would benefit from the attribute.  I'm not
>>>>> sufficiently familiar with Glibc data structures so it's a very
>>>>> slow going.  Could someone help suggests data structures with
>>>>> array members that might be candidates?
>>>>
>>>> struct sockaddr's sun_path?
>>>>
>>>> http://thread.gmane.org/gmane.comp.standards.posix.austin.general/5735
>>>>
>>>> Is that what you need help finding?
>>>
>>> Yes, that's what I'm looking for, thanks!
>>>
>>>  From the referenced thread it sounds like POSIX doesn't require
>>> sun_path to be nul-terminated and BSD UNIX doesn't terminate it.
>>> But I'm not sure what happens on Linux.  According to Michael
>>> Kerrisk's response it sounds like it is nul-terminated, but
>>> then according to the longer discussion on linux.kernel.api
>>> it sounds like it isn't.  Which is it?
>>
>> When struct sockaddr_un is passed to linux kernel, the kernel doesn't
>> treat sun_path as nul-terminated, see net/unix/af_unix.c:unix_mkname
>> for implementation details.
> 
> Thank you.  This is good to know (and expected).  The Glibc
> attribute is meant to help with the user side of things so
> it's your note below that's especially relevant.  (There
> are a few warnings in the Linux kernel suggesting it too
> might benefit from the new attribute, if only to suppress
> them.)
> 
>> However, when linux kernel returns struct sockaddr_un, sun_path is
>> nul-terminated if there is enough room provided by userspace, e.g.
>> it may need sizeof(struct sockaddr_un) + 1 bytes to write that NUL
>> beyond struct sockaddr_un.sun_path.
> 
> So if I understand correctly, sun_path will contain a nul-
> terminated string if there is enough room for the terminating
> nul but not otherwise.  If that's so, that would suggest that
> adding attribute nonstring to sum_path might be appropriate.
> At the same time, based on existing usage it seems that it
> would be prone to false positives.  Annotating sun_path with
> it turns up only one warning in a Glibc build:
> 
> clnt_unix.c:137:13: warning: ‘strlen’ argument 1 declared attribute ‘nonstring’ [-Wstringop-overflow=]
>        len = strlen (raddr->sun_path) + sizeof (raddr->sun_family) + 1;
> 
> AFAICS, the function is only called from clnt_create() where
> it's passed a nul-terminated string, so unless it can be called
> from user code with a non-nul-terminated array this would be
> an example of such a false positive.
> 
> Carlos, do you think the attribute would be helpful despite
> the false positives?
I think it would be helpful.

I consider it bad form to manipulate sun_path with strlen at *any*
time since the code might change along with expectations of where
the data came from, and that's how we end up with hard to find
exploits.

To be clear, I advocate for:

* Add the attribute + fix the glibc usage.

-- 
Cheers,
Carlos.