[v1,11/13] ui: place a hard cap on VNC server output buffer size

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 <berrange@redhat.com>
---
 ui/vnc.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Hi

On Mon, Dec 18, 2017 at 8:12 PM, Daniel P. Berrange <berrange@redhat.com> 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 <berrange@redhat.com>
> ---
>  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?

> +        return;
> +    }
>      buffer_reserve(&vs->output, len);
>
>      if (vs->ioc != NULL && buffer_empty(&vs->output)) {
> --
> 2.14.3
>
>

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 <berrange@redhat.com> 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 <berrange@redhat.com>
> > ---
> >  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