Re: [PATCH] cygwin: fix potential buffer overflow in fork

[Michael Haubenwallner <michael dot haubenwallner at ssi-schaefer dot com>]

On 10/10/2017 02:44 PM, Corinna Vinschen wrote:
> On Oct 10 14:26, Michael Haubenwallner wrote:
>> On 10/10/2017 01:48 PM, Corinna Vinschen wrote:
>>> Hi Michael,
>>>
>>> On Oct  9 18:58, Michael Haubenwallner wrote:
>>>> When fork fails, we can use "%s" now with system_sprintf for the errmsg
>>>> rather than a (potentially too small) buffer for the format string.
>>>
>>> How could buf be too small?
>>
>> See below.
>>
>> Actually I've found this by searching for suspect char array definitions
>> while hunting the "uninitialized variable for RtlLookupFunctionEntry" bug.
>>
>>>> * fork.cc (fork): Use "%s" with system_printf now.
>>>> ---
>>>>  winsup/cygwin/fork.cc | 9 ++-------
>>>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
>>>> index 73a72f530..bcbef12d8 100644
>>>> --- a/winsup/cygwin/fork.cc
>>>> +++ b/winsup/cygwin/fork.cc
>>>> @@ -618,13 +618,8 @@ fork ()
>>>>        if (!grouped.errmsg)
>>>>  	syscall_printf ("fork failed - child pid %d, errno %d", grouped.child_pid, grouped.this_errno);
>>>>        else
>>>> -	{
>>>> -	  char buf[strlen (grouped.errmsg) + sizeof ("child %d - , errno 4294967295  ")];
>>
>> Usually child_pid is longer than the 2 characters counted by "%d", but
>> errno usually is shorther than the 10 characters counted by "4294967295",
>> and there is another 2 reserved characters counted by trailing "  ".
>>
>> In practice the buffer unlikely will be too small, so this is merely cosmetics.
> 
> But buf is just the format string.  It won't get manipulated by
> system_printf.  Which means the 4294967295 is nonsense, too, a %d
> would have been sufficient.

Indeed! (out of coffee exception)

Thanks!
/haubi/