On Wed, Dec 20, 2017 at 12:32:51PM +0100, Marc-André Lureau wrote: > Hi > > On Mon, Dec 18, 2017 at 8:12 PM, Daniel P. Berrange <[email protected]> > wrote: > > The previous patches fix problems with throttling of forced framebuffer > > updates > > and audio data capture that would cause the QEMU output buffer size to grow > > without bound. Those fixes are graceful in that once the client catches up > > with > > reading data from the server, everything continues operating normally. > > > > There is some data which the server sends to the client that is impractical > > to > > throttle. Specifically there are various pseudo framebuffer update > > encodings to > > inform the client of things like desktop resizes, pointer changes, audio > > playback start/stop, LED state and so on. These generally only involve > > sending > > a very small amount of data to the client, but a malicious guest might be > > able > > to do things that trigger these changes at a very high rate. Throttling > > them is > > not practical as missed or delayed events would cause broken behaviour for > > the > > client. > > > > This patch thus takes a more forceful approach of setting an absolute upper > > bound on the amount of data we permit to be present in the output buffer at > > any time. The previous patch set a threshold for throttling the output > > buffer > > by allowing an amount of data equivalent to one complete framebuffer update > > and > > one seconds worth of audio data. On top of this it allowed for one further > > forced framebuffer update to be queued. > > > > To be conservative, we thus take that throttling threshold and multiply it > > by > > 5 to form an absolute upper bound. If this bound is hit during vnc_write() > > we > > forceably disconnect the client, refusing to queue further data. This limit > > is > > high enough that it should never be hit unless a malicious client is trying > > to > > exploit the sever, or the network is completely saturated preventing any > > sending > > of data on the socket. > > > > This completes the fix for CVE-2017-15124 started in the previous patches. > > > > Signed-off-by: Daniel P. Berrange <[email protected]> > > --- > > ui/vnc.c | 29 +++++++++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/ui/vnc.c b/ui/vnc.c > > index 4021c0118c..a4f0279cdc 100644 > > --- a/ui/vnc.c > > +++ b/ui/vnc.c > > @@ -1521,8 +1521,37 @@ gboolean vnc_client_io(QIOChannel *ioc G_GNUC_UNUSED, > > } > > > > > > +/* > > + * Scale factor to apply to vs->throttle_output_offset when checking for > > + * hard limit. Worst case normal usage could be x2, if we have a complete > > + * incremental update and complete forced update in the output buffer. > > + * So x3 should be good enough, but we pick x5 to be conservative and thus > > + * (hopefully) never trigger incorrectly. > > + */ > > +#define VNC_THROTTLE_OUTPUT_LIMIT_SCALE 5 > > + > > void vnc_write(VncState *vs, const void *data, size_t len) > > { > > + if (vs->disconnecting) { > > + return; > > + } > > + /* Protection against malicious client/guest to prevent our output > > + * buffer growing without bound if client stops reading data. This > > + * should rarely trigger, because we have earlier throttling code > > + * which stops issuing framebuffer updates and drops audio data > > + * if the throttle_output_offset value is exceeded. So we only reach > > + * this higher level if a huge number of pseudo-encodings get > > + * triggered while data can't be sent on the socket. > > + * > > + * NB throttle_output_offset can be zero during early protocol > > + * handshake, or from the job thread's VncState clone > > + */ > > + if (vs->throttle_output_offset != 0 && > > + vs->output.offset > (vs->throttle_output_offset * > > + VNC_THROTTLE_OUTPUT_LIMIT_SCALE)) { > > + vnc_disconnect_start(vs); > > It seems to me that the main source of data, the display, bypass this check. > > The vnc_worker_thread_loop() uses a local VncState & buffer. The > result is moved to the vs->jobs_buffer, which is later moved in > vnc_jobs_consume_buffer() to the vs->output in bottom-half. > > So in theory, it seems it would be possible for a client to make > several update-request (assuming guest display content changed), and > have several vnc jobs queued. In the unlikely events they would be > consumed together, they would not respect the hard cap. I am not sure > how the vnc-job queueing should be improved, just wanted to raise some > concerns around that code and the fact it doesn't really respect the > hard limits apparently. Am I wrong? > > Perhaps the hard limit should also be put in vnc_jobs_consume_buffer() > ? Then I can imagine synchronization issues if the hard limit changes > before the job buffer are consumed. > > May be we should limit the amount of jobs that can be queued? If we > can estimate the max result buffer size of a job buffer, > vnc_should_update() could take that into account?
The vnc_should_update() already prevents there being more than 1 queued job for the worker thread. So even if the client reuqests more updates, we won't start processing them until the worker thread as copied the job_buffer into output in the vnc_jobs_consume_buffer bottom half. So no matter what the client requests, and how frequently the guest display updates, we're still limiting output buffer size in the vnc_update_client method. This vnc_write protection only needs to cope with non-display updates, for things like psuedo-encoding messages. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
