[Spice-devel] [vdagent-linux v8 1/4] vdagentd: early return on bad message size
From: Victor Toso
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
Signed-off-by: Michal Suchanek
---
src/vdagentd/vdagentd.c | 133 +---
1 file changed, 91 insertions(+), 42 deletions(-)
diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index 5ca0106..ea8482b 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -341,34 +341,109 @@ 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[] =
+{
+-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(const 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 ||
+message_header->type >= G_N_ELEMENTS(vdagent_message_min_size)) {
+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_CLIPBOARD:
+case VD_AGENT_CLIPBOARD_GRAB:
+case VD_AGENT_CLIPBOARD_REQUEST:
+case VD_AGENT_CLIPBOARD_RELEASE:
+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_MAX_CLIPBOARD:
+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_FILE_XFER_START:
+case VD_AGENT_FILE_XFER_DATA:
+case VD_AGENT_FILE_XFER_STATUS:
+case VD_AGENT_CLIENT_DISCONNECTED:
+/* No size checks for these at the moment */
+break;
+case VD_AGENT_DISPLAY_CONFIG:
+case VD_AGENT_REPLY:
+default:
+g_warn_if_reached();
+return FALSE;
+}
+return TRUE;
+}
+
static int virtio_port_read_complete(
struct vdagent_virtio_port *vport,
int port_nr,
VDAgentMessage *message_header,
uint8_t *data)
{
-uint32_t min_size = 0;
-
-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);
brea
Re: [Spice-devel] [vdagent-linux v8 1/4] vdagentd: early return on bad message size
Hi,
On Wed, Feb 15, 2017 at 10:48:04AM +0100, Victor Toso wrote:
> From: Victor Toso
>
> 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().
This is not 100% true anymore. We've also created a
vdagent_message_check_size() which is the one that uses the
vdagent_message_min_size[].
Maybe:
"This patch creates:
* vdagent_message_min_size[] static array with the
expected size for each message;
* vdagent_message_check_size() which checks the size of message's
payload based on the type of message and by using
vdagent_message_min_size[] as reference"
(maybe it got worse, comments are welcome :))
>
> Signed-off-by: Victor Toso
> Signed-off-by: Michal Suchanek
> ---
> src/vdagentd/vdagentd.c | 133
> +---
> 1 file changed, 91 insertions(+), 42 deletions(-)
>
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 5ca0106..ea8482b 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -341,34 +341,109 @@ 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[] =
> +{
> +-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(const 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 ||
> +message_header->type >= G_N_ELEMENTS(vdagent_message_min_size)) {
> +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_CLIPBOARD:
> +case VD_AGENT_CLIPBOARD_GRAB:
> +case VD_AGENT_CLIPBOARD_REQUEST:
> +case VD_AGENT_CLIPBOARD_RELEASE:
> +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_MAX_CLIPBOARD:
> +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_FILE_XFER_START:
> +case VD_AGENT_FILE_XFER_DATA:
> +case VD_AGENT_FILE_XFER_STATUS:
> +case VD_AGENT_CLIENT_DISCONNECTED:
> +/* No size checks for these at the moment */
> +break;
> +case VD_AGENT_DISPLAY_CONFIG:
> +case VD_AGENT_REPLY:
> +default:
> +g_warn_if_reached();
> +return FALSE;
> +}
> +return TRUE;
> +}
> +
> static int virtio_port_read_complete(
> struct vdagent_virtio_port *vport,
> int port_nr,
> VDAgentMessage *message_header,
>
Re: [Spice-devel] [vdagent-linux v8 1/4] vdagentd: early return on bad message size
On Wed, Feb 15, 2017 at 10:58:14AM +0100, Victor Toso wrote:
> Hi,
>
> On Wed, Feb 15, 2017 at 10:48:04AM +0100, Victor Toso wrote:
> > From: Victor Toso
> >
> > 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().
>
> This is not 100% true anymore. We've also created a
> vdagent_message_check_size() which is the one that uses the
> vdagent_message_min_size[].
>
> Maybe:
>
> "This patch creates:
> * vdagent_message_min_size[] static array with the
>expected size for each message;
> * vdagent_message_check_size() which checks the size of message's
>payload based on the type of message and by using
>vdagent_message_min_size[] as reference"
Yup, looks good this way and the patch overall looked fine as I remember
it ;)
Christophe
>
> (maybe it got worse, comments are welcome :))
>
> >
> > Signed-off-by: Victor Toso
> > Signed-off-by: Michal Suchanek
> > ---
> > src/vdagentd/vdagentd.c | 133
> > +---
> > 1 file changed, 91 insertions(+), 42 deletions(-)
> >
> > diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> > index 5ca0106..ea8482b 100644
> > --- a/src/vdagentd/vdagentd.c
> > +++ b/src/vdagentd/vdagentd.c
> > @@ -341,34 +341,109 @@ 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[] =
> > +{
> > +-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(const 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 ||
> > +message_header->type >= G_N_ELEMENTS(vdagent_message_min_size)) {
> > +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_CLIPBOARD:
> > +case VD_AGENT_CLIPBOARD_GRAB:
> > +case VD_AGENT_CLIPBOARD_REQUEST:
> > +case VD_AGENT_CLIPBOARD_RELEASE:
> > +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_MAX_CLIPBOARD:
> > +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_FILE_XFER_START:
> > +case VD_AGENT_FILE_XFER_DATA:
> > +case VD_AGENT_FILE_XFER_STATUS:
> > +case VD_AGENT_CLIENT_DISCONNECTED:
> > +
