[Spice-devel] [PATCH spice-server] inputs-channel: Check message size handling migration data

[Frediano Ziglio]

> 
> On Fri, Oct 06, 2017 at 11:58:33AM +0100, Frediano Ziglio wrote:
> > Prevent possible buffer reading overflow.
> > Note that message pointer must be valid and data are checked
> > value by value so even on overflow you just get an error.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/inputs-channel.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> > index 2de1c7c80..3d43e90ff 100644
> > --- a/server/inputs-channel.c
> > +++ b/server/inputs-channel.c
> > @@ -507,6 +507,11 @@ static bool
> > inputs_channel_handle_migrate_data(RedChannelClient *rcc,
> >      SpiceMigrateDataHeader *header;
> >      SpiceMigrateDataInputs *mig_data;
> >  
> > +    if (size < sizeof(SpiceMigrateDataHeader) +
> > sizeof(SpiceMigrateDataInputs)) {
> > +        spice_warning("bad message size %u", size);
> > +        return FALSE;
> > +    }
> > +
> 
> I would say this is a good candidate for using red_channel_warning()
> which I sent a series for ;)
> If the header is invalid, spice_error() is used, better to use
> spice_error() here too for consistency? (or the opposite).
> 
> 
> Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> 
> Christophe
> 

Actually I think here (in similar paths) spice_error is wrong, the messages
came from the client. Basically a client can cause server crash sending
invalid messages so causing migration to fail (VM should resume in source).
Not sure that having the client failing the migration is so a nice idea.
I think migration process is usually triggered by a more powerful (an
administrator) user, the fact that a normal user can cancel the job
looks wrong to me.

Frediano