Re: [Xen-devel] [PATCH 1/2] xenstore: sanity check incoming message body lengths

[Matthew Daley <mattd@xxxxxxxxxxx>]

On Tue, Dec 3, 2013 at 12:33 AM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:
> Matthew Daley writes ("[PATCH 1/2] xenstore: sanity check incoming message 
> body lengths"):
>> This is for the client-side receiving messages from xenstored, so there
>> is no security impact, unlike XSA-72.
> ...
>> +     /* Sanity check message body length. */
>> +     if (msg->hdr.len > XENSTORE_PAYLOAD_MAX) {
>> +             saved_errno = E2BIG;
>> +             goto error_freemsg;
>> +     }
>
> If this situation should arise, your proposal would discard the
> headers of the bogus message and read the start of what would be the
> over-long payload as the next header.
>
> Unfortunately, it looks like the existing code already does exactly
> this if it experiences a malloc failure !
>
> It would be best to either kill the connection dead, or perhaps to
> skip the overlong data.

The only callers of read_message I can see are read_reply,
read_watch_internal and read_thread. read_reply's only caller is
xs_talkv, which closes the connection on the failure being passed
down. read_watch_internal doesn't, and neither do its callers.
read_thread does close the connection.

So, with that said, where should the handling of the failure go? Would
it be good to consolidate the handling in one spot, ie. directly in
read_message? Since read_message is the root function for all these
other functions, and since read_thread already does handle the failure
and the other functions use that if implicitly if threaded background
reading is enabled, it seems like a good idea to do so.

- Matthew

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