Re: [Xen-devel] [Patch v2 3/4] tools/xenstored: Avoid buffer overflows while setting up sockets

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

Given the spirit today of missed pings on patches,

Third time lucky?

~Andrew

On 09/12/2013 13:32, Andrew Cooper wrote:
> Ping again?
>
> On 02/12/13 13:18, Andrew Cooper wrote:
>> Ping?  This v2 patch appears to have slipped through the cracks from my
>> set of Coverity fixes.
>>
>> ~Andrew
>>
>> On 25/11/13 14:38, Andrew Cooper wrote:
>>> Coverity ID: 1055996 1056002
>>>
>>> Cache the xs_daemon_socket{,_ro}() strings to save pointlessly
>>> re-snprintf()'ing the same path, and add explicit size checks against
>>> addr.sun_path before strcpy()'ing into it.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
>>> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
>>> CC: Matthew Daley <mattd@xxxxxxxxxxx>
>>>
>>> ---
>>> Changes in v2:
>>>  * Use logic similar to f220279c14
>>> ---
>>>  tools/xenstore/xenstored_core.c |   27 ++++++++++++++++++---------
>>>  1 file changed, 18 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/tools/xenstore/xenstored_core.c 
>>> b/tools/xenstore/xenstored_core.c
>>> index ccfdaa3..2324e53 100644
>>> --- a/tools/xenstore/xenstored_core.c
>>> +++ b/tools/xenstore/xenstored_core.c
>>> @@ -1718,6 +1718,9 @@ static void init_sockets(int **psock, int **pro_sock)
>>>  {
>>>     struct sockaddr_un addr;
>>>     int *sock, *ro_sock;
>>> +   const char *soc_str = xs_daemon_socket();
>>> +   const char *soc_str_ro = xs_daemon_socket_ro();
>>> +
>>>     /* Create sockets for them to listen to. */
>>>     *psock = sock = talloc(talloc_autofree_context(), int);
>>>     *sock = socket(PF_UNIX, SOCK_STREAM, 0);
>>> @@ -1731,19 +1734,25 @@ static void init_sockets(int **psock, int 
>>> **pro_sock)
>>>     talloc_set_destructor(ro_sock, destroy_fd);
>>>  
>>>     /* FIXME: Be more sophisticated, don't mug running daemon. */
>>> -   unlink(xs_daemon_socket());
>>> -   unlink(xs_daemon_socket_ro());
>>> +   unlink(soc_str);
>>> +   unlink(soc_str_ro);
>>>  
>>>     addr.sun_family = AF_UNIX;
>>> -   strcpy(addr.sun_path, xs_daemon_socket());
>>> +
>>> +   if(strlen(soc_str) >= sizeof(addr.sun_path))
>>> +           barf_perror("socket string '%s' too long", soc_str);
>>> +   strcpy(addr.sun_path, soc_str);
>>>     if (bind(*sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
>>> -           barf_perror("Could not bind socket to %s", xs_daemon_socket());
>>> -   strcpy(addr.sun_path, xs_daemon_socket_ro());
>>> +           barf_perror("Could not bind socket to %s", soc_str);
>>> +
>>> +   if(strlen(soc_str_ro) >= sizeof(addr.sun_path))
>>> +           barf_perror("socket string '%s' too long", soc_str_ro);
>>> +   strcpy(addr.sun_path, soc_str_ro);
>>>     if (bind(*ro_sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
>>> -           barf_perror("Could not bind socket to %s",
>>> -                       xs_daemon_socket_ro());
>>> -   if (chmod(xs_daemon_socket(), 0600) != 0
>>> -       || chmod(xs_daemon_socket_ro(), 0660) != 0)
>>> +           barf_perror("Could not bind socket to %s", soc_str_ro);
>>> +
>>> +   if (chmod(soc_str, 0600) != 0
>>> +       || chmod(soc_str_ro, 0660) != 0)
>>>             barf_perror("Could not chmod sockets");
>>>  
>>>     if (listen(*sock, 1) != 0


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel