yeahuh / rpms / qemu-kvm

Forked from rpms/qemu-kvm 2 years ago
Clone

Blame SOURCES/kvm-ui-place-a-hard-cap-on-VNC-server-output-buffer-size.patch

5d360b
From 52fe55e2bf9df408ebe127a670ee698642d3fcb4 Mon Sep 17 00:00:00 2001
5d360b
From: "Daniel P. Berrange" <berrange@redhat.com>
5d360b
Date: Thu, 8 Feb 2018 17:50:38 +0100
5d360b
Subject: [PATCH 24/27] ui: place a hard cap on VNC server output buffer size
5d360b
MIME-Version: 1.0
5d360b
Content-Type: text/plain; charset=UTF-8
5d360b
Content-Transfer-Encoding: 8bit
5d360b
5d360b
RH-Author: Daniel P. Berrange <berrange@redhat.com>
5d360b
Message-id: <20180208175041.5634-25-berrange@redhat.com>
5d360b
Patchwork-id: 78957
5d360b
O-Subject: [RHEL-7.5 qemu-kvm PATCH v1 24/27] ui: place a hard cap on VNC server output buffer size
5d360b
Bugzilla: 1527405
5d360b
RH-Acked-by: Laszlo Ersek <lersek@redhat.com>
5d360b
RH-Acked-by: Gerd Hoffmann <kraxel@redhat.com>
5d360b
RH-Acked-by: Miroslav Rezanina <mrezanin@redhat.com>
5d360b
5d360b
From: "Daniel P. Berrange" <berrange@redhat.com>
5d360b
5d360b
The previous patches fix problems with throttling of forced framebuffer updates
5d360b
and audio data capture that would cause the QEMU output buffer size to grow
5d360b
without bound. Those fixes are graceful in that once the client catches up with
5d360b
reading data from the server, everything continues operating normally.
5d360b
5d360b
There is some data which the server sends to the client that is impractical to
5d360b
throttle. Specifically there are various pseudo framebuffer update encodings to
5d360b
inform the client of things like desktop resizes, pointer changes, audio
5d360b
playback start/stop, LED state and so on. These generally only involve sending
5d360b
a very small amount of data to the client, but a malicious guest might be able
5d360b
to do things that trigger these changes at a very high rate. Throttling them is
5d360b
not practical as missed or delayed events would cause broken behaviour for the
5d360b
client.
5d360b
5d360b
This patch thus takes a more forceful approach of setting an absolute upper
5d360b
bound on the amount of data we permit to be present in the output buffer at
5d360b
any time. The previous patch set a threshold for throttling the output buffer
5d360b
by allowing an amount of data equivalent to one complete framebuffer update and
5d360b
one seconds worth of audio data. On top of this it allowed for one further
5d360b
forced framebuffer update to be queued.
5d360b
5d360b
To be conservative, we thus take that throttling threshold and multiply it by
5d360b
5 to form an absolute upper bound. If this bound is hit during vnc_write() we
5d360b
forceably disconnect the client, refusing to queue further data. This limit is
5d360b
high enough that it should never be hit unless a malicious client is trying to
5d360b
exploit the sever, or the network is completely saturated preventing any sending
5d360b
of data on the socket.
5d360b
5d360b
This completes the fix for CVE-2017-15124 started in the previous patches.
5d360b
5d360b
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
5d360b
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
5d360b
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
5d360b
Message-id: 20171218191228.31018-12-berrange@redhat.com
5d360b
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
5d360b
(cherry picked from commit f887cf165db20f405cb8805c716bd363aaadf815)
5d360b
5d360b
 Conflicts:
5d360b
	ui/vnc.c - context differences and no 'vs->disconnecting' flag.
5d360b
	Using share_mode as a better check for the disconnecting state
5d360b
	than csock == -1, because the worker thread calls vnc_write()
5d360b
	with a fake VncState that has csock == -1.
5d360b
5d360b
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
5d360b
---
5d360b
 ui/vnc.c | 29 +++++++++++++++++++++++++++++
5d360b
 1 file changed, 29 insertions(+)
5d360b
5d360b
diff --git a/ui/vnc.c b/ui/vnc.c
5d360b
index 96b6caf..61fbec2 100644
5d360b
--- a/ui/vnc.c
5d360b
+++ b/ui/vnc.c
5d360b
@@ -1460,8 +1460,37 @@ void vnc_client_read(void *opaque)
5d360b
     }
5d360b
 }
5d360b
 
5d360b
+/*
5d360b
+ * Scale factor to apply to vs->throttle_output_offset when checking for
5d360b
+ * hard limit. Worst case normal usage could be x2, if we have a complete
5d360b
+ * incremental update and complete forced update in the output buffer.
5d360b
+ * So x3 should be good enough, but we pick x5 to be conservative and thus
5d360b
+ * (hopefully) never trigger incorrectly.
5d360b
+ */
5d360b
+#define VNC_THROTTLE_OUTPUT_LIMIT_SCALE 5
5d360b
+
5d360b
 void vnc_write(VncState *vs, const void *data, size_t len)
5d360b
 {
5d360b
+    if (vs->share_mode == VNC_SHARE_MODE_DISCONNECTED) {
5d360b
+        return;
5d360b
+    }
5d360b
+    /* Protection against malicious client/guest to prevent our output
5d360b
+     * buffer growing without bound if client stops reading data. This
5d360b
+     * should rarely trigger, because we have earlier throttling code
5d360b
+     * which stops issuing framebuffer updates and drops audio data
5d360b
+     * if the throttle_output_offset value is exceeded. So we only reach
5d360b
+     * this higher level if a huge number of pseudo-encodings get
5d360b
+     * triggered while data can't be sent on the socket.
5d360b
+     *
5d360b
+     * NB throttle_output_offset can be zero during early protocol
5d360b
+     * handshake, or from the job thread's VncState clone
5d360b
+     */
5d360b
+    if (vs->throttle_output_offset != 0 &&
5d360b
+        vs->output.offset > (vs->throttle_output_offset *
5d360b
+                             VNC_THROTTLE_OUTPUT_LIMIT_SCALE)) {
5d360b
+        vnc_disconnect_start(vs);
5d360b
+        return;
5d360b
+    }
5d360b
     buffer_reserve(&vs->output, len);
5d360b
 
5d360b
     if (vs->csock != -1 && buffer_empty(&vs->output)) {
5d360b
-- 
5d360b
1.8.3.1
5d360b