[Spice-devel] [PATCH v6 4/5] vdagentd: early return on bad message size
Christophe Fergeau
cfergeau at redhat.com
Tue Jan 24 08:45:37 UTC 2017
Hey,
On Mon, Jan 23, 2017 at 02:53:54PM +0100, Michal Suchanek wrote:
> 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>
> Signed-off-by: Michal Suchanek <msuchanek at suse.de>
> ---
> v6
> - bring back the proper size check for clipboard messages
> - sort messages to fixed and variable length and check size accordingly
> - size the vdagent_message_min_size array with VD_AGENT_END_MESSAGE
Fwiw, not all comments from Frediano in
https://lists.freedesktop.org/archives/spice-devel/2017-January/035095.html
were answered (I'd also at least use G_N_ELEMENTS rather than a direct
check against VD_AGENT_END_MESSAGE).
> ---
> src/vdagentd/vdagentd.c | 111 +++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 86 insertions(+), 25 deletions(-)
>
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 785ce50..7e16cd2 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -341,6 +341,88 @@ static void do_client_file_xfer(struct vdagent_virtio_port *vport,
> udscs_write(conn, msg_type, 0, 0, data, message_header->size);
> }
>
> +static gsize vdagent_message_min_size[VD_AGENT_END_MESSAGE] =
> +{
> + -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 */
> +};
> +
> +static gboolean vdagent_message_check_size(VDAgentMessage *message_header)
> +{
> + uint32_t min_size = 0;
> +
> + if (message_header->protocol != VD_AGENT_PROTOCOL) {
> + syslog(LOG_ERR, "message with wrong protocol version ignoring");
> + return FALSE;
> + }
> +
> + if (message_header->type >= VD_AGENT_END_MESSAGE) {
> + syslog(LOG_WARNING, "unknown message type %d, ignoring",
> + message_header->type);
> + return FALSE;
> + }
> +
> + min_size = vdagent_message_min_size[message_header->type];
> + if (VD_AGENT_HAS_CAPABILITY(capabilities, capabilities_size,
> + VD_AGENT_CAP_CLIPBOARD_SELECTION)) {
> + switch (message_header->type) {
> + case VD_AGENT_CLIPBOARD_GRAB:
> + case VD_AGENT_CLIPBOARD_REQUEST:
> + case VD_AGENT_CLIPBOARD:
> + case VD_AGENT_CLIPBOARD_RELEASE:
> + min_size += 4;
> + }
> + }
> +
> + switch (message_header->type) {
> + case VD_AGENT_MONITORS_CONFIG:
> + case VD_AGENT_FILE_XFER_START:
> + case VD_AGENT_FILE_XFER_DATA:
> + case VD_AGENT_CLIPBOARD:
> + case VD_AGENT_CLIPBOARD_GRAB:
> + case VD_AGENT_AUDIO_VOLUME_SYNC:
> + case VD_AGENT_ANNOUNCE_CAPABILITIES:
> + if (message_header->size < min_size) {
> + syslog(LOG_ERR, "read: invalid message size: %u for message type: %u",
> + message_header->size, message_header->type);
> + return FALSE;
> + }
> + break;
> + case VD_AGENT_MOUSE_STATE:
> + case VD_AGENT_FILE_XFER_STATUS:
> + case VD_AGENT_DISPLAY_CONFIG:
> + case VD_AGENT_REPLY:
> + case VD_AGENT_CLIPBOARD_REQUEST:
> + case VD_AGENT_CLIPBOARD_RELEASE:
> + case VD_AGENT_MAX_CLIPBOARD:
> + case VD_AGENT_CLIENT_DISCONNECTED:
> + if (message_header->size != min_size) {
> + syslog(LOG_ERR, "read: invalid message size: %u for message type: %u",
> + message_header->size, message_header->type);
> + return FALSE;
> + }
> + break;
In this big switch, VD_AGENT_FILE_XFER_* and
VD_AGENT_CLIENT_DISCONNECTED were previously not subject to
a size check, VD_AGENT_DISPLAY_CONFIG and VD_AGENT_REPLY did not appear
in the switch, and VD_AGENT_CLIPBOARD_RELEASE/VD_AGENT_CLIPBOARD_RELEASE
were doing a < comparison, not a !=. I'm not necessarily opposed to
these changes, but I'd keep them for an additional commit, or at least
explain why this is ok to do in the commit log.
> + default:
> + g_warn_if_reached();
> + return FALSE;
> + }
> + return TRUE;
> +}
> +
> static int virtio_port_read_complete(
> struct vdagent_virtio_port *vport,
> int port_nr,
> @@ -349,26 +431,18 @@ static int virtio_port_read_complete(
> {
> uint32_t min_size = 0;
With the introduction of vdagent_message_check_size(), this variable is no longer
needed...
>
> - if (message_header->protocol != VD_AGENT_PROTOCOL) {
> - syslog(LOG_ERR, "message with wrong protocol version ignoring");
> + if (!vdagent_message_check_size(message_header))
> return 0;
> - }
>
> switch (message_header->type) {
> case VD_AGENT_MOUSE_STATE:
> - if (message_header->size != sizeof(VDAgentMouseState))
> - goto size_error;
> do_client_mouse(&uinput, (VDAgentMouseState *)data);
> 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;
> @@ -388,9 +462,6 @@ static int virtio_port_read_complete(
> VD_AGENT_CAP_CLIPBOARD_SELECTION)) {
> min_size += 4;
> }
... once you remove the block before this which still computes min_size,
which is no longer used.
Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170124/1f1c389a/attachment.sig>
More information about the Spice-devel
mailing list