[Spice-devel] [spice v13 03/29] server: Add a GStreamer 1.0 MJPEG video encoder and use it by default

[Francois Gouget]

On Fri, 13 May 2016, Christophe Fergeau wrote:
[...]
> Yeah, the only reason for these questions is to know whether a malicious
> client could trigger misbehaviour in this part of the code by sending us
> some unexpected data. I agree when all is good, this does not make
> sense/should not happen, but I'd prefer to be 100% sure it cannot happen
> regardless of what we receive from the guest.

By 'malicious client' I guess you don't mean Spicy (since the video 
doesn't flow upstream). So presumably you're concerned by a buggy/rogue 
QXL driver in a VM.

In the video streaming case, both stream_stride and bitmap->stride come 
from RedDrawable.u.copy:
 * bitmap is the SpiceCopy.src_bitmap field
 * stream_stride is computed from the SpiceCopy.src_area field
  (src_area.right - src_area.left).

Now if the content of SpiceCopy cannot be trusted, the GStreamer encoder 
is the wrong place to check for issues : there are at least three places 
where the SpiceCopy bitmap can be used: the MJPEG encoder, the GStreamer 
encoder and the regular screen update code (which may or may not involve 
the JPEG encoder) [1].

Duplicating checks all over the place can only lead to maintenance 
issues and incomplete checks. So validating the SpiceCopy structure 
should be done before it is sent down one or the other path. The best 
place would probably be either before red_get_native_drawable() is 
called, inside red_get_native_drawable(), or in the relevant 
red_get_XXX_ptr() helper.

However I don't see any such check there or down the path:
 * red-parse-qxl.c never dereferences src_area.
 * dcc-send.c does not check consistency.

In fact I suggest trying variations of the following patch:

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 0dafbef..965b5ef 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -682,6 +682,8 @@ static int red_get_copy_ptr(RedMemSlotInfo *slots, int group_id,
         return 1;
     }
     red_get_rect_ptr(&red->src_area, &qxl->src_area);
+    red->src_area.right += 10;
+    red->src_area.bottom += 10;
     red->rop_descriptor  = qxl->rop_descriptor;
     red->scale_mode      = qxl->scale_mode;
     red_get_qmask_ptr(slots, group_id, &red->mask, &qxl->mask, flags);


Everything with go fine, modulo significant display issues in the 
client[2], until the MJPEG encoder tries to handle the first frame and 
the JPEG library kills the process with:

Improper call to JPEG library in state 101




[1] To be fair it looks like this last case is handled by sending the 
    whole bitmap (see spice_marshall_Copy() & spice_marshall_Image()), 
    even if only a small bit is supposed to be copied. That does not 
    seem very bandwidth efficient but it side-steps any src_bitmap vs. 
    src_area consistency issues.

[2] So the client does not validate the SpiceCopy object either. Whether 
    its behavior is sensible is not clear however.

-- 
Francois Gouget <fgouget at codeweavers.com>