Fix VNC server unbounded memory usage

Small nit on the subject s/parametr/parameter/

Otherwise, looks good.

Reviewed-by Darren Kenny <[email protected]>

Thanks,

Darren.

Quoted 58 lines · Daniel P. Berrange

There is only one caller of vnc_update_client and that always passes false for the 'sync' parameter.

Signed-off-by Daniel P. Berrange <[email protected]>

---
ui/vnc.c | 11 +++--------

1 file changed, 3 insertions(+), 8 deletions(-)

ui/vnc.c +3 -8

index 9f8d5a1b1f..7ba3297dfa 100644
@@ -596,7 +596,7 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp)
   3) resolutions > 1024
 */
 
-static int vnc_update_client(VncState *vs, int has_dirty, bool sync);
+static int vnc_update_client(VncState *vs, int has_dirty);
 static void vnc_disconnect_start(VncState *vs);
 
 static void vnc_colordepth(VncState *vs);

@@ -961,7 +961,7 @@ static int find_and_clear_dirty_height(VncState *vs,
    return h;
 }
 
-static int vnc_update_client(VncState *vs, int has_dirty, bool sync)
+static int vnc_update_client(VncState *vs, int has_dirty)
 {
    if (vs->disconnecting) {
        vnc_disconnect_finish(vs);

@@ -1025,9 +1025,6 @@ static int vnc_update_client(VncState *vs, int has_dirty, bool sync)
        }
 
        vnc_job_push(job);
-        if (sync) {
-            vnc_jobs_join(vs);
-        }
        vs->force_update = 0;
        vs->has_dirty = 0;
        return n;

@@ -1035,8 +1032,6 @@ static int vnc_update_client(VncState *vs, int has_dirty, bool sync)
 
    if (vs->disconnecting) {
        vnc_disconnect_finish(vs);
-    } else if (sync) {
-        vnc_jobs_join(vs);
    }
 
    return 0;

@@ -2863,7 +2858,7 @@ static void vnc_refresh(DisplayChangeListener *dcl)
    vnc_unlock_display(vd);
 
    QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
-        rects += vnc_update_client(vs, has_dirty, false);
+        rects += vnc_update_client(vs, has_dirty);
        /* vs might be free()ed here */
    }
 

Quoted 4 lines

Signature · 1 line

2.14.3

There is only one caller of vnc_update_client and that always passes false for the 'sync' parameter.

Quoted 2 lines · Daniel P. Berrange

There is only one caller of vnc_update_client and that always passes false for the 'sync' parameter.

Unused since commit 50628d3479e4f9aa97e323506856e394fe7ad7a6. Good catch.

What is the state of this series btw (/me busy wading through my email backlog ...) ?

Quoted 2 lines

What is the state of this series btw (/me busy wading through my email backlog ...) ?

Marc-André reviewed it, but I was assuming you would want to review and (if acceptable) queue it, since you're the designated VNC maintainer. There's just one nit-pick in the Subject of this mail so far.

The VNC server must throttle data sent to the client to prevent the 'output' buffer size growing without bound, if the client stops reading data off the socket (either maliciously or due to stalled/slow network connection).

The current throttling is very crude because it simply checks whether the output buffer offset is zero. This check is disabled if the client has requested a forced update, because we want to send these as soon as possible.

As a result, the VNC client can cause QEMU to allocate arbitrary amounts of RAM. They can first start something in the guest that triggers lots of framebuffer updates eg play a youtube video. Then repeatedly send full framebuffer update requests, but never read data back from the server. This can easily make QEMU's VNC server send buffer consume 100MB of RAM per second, until the OOM killer starts reaping processes (hopefully the rogue QEMU process, but it might pick others...).

To address this we make the throttling more intelligent, so we can throttle full updates. When we get a forced update request, we keep track of exactly how much data we put on the output buffer. We will not process a subsequent forced update request until this data has been fully sent on the wire. We always allow one forced update request to be in flight, regardless of what data is queued for incremental updates or audio data. The slight complication is that we do not initially know how much data an update will send, as this is done in the background by the VNC job thread. So we must track the fact that the job thread has an update pending, and not process any further updates until this job is has been completed & put data on the output buffer.

This unbounded memory growth affects all VNC server configurations supported by QEMU, with no workaround possible. The mitigating factor is that it can only be triggered by a client that has authenticated with the VNC server, and who is able to trigger a large quantity of framebuffer updates or audio samples from the guest OS. Mostly they'll just succeed in getting the OOM killer to kill their own QEMU process, but its possible other processes can get taken out as collateral damage.

This is a more general variant of the similar unbounded memory usage flaw in the websockets server, that was previously assigned CVE-2017-15268, and fixed in 2.11 by:

commit a7b20a8efa28e5f22c26c06cd06c2f12bc863493
Author: Daniel P. Berrange <[email protected]>
Date: Mon Oct 9 14:43:42 2017 +0100

io: monitor encoutput buffer size from websocket GSource

This new general memory usage flaw has been assigned CVE-2017-15124, and is partially fixed by this patch.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 ui/vnc-auth-sasl.c |  5 +++++
 ui/vnc-jobs.c      |  5 +++++
 ui/vnc.c           | 28 ++++++++++++++++++++++++----
 ui/vnc.h           |  7 +++++++
 4 files changed, 41 insertions(+), 4 deletions(-)

ui/vnc-auth-sasl.c +5 -0

index 761493b9b2..8c1cdde3db 100644
@@ -79,6 +79,11 @@ long vnc_client_write_sasl(VncState *vs)
 
     vs->sasl.encodedOffset += ret;
     if (vs->sasl.encodedOffset == vs->sasl.encodedLength) {
+        if (vs->sasl.encodedRawLength >= vs->force_update_offset) {
+            vs->force_update_offset = 0;
+        } else {
+            vs->force_update_offset -= vs->sasl.encodedRawLength;
+        }
         vs->output.offset -= vs->sasl.encodedRawLength;
         vs->sasl.encoded = NULL;
         vs->sasl.encodedOffset = vs->sasl.encodedLength = 0;

ui/vnc-jobs.c +5 -0

index f7867771ae..e326679dd0 100644
@@ -152,6 +152,11 @@ void vnc_jobs_consume_buffer(VncState *vs)
                 vs->ioc, G_IO_IN | G_IO_OUT, vnc_client_io, vs, NULL);
         }
         buffer_move(&vs->output, &vs->jobs_buffer);
+
+        if (vs->job_update == VNC_STATE_UPDATE_FORCE) {
+            vs->force_update_offset = vs->output.offset;
+        }
+        vs->job_update = VNC_STATE_UPDATE_NONE;
     }
     flush = vs->ioc != NULL && vs->abort != true;
     vnc_unlock_output(vs);

ui/vnc.c +17 -3

index a2699f534d..4021c0118c 100644
@@ -1021,14 +1021,28 @@ static bool vnc_should_update(VncState *vs)
         break;
     case VNC_STATE_UPDATE_INCREMENTAL:
         /* Only allow incremental updates if the pending send queue
-         * is less than the permitted threshold
+         * is less than the permitted threshold, and the job worker
+         * is completely idle.
          */
-        if (vs->output.offset < vs->throttle_output_offset) {
+        if (vs->output.offset < vs->throttle_output_offset &&
+            vs->job_update == VNC_STATE_UPDATE_NONE) {
             return true;
         }
         break;
     case VNC_STATE_UPDATE_FORCE:
-        return true;
+        /* Only allow forced updates if the pending send queue
+         * does not contain a previous forced update, and the
+         * job worker is completely idle.
+         *
+         * Note this means we'll queue a forced update, even if
+         * the output buffer size is otherwise over the throttle
+         * output limit.
+         */
+        if (vs->force_update_offset == 0 &&
+            vs->job_update == VNC_STATE_UPDATE_NONE) {
+            return true;
+        }
+        break;
     }
     return false;
 }

ui/vnc.c +6 -0

     vs->update = VNC_STATE_UPDATE_NONE;
+    vnc_job_push(job);
     vs->has_dirty = 0;
     return n;
 }

@@ -1332,6 +1347,11 @@ static ssize_t vnc_client_write_plain(VncState *vs)
     if (!ret)
         return 0;
 
+    if (ret >= vs->force_update_offset) {
+        vs->force_update_offset = 0;
+    } else {
+        vs->force_update_offset -= ret;
+    }
     buffer_advance(&vs->output, ret);
 
     if (vs->output.offset == 0) {

ui/vnc.h +7 -1

index 8fe69595c6..3f4cd4d93d 100644
@@ -271,6 +271,7 @@ struct VncState
 
     VncDisplay *vd;
     VncStateUpdate update; /* Most recent pending request from client */
+    VncStateUpdate job_update; /* Currently processed by job thread */
     int has_dirty;
     uint32_t features;
     int absolute;

@@ -298,6 +299,12 @@ struct VncState
 
     VncClientInfo *info;
 
+    /* Job thread bottom half has put data for a forced update
+     * into the output buffer. This offset points to the end of
+     * the update data in the output buffer. This lets us determine
+     * when a force update is fully sent to the client, allowing
+     * us to process further forced updates. */
+    size_t force_update_offset;
     /* We allow multiple incremental updates or audio capture
      * samples to be queued in output buffer, provided the
      * buffer size doesn't exceed this threshold. The value
--

2.14.3

Hi

Quoted 128 lines · Daniel P. Berrange

The VNC server must throttle data sent to the client to prevent the 'output' buffer size growing without bound, if the client stops reading data off the socket (either maliciously or due to stalled/slow network connection).

The current throttling is very crude because it simply checks whether the output buffer offset is zero. This check is disabled if the client has requested a forced update, because we want to send these as soon as possible.

As a result, the VNC client can cause QEMU to allocate arbitrary amounts of RAM. They can first start something in the guest that triggers lots of framebuffer updates eg play a youtube video. Then repeatedly send full framebuffer update requests, but never read data back from the server. This can easily make QEMU's VNC server send buffer consume 100MB of RAM per second, until the OOM killer starts reaping processes (hopefully the rogue QEMU process, but it might pick others...).

To address this we make the throttling more intelligent, so we can throttle full updates. When we get a forced update request, we keep track of exactly how much data we put on the output buffer. We will not process a subsequent forced update request until this data has been fully sent on the wire. We always allow one forced update request to be in flight, regardless of what data is queued for incremental updates or audio data. The slight complication is that we do not initially know how much data an update will send, as this is done in the background by the VNC job thread. So we must track the fact that the job thread has an update pending, and not process any further updates until this job is has been completed & put data on the output buffer.

This unbounded memory growth affects all VNC server configurations supported by QEMU, with no workaround possible. The mitigating factor is that it can only be triggered by a client that has authenticated with the VNC server, and who is able to trigger a large quantity of framebuffer updates or audio samples from the guest OS. Mostly they'll just succeed in getting the OOM killer to kill their own QEMU process, but its possible other processes can get taken out as collateral damage.

This is a more general variant of the similar unbounded memory usage flaw in the websockets server, that was previously assigned CVE-2017-15268, and fixed in 2.11 by:

commit a7b20a8efa28e5f22c26c06cd06c2f12bc863493
Author: Daniel P. Berrange <[email protected]>
Date: Mon Oct 9 14:43:42 2017 +0100

io: monitor encoutput buffer size from websocket GSource

This new general memory usage flaw has been assigned CVE-2017-15124, and is partially fixed by this patch.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 ui/vnc-auth-sasl.c |  5 +++++
 ui/vnc-jobs.c      |  5 +++++
 ui/vnc.c           | 28 ++++++++++++++++++++++++----
 ui/vnc.h           |  7 +++++++
 4 files changed, 41 insertions(+), 4 deletions(-)

ui/vnc-auth-sasl.c +5 -0

index 761493b9b2..8c1cdde3db 100644
@@ -79,6 +79,11 @@ long vnc_client_write_sasl(VncState *vs)
 
     vs->sasl.encodedOffset += ret;
     if (vs->sasl.encodedOffset == vs->sasl.encodedLength) {
+        if (vs->sasl.encodedRawLength >= vs->force_update_offset) {
+            vs->force_update_offset = 0;
+        } else {
+            vs->force_update_offset -= vs->sasl.encodedRawLength;
+        }
         vs->output.offset -= vs->sasl.encodedRawLength;
         vs->sasl.encoded = NULL;
         vs->sasl.encodedOffset = vs->sasl.encodedLength = 0;

ui/vnc-jobs.c +5 -0

index f7867771ae..e326679dd0 100644
@@ -152,6 +152,11 @@ void vnc_jobs_consume_buffer(VncState *vs)
                 vs->ioc, G_IO_IN | G_IO_OUT, vnc_client_io, vs, NULL);
         }
         buffer_move(&vs->output, &vs->jobs_buffer);
+
+        if (vs->job_update == VNC_STATE_UPDATE_FORCE) {
+            vs->force_update_offset = vs->output.offset;
+        }
+        vs->job_update = VNC_STATE_UPDATE_NONE;
     }
     flush = vs->ioc != NULL && vs->abort != true;
     vnc_unlock_output(vs);

ui/vnc.c +17 -3

index a2699f534d..4021c0118c 100644
@@ -1021,14 +1021,28 @@ static bool vnc_should_update(VncState *vs)
         break;
     case VNC_STATE_UPDATE_INCREMENTAL:
         /* Only allow incremental updates if the pending send queue
-         * is less than the permitted threshold
+         * is less than the permitted threshold, and the job worker
+         * is completely idle.
          */
-        if (vs->output.offset < vs->throttle_output_offset) {
+        if (vs->output.offset < vs->throttle_output_offset &&
+            vs->job_update == VNC_STATE_UPDATE_NONE) {
             return true;
         }
         break;
     case VNC_STATE_UPDATE_FORCE:
-        return true;
+        /* Only allow forced updates if the pending send queue
+         * does not contain a previous forced update, and the
+         * job worker is completely idle.
+         *
+         * Note this means we'll queue a forced update, even if
+         * the output buffer size is otherwise over the throttle
+         * output limit.
+         */
+        if (vs->force_update_offset == 0 &&
+            vs->job_update == VNC_STATE_UPDATE_NONE) {
+            return true;
+        }
+        break;
     }
     return false;
 }

@@ -1096,8 +1110,9 @@ static int vnc_update_client(VncState *vs, int has_dirty)
         }
     }
 

ui/vnc.c static int vnc_update_client(VncState *vs, int has_dirty) · line 1113

Quoted 2 lines

ui/vnc.c +1 -1

-    vnc_job_push(job);
+    vs->job_update = vs->update;

How is this going to match the buffer job in vnc_jobs_consume_buffer() ?

(isn't this potentially taking the next job to finish as a force-update?)

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(+)

ui/vnc.c +17 -0

index 4021c0118c..a4f0279cdc 100644
@@ -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

ui/vnc.c +2 -1

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

2.14.3

Hi

Quoted 62 lines · Daniel P. Berrange

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(+)

ui/vnc.c +17 -0

index 4021c0118c..a4f0279cdc 100644
@@ -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

ui/vnc.c gboolean vnc_client_io(QIOChannel *ioc G_GNUC_UNUSED, · line 1552

Quoted 10 lines

ui/vnc.c +10 -0

+     * 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?

In the 2.11 release we fixed CVE-2017-15268, which allowed the VNC websockets server to consume arbitrary memory when a slow client was connected. I have since discovered that this same type of problem can be triggered in several other ways in the regular (non-websockets) VNC server. This patch series attempts to fix this problem by limiting framebuffer updates and other data sent from server to client. The mitigating factor is that you need to have successfully authenticated with the VNC server to trigger these new flaws. This new more general flaw is assigned CVE-2017-15124 by the Red Hat security team.

The key patches containing the security fix are 9, 10, 11.

Since this code is incredibly subtle & hard to understand though, the first 8 patches do a bunch of independant cleanups/refactoring to make the security fixes clearer. The last two patches are just some extra cleanup / help for future maint.

Daniel P. Berrange (13):
ui: remove 'sync' parametr from vnc_update_client
ui: remove unreachable code in vnc_update_client
ui: remove redundant indentation in vnc_client_update
ui: avoid pointless VNC updates if framebuffer isn't dirty
ui: track how much decoded data we consumed when doing SASL encoding
ui: introduce enum to track VNC client framebuffer update request
state
ui: correctly reset framebuffer update state after processing dirty
regions
ui: refactor code for determining if an update should be sent to the
client
ui: fix VNC client throttling when audio capture is active
ui: fix VNC client throttling when forced update is requested
ui: place a hard cap on VNC server output buffer size
ui: add trace events related to VNC client throttling
ui: mix misleading comments & return types of VNC I/O helper methods

ui/trace-events    |   7 ++
ui/vnc-auth-sasl.c |  16 ++-
ui/vnc-auth-sasl.h |   5 +-
ui/vnc-jobs.c      |   5 +
ui/vnc.c           | 320 ++++++++++++++++++++++++++++++++++++++---------------
ui/vnc.h           |  28 ++++-
6 files changed, 277 insertions(+), 104 deletions(-)

Signature · 1 line

2.14.3

In the 2.11 release we fixed CVE-2017-15268, which allowed the VNC websockets server to consume arbitrary memory when a slow client was connected. I have since discovered that this same type of problem can be triggered in several other ways in the regular (non-websockets) VNC server. This patch series attempts to fix this problem by limiting framebuffer updates and other data sent from server to client. The mitigating factor is that you need to have successfully authenticated with the VNC server to trigger these new flaws. This new more general flaw is assigned CVE-2017-15124 by the Red Hat security team.

The key patches containing the security fix are 9, 10, 11.

Since this code is incredibly subtle & hard to understand though, the first 8 patches do a bunch of independant cleanups/refactoring to make the security fixes clearer. The last two patches are just some extra cleanup / help for future maint.

Daniel P. Berrange (13):
ui: remove 'sync' parametr from vnc_update_client
ui: remove unreachable code in vnc_update_client
ui: remove redundant indentation in vnc_client_update
ui: avoid pointless VNC updates if framebuffer isn't dirty
ui: track how much decoded data we consumed when doing SASL encoding
ui: introduce enum to track VNC client framebuffer update request
state
ui: correctly reset framebuffer update state after processing dirty
regions
ui: refactor code for determining if an update should be sent to the
client
ui: fix VNC client throttling when audio capture is active
ui: fix VNC client throttling when forced update is requested
ui: place a hard cap on VNC server output buffer size
ui: add trace events related to VNC client throttling
ui: mix misleading comments & return types of VNC I/O helper methods

 ui/trace-events    |   7 ++
 ui/vnc-auth-sasl.c |  16 ++-
 ui/vnc-auth-sasl.h |   5 +-
 ui/vnc-jobs.c      |   5 +
 ui/vnc.c           | 320
 ++++++++++++++++++++++++++++++++++++++---------------
 ui/vnc.h           |  28 ++++-
 6 files changed, 277 insertions(+), 104 deletions(-)

--
2.14.3

Hi

----- Original Message -----

Quoted 10 lines

In the 2.11 release we fixed CVE-2017-15268, which allowed the VNC websockets server to consume arbitrary memory when a slow client was connected. I have since discovered that this same type of problem can be triggered in several other ways in the regular (non-websockets) VNC server. This patch series attempts to fix this problem by limiting framebuffer updates and other data sent from server to client. The mitigating factor is that you need to have successfully authenticated with the VNC server to trigger these new flaws. This new more general flaw is assigned CVE-2017-15124 by the Red Hat security team.

Quoted 6 lines

The key patches containing the security fix are 9, 10, 11.

Since this code is incredibly subtle & hard to understand though, the first 8 patches do a bunch of independant cleanups/refactoring to make the security fixes clearer. The last two patches are just some extra cleanup / help for future maint.

I already reviewed the series privately, and in general, I'd ack it.

Did you manage to run some throttle tests? (I tried to trigger them manually by slowing artificially network or calling framebuffer_update_request() in gdb, I didn't manage yet :-/)

In the 2.11 release we fixed CVE-2017-15268, which allowed the VNC websockets server to consume arbitrary memory when a slow client was connected. I have since discovered that this same type of problem can be triggered in several other ways in the regular (non-websockets) VNC server. This patch series attempts to fix this problem by limiting framebuffer updates and other data sent from server to client. The mitigating factor is that you need to have successfully authenticated with the VNC server to trigger these new flaws. This new more general flaw is assigned CVE-2017-15124 by the Red Hat security team.

The key patches containing the security fix are 9, 10, 11.

Since this code is incredibly subtle & hard to understand though, the first 8 patches do a bunch of independant cleanups/refactoring to make the security fixes clearer. The last two patches are just some extra cleanup / help for future maint.

Daniel P. Berrange (13):
ui: remove 'sync' parametr from vnc_update_client
ui: remove unreachable code in vnc_update_client
ui: remove redundant indentation in vnc_client_update
ui: avoid pointless VNC updates if framebuffer isn't dirty
ui: track how much decoded data we consumed when doing SASL encoding
ui: introduce enum to track VNC client framebuffer update request
state
ui: correctly reset framebuffer update state after processing dirty
regions
ui: refactor code for determining if an update should be sent to the
client
ui: fix VNC client throttling when audio capture is active
ui: fix VNC client throttling when forced update is requested
ui: place a hard cap on VNC server output buffer size
ui: add trace events related to VNC client throttling
ui: mix misleading comments & return types of VNC I/O helper methods

 ui/trace-events    |   7 ++
 ui/vnc-auth-sasl.c |  16 ++-
 ui/vnc-auth-sasl.h |   5 +-
 ui/vnc-jobs.c      |   5 +
 ui/vnc.c           | 320 ++++++++++++++++++++++++++++++++++++++---------------
 ui/vnc.h           |  28 ++++-
 6 files changed, 277 insertions(+), 104 deletions(-)