[Spice-devel] [PATCH spice-server 3/3] Dispatcher: validate received message types

[Jonathon Jongsma]

On Mon, 2017-09-04 at 06:25 -0400, Frediano Ziglio wrote:
> > 
> > Although dispatcher_send_message() does not allow you to send a
> > message
> > type that is invalid for a dispatcher, it still makes sense to be
> > defensive in the receiver. Validate that the message type that was
> > read
> > from the socket is in fact a valid message type for this
> > dispatcher.
> > ---
> >  server/dispatcher.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/server/dispatcher.c b/server/dispatcher.c
> > index 4e03ea046..965bae8e8 100644
> > --- a/server/dispatcher.c
> > +++ b/server/dispatcher.c
> > @@ -285,6 +285,10 @@ static int
> > dispatcher_handle_single_read(Dispatcher
> > *dispatcher)
> >          /* no messsage */
> >          return 0;
> >      }
> > +    if (type >= dispatcher->priv->max_message_type) {
> > +        spice_printerr("Invalid message type for this dispatcher:
> > %u",
> > type);
> > +        return 0;
> > +    }
> >      msg = &dispatcher->priv->messages[type];
> >      if (read_safe(dispatcher->priv->recv_fd, payload, msg->size,
> > 1) == -1) {
> >          spice_printerr("error reading from dispatcher: %d",
> > errno);
> 
> This patch looks like not really defensive. The events where this can
> occurs
> in my mind are:
> - physical damage (memory/cpu);
> - memory corruption;
> - file descriptor messing.
> All quite critical. You print an error and return leaving the state
> inconsistent potentially opening the door at any breach.
> I would use a spice_error, process should die.

Yeah, I thought about that. You make a good point. I'll change it.


> 
> On the paranoia level I would use an UNLIKELY macro to hint the
> compiler.
> 
> Frediano