[Spice-devel] [vdagent-linux v5 1/3] vdagentd: early return on bad message size

[Frediano Ziglio]

> 
> From: Victor Toso <me at victortoso.com>
> 
> The payload size for each message should be the size of the expected
> struct or bigger when it contain arrays of no fixed size.
> 
> This patch creates the vdagent_message_min_size[] static array with
> the expected size for each message so we can check on
> virtio_port_read_complete().
> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/vdagentd/vdagentd.c | 56
>  ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 34 insertions(+), 22 deletions(-)
> 
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index a1faf23..4eb8e80 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -77,6 +77,25 @@ static int retval = 0;
>  static int client_connected = 0;
>  static int max_clipboard = -1;
>  
> +static gsize vdagent_message_min_size[VD_AGENT_END_MESSAGE] = {

+const

what happens if protocol add some messages so VD_AGENT_END_MESSAGE
get increased?
I think would be better to declare as

static const gsize vdagent_message_min_size[] = {

so all messages listed are supported

I would use something smaller that gsize but I'm just paranoid.

> +    -1, /* Does not exist */
> +    sizeof(VDAgentMouseState), /* VD_AGENT_MOUSE_STATE */
> +    sizeof(VDAgentMonitorsConfig), /* VD_AGENT_MONITORS_CONFIG */
> +    sizeof(VDAgentReply), /* VD_AGENT_REPLY */
> +    sizeof(VDAgentClipboard), /* VD_AGENT_CLIPBOARD */
> +    sizeof(VDAgentDisplayConfig), /* VD_AGENT_DISPLAY_CONFIG */
> +    sizeof(VDAgentAnnounceCapabilities), /* VD_AGENT_ANNOUNCE_CAPABILITIES
> */
> +    sizeof(VDAgentClipboardGrab), /* VD_AGENT_CLIPBOARD_GRAB */
> +    sizeof(VDAgentClipboardRequest), /* VD_AGENT_CLIPBOARD_REQUEST */
> +    sizeof(VDAgentClipboardRelease), /* VD_AGENT_CLIPBOARD_RELEASE */
> +    sizeof(VDAgentFileXferStartMessage), /* VD_AGENT_FILE_XFER_START */
> +    sizeof(VDAgentFileXferStatusMessage), /* VD_AGENT_FILE_XFER_STATUS */
> +    sizeof(VDAgentFileXferDataMessage), /* VD_AGENT_FILE_XFER_DATA */
> +    0, /* VD_AGENT_CLIENT_DISCONNECTED */
> +    sizeof(VDAgentMaxClipboard), /* VD_AGENT_MAX_CLIPBOARD */
> +    sizeof(VDAgentAudioVolumeSync) /* VD_AGENT_AUDIO_VOLUME_SYNC */
> +};
> +
>  /* utility functions */
>  /* vdagentd <-> spice-client communication handling */
>  static void send_capabilities(struct vdagent_virtio_port *vport,
> @@ -329,10 +348,20 @@ static int virtio_port_read_complete(
>          return 0;
>      }
>  
> +    if (message_header->type >= VD_AGENT_END_MESSAGE) {

Here G_N_ELEMENTS and you should check for 0 too.

> +        syslog(LOG_WARNING, "unknown message type %d, ignoring",
> +               message_header->type);
> +        return 0;
> +    }
> +
> +    if (message_header->size <
> vdagent_message_min_size[message_header->type]) {
> +        syslog(LOG_ERR, "read: invalid message size: %u for message type:
> %u",
> +               message_header->size, message_header->type);
> +        return 0;
> +    }
> +
>      switch (message_header->type) {
>      case VD_AGENT_MOUSE_STATE:
> -        if (message_header->size != sizeof(VDAgentMouseState))
> -            goto size_error;

what happens if message is longer? Previously an error, now I think
the rest of the message is ignored

>          vdagentd_uinput_do_mouse(&uinput, (VDAgentMouseState *)data);
>          if (!uinput) {
>              /* Try to re-open the tablet */
> @@ -354,14 +383,10 @@ static int virtio_port_read_complete(
>          }
>          break;
>      case VD_AGENT_MONITORS_CONFIG:
> -        if (message_header->size < sizeof(VDAgentMonitorsConfig))
> -            goto size_error;
>          do_client_monitors(vport, port_nr, message_header,
>                      (VDAgentMonitorsConfig *)data);
>          break;
>      case VD_AGENT_ANNOUNCE_CAPABILITIES:
> -        if (message_header->size < sizeof(VDAgentAnnounceCapabilities))
> -            goto size_error;
>          do_client_capabilities(vport, message_header,
>                          (VDAgentAnnounceCapabilities *)data);
>          break;
> @@ -381,9 +406,6 @@ static int virtio_port_read_complete(
>                                      VD_AGENT_CAP_CLIPBOARD_SELECTION)) {
>              min_size += 4;
>          }
> -        if (message_header->size < min_size) {
> -            goto size_error;
> -        }

This should not be removed, it checks for 4 bytes more than the standard.
You could also reuse the array you added to compute min_size in the
previous lines.

>          do_client_clipboard(vport, message_header, data);
>          break;
>      case VD_AGENT_FILE_XFER_START:
> @@ -395,31 +417,21 @@ static int virtio_port_read_complete(
>          vdagent_virtio_port_reset(vport, VDP_CLIENT_PORT);
>          do_client_disconnect();
>          break;
> -    case VD_AGENT_MAX_CLIPBOARD:
> -        if (message_header->size != sizeof(VDAgentMaxClipboard))
> -            goto size_error;

Same as VD_AGENT_MOUSE_STATE

> +    case VD_AGENT_MAX_CLIPBOARD: {
>          VDAgentMaxClipboard *msg = (VDAgentMaxClipboard *)data;
>          syslog(LOG_DEBUG, "Set max clipboard: %d", msg->max);
>          max_clipboard = msg->max;
>          break;
> +    }

Why the brackets are not necessary?
Could be written as

       max_clipboard = ((VDAgentMaxClipboard *)data)->max;
       syslog(LOG_DEBUG, "Set max clipboard: %d", max_clipboard);

>      case VD_AGENT_AUDIO_VOLUME_SYNC:
> -        if (message_header->size < sizeof(VDAgentAudioVolumeSync))
> -            goto size_error;
> -
>          do_client_volume_sync(vport, port_nr, message_header,
>                  (VDAgentAudioVolumeSync *)data);
>          break;
>      default:
> -        syslog(LOG_WARNING, "unknown message type %d, ignoring",
> -               message_header->type);
> +        g_warn_if_reached();
>      }
>  
>      return 0;
> -
> -size_error:
> -    syslog(LOG_ERR, "read: invalid message size: %u for message type: %u",
> -           message_header->size, message_header->type);
> -    return 0;
>  }
>  
>  static void virtio_write_clipboard(uint8_t selection, uint32_t msg_type,

Frediano