[Qemu-devel] [PATCH v1 0/7] Limit websockets memory usage & other bug fixes

[Qemu-devel] [PATCH v1 0/7] Limit websockets memory usage & other bug fixes

[Daniel P. Berrange]

The core motivation for this patch series is to fix a security
issue publically reported, where websockets code can consume
arbitrary amounts of RAM with slow clients:

   https://bugs.launchpad.net/qemu/+bug/1718964

I've asked for a CVE but its not assigned yet. Since the bug
is public we might as well get on with code review while waiting
for the CVE number.

The first patch is the minimum required to fix the actual CVE
in git master, taking advantage of how we know the VNC server
will call us. The 5th patch lets us tighten up buffer limiting
of writes further, so we're not making assumptions about VNC
server code.

The websockets code is broken right back to the day it was
merged in QEMU 1.2.1

The fix in patch 1 can apply to stable branches from 2.6 -> 2.10
inclusive, provided another fix from master is cherry-picked
first

  commit eefa3d8ef649f9055611361e2201cca49f8c3433
  Author: Brandon Carpenter <brandon.carpenter@cypherpath.com>
  Date:   Tue Sep 12 08:21:48 2017 -0700

    io: Small updates in preparation for websocket changes

since that refactors code duplication in the GSource impl.
Once we merge for master, I'll send a patch to qemu-stable.

Versions prior to 2.6 would require a fix to be done in
the ui/vnc.c file vnc_update_client method instead. It
would need to check vs->ws_output buffer size. I'm not
intending to write any such patch - this is just info in
case anyone is stuck on such ancient versions and needs
to figure out a fix.

Daniel P. Berrange (7):
  io: monitor encoutput buffer size from websocket GSource
  io: simplify websocket ping reply handling
  io: get rid of qio_channel_websock_encode helper method
  io: pass a struct iovec into qio_channel_websock_encode
  io: get rid of bounce buffering in websock write path
  io: cope with websock 'Connection' header having multiple values
  io: add trace points for websocket HTTP protocol headers

 include/io/channel-websock.h |   3 +-
 io/channel-websock.c         | 163 ++++++++++++++++++++++++-------------------
 io/trace-events              |   2 +
 3 files changed, 93 insertions(+), 75 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH v1 1/7] io: monitor encoutput buffer size from websocket GSource

[Daniel P. Berrange]

The websocket GSource is monitoring the size of the rawoutput
buffer to determine if the channel can accepts more writes.
The rawoutput buffer, however, is merely a temporary staging
buffer before data is copied into the encoutput buffer. This
its size will always be zero when the GSource runs.

This flaw causes the encoutput buffer to grow without bound
if the other end of the underlying data channel doesn't
read data being sent. This can be seen with VNC if a client
is on a slow WAN link and the guest OS is sending many screen
updates. A malicious VNC client can act like it is on a slow
link by playing a video in the guest and then reading data
very slowly, causing QEMU host memory to expand arbitrarily.

This issue is assigned CVE-2017-????, publically reported in

  https://bugs.launchpad.net/qemu/+bug/1718964

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-websock.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/io/channel-websock.c b/io/channel-websock.c
index d1d471f86e..04bcc059cd 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -28,7 +28,7 @@
 #include <time.h>
 
 
-/* Max amount to allow in rawinput/rawoutput buffers */
+/* Max amount to allow in rawinput/encoutput buffers */
 #define QIO_CHANNEL_WEBSOCK_MAX_BUFFER 8192
 
 #define QIO_CHANNEL_WEBSOCK_CLIENT_KEY_LEN 24
@@ -1208,7 +1208,7 @@ qio_channel_websock_source_check(GSource *source)
     if (wsource->wioc->rawinput.offset || wsource->wioc->io_eof) {
         cond |= G_IO_IN;
     }
-    if (wsource->wioc->rawoutput.offset < QIO_CHANNEL_WEBSOCK_MAX_BUFFER) {
+    if (wsource->wioc->encoutput.offset < QIO_CHANNEL_WEBSOCK_MAX_BUFFER) {
         cond |= G_IO_OUT;
     }
 
-- 
2.13.5

[Qemu-devel] [PATCH v1 2/7] io: simplify websocket ping reply handling

[Daniel P. Berrange]

We must ensure we don't get flooded with ping replies if the outbound
channel is slow. Currently we do this by keeping the ping reply in a
separate temporary buffer and only writing it if the encoutput buffer
is completely empty. This is overly pessimistic, as it is reasonable
to add a ping reply to the encoutput buffer even if it has previous
data in it, as long as that previous data doesn't include a ping
reply.

To track this better, put the ping reply directly into the encoutput
buffer, and then record the size of encoutput at this time in
ping_remain. As we write encoutput to the underlying channel, we
can decrement the ping_remain counter. Once it hits zero, we can
accept further ping replies for transmission.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/io/channel-websock.h |  2 +-
 io/channel-websock.c         | 28 +++++++++++++++-------------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/include/io/channel-websock.h b/include/io/channel-websock.h
index ff32d8651b..3f92535cae 100644
--- a/include/io/channel-websock.h
+++ b/include/io/channel-websock.h
@@ -60,8 +60,8 @@ struct QIOChannelWebsock {
     Buffer encoutput;
     Buffer rawinput;
     Buffer rawoutput;
-    Buffer ping_reply;
     size_t payload_remain;
+    size_t ping_remain;
     QIOChannelWebsockMask mask;
     guint io_tag;
     Error *io_err;
diff --git a/io/channel-websock.c b/io/channel-websock.c
index 04bcc059cd..1c34f68012 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -825,11 +825,14 @@ static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
         }
         return -1;
     } else if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_PING) {
-        /* ping frames produce an immediate reply */
-        buffer_reset(&ioc->ping_reply);
-        qio_channel_websock_encode_buffer(
-            ioc, &ioc->ping_reply, QIO_CHANNEL_WEBSOCK_OPCODE_PONG,
-            &ioc->encinput);
+        /* ping frames produce an immediate reply, as long as we've not still
+         * got a previous ping queued, in which case we drop the new pong */
+        if (ioc->ping_remain == 0) {
+            qio_channel_websock_encode_buffer(
+                ioc, &ioc->encoutput, QIO_CHANNEL_WEBSOCK_OPCODE_PONG,
+                &ioc->encinput);
+            ioc->ping_remain = ioc->encoutput.offset;
+        }
     }   /* pong frames are ignored */
 
     if (payload_len) {
@@ -888,7 +891,6 @@ static void qio_channel_websock_finalize(Object *obj)
     buffer_free(&ioc->encoutput);
     buffer_free(&ioc->rawinput);
     buffer_free(&ioc->rawoutput);
-    buffer_free(&ioc->ping_reply);
     object_unref(OBJECT(ioc->master));
     if (ioc->io_tag) {
         g_source_remove(ioc->io_tag);
@@ -946,12 +948,7 @@ static ssize_t qio_channel_websock_write_wire(QIOChannelWebsock *ioc,
     ssize_t ret;
     ssize_t done = 0;
 
-    /* ping replies take priority over binary data */
-    if (!ioc->ping_reply.offset) {
-        qio_channel_websock_encode(ioc);
-    } else if (!ioc->encoutput.offset) {
-        buffer_move_empty(&ioc->encoutput, &ioc->ping_reply);
-    }
+    qio_channel_websock_encode(ioc);
 
     while (ioc->encoutput.offset > 0) {
         ret = qio_channel_write(ioc->master,
@@ -968,6 +965,11 @@ static ssize_t qio_channel_websock_write_wire(QIOChannelWebsock *ioc,
         }
         buffer_advance(&ioc->encoutput, ret);
         done += ret;
+        if (ioc->ping_remain < ret) {
+            ioc->ping_remain = 0;
+        } else {
+            ioc->ping_remain -= ret;
+        }
     }
     return done;
 }
@@ -1026,7 +1028,7 @@ static void qio_channel_websock_set_watch(QIOChannelWebsock *ioc)
         return;
     }
 
-    if (ioc->encoutput.offset || ioc->ping_reply.offset) {
+    if (ioc->encoutput.offset) {
         cond |= G_IO_OUT;
     }
     if (ioc->encinput.offset < QIO_CHANNEL_WEBSOCK_MAX_BUFFER &&
-- 
2.13.5

[Qemu-devel] [PATCH v1 3/7] io: get rid of qio_channel_websock_encode helper method

[Daniel P. Berrange]

The qio_channel_websock_encode method is only used in one place,
everything else calls qio_channel_websock_encode_buffer directly.
It can also be pushed up a level into the qio_channel_websock_writev
method, since every other caller of qio_channel_websock_write_wire
has already filled encoutput.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-websock.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/io/channel-websock.c b/io/channel-websock.c
index 1c34f68012..17f50f5bb1 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -616,18 +616,6 @@ static void qio_channel_websock_encode_buffer(QIOChannelWebsock *ioc,
 }
 
 
-static void qio_channel_websock_encode(QIOChannelWebsock *ioc)
-{
-    if (!ioc->rawoutput.offset) {
-        return;
-    }
-    qio_channel_websock_encode_buffer(
-        ioc, &ioc->encoutput, QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME,
-        &ioc->rawoutput);
-    buffer_reset(&ioc->rawoutput);
-}
-
-
 static ssize_t qio_channel_websock_write_wire(QIOChannelWebsock *, Error **);
 
 
@@ -948,8 +936,6 @@ static ssize_t qio_channel_websock_write_wire(QIOChannelWebsock *ioc,
     ssize_t ret;
     ssize_t done = 0;
 
-    qio_channel_websock_encode(ioc);
-
     while (ioc->encoutput.offset > 0) {
         ret = qio_channel_write(ioc->master,
                                 (char *)ioc->encoutput.buffer,
@@ -1134,6 +1120,12 @@ static ssize_t qio_channel_websock_writev(QIOChannel *ioc,
     }
 
  done:
+    if (ioc->rawoutput.offset) {
+        qio_channel_websock_encode_buffer(
+            ioc, &ioc->encoutput, QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME,
+            &ioc->rawoutput);
+        buffer_reset(&ioc->rawoutput);
+    }
     ret = qio_channel_websock_write_wire(wioc, errp);
     if (ret < 0 &&
         ret != QIO_CHANNEL_ERR_BLOCK) {
-- 
2.13.5

[Qemu-devel] [PATCH v1 4/7] io: pass a struct iovec into qio_channel_websock_encode

[Daniel P. Berrange]

Instead of requiring use of another Buffer, pass a struct iovec
into qio_channel_websock_encode, which gives callers more
flexibility in how they process data.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-websock.c | 69 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 42 insertions(+), 27 deletions(-)

diff --git a/io/channel-websock.c b/io/channel-websock.c
index 17f50f5bb1..ad62dda479 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -582,11 +582,14 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
 }
 
 
-static void qio_channel_websock_encode_buffer(QIOChannelWebsock *ioc,
-                                              Buffer *output,
-                                              uint8_t opcode, Buffer *buffer)
+static void qio_channel_websock_encode(QIOChannelWebsock *ioc,
+                                       uint8_t opcode,
+                                       const struct iovec *iov,
+                                       size_t niov,
+                                       size_t size)
 {
     size_t header_size;
+    size_t i;
     union {
         char buf[QIO_CHANNEL_WEBSOCK_HEADER_LEN_64_BIT];
         QIOChannelWebsockHeader ws;
@@ -594,25 +597,31 @@ static void qio_channel_websock_encode_buffer(QIOChannelWebsock *ioc,
 
     header.ws.b0 = QIO_CHANNEL_WEBSOCK_HEADER_FIELD_FIN |
         (opcode & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE);
-    if (buffer->offset < QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_7_BIT) {
-        header.ws.b1 = (uint8_t)buffer->offset;
+    if (size < QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_7_BIT) {
+        header.ws.b1 = (uint8_t)size;
         header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_7_BIT;
-    } else if (buffer->offset <
-               QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_16_BIT) {
+    } else if (size < QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_16_BIT) {
         header.ws.b1 = QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_MAGIC_16_BIT;
-        header.ws.u.s16.l16 = cpu_to_be16((uint16_t)buffer->offset);
+        header.ws.u.s16.l16 = cpu_to_be16((uint16_t)size);
         header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_16_BIT;
     } else {
         header.ws.b1 = QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_MAGIC_64_BIT;
-        header.ws.u.s64.l64 = cpu_to_be64(buffer->offset);
+        header.ws.u.s64.l64 = cpu_to_be64(size);
         header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_64_BIT;
     }
     header_size -= QIO_CHANNEL_WEBSOCK_HEADER_LEN_MASK;
 
-    trace_qio_channel_websock_encode(ioc, opcode, header_size, buffer->offset);
-    buffer_reserve(output, header_size + buffer->offset);
-    buffer_append(output, header.buf, header_size);
-    buffer_append(output, buffer->buffer, buffer->offset);
+    trace_qio_channel_websock_encode(ioc, opcode, header_size, size);
+    buffer_reserve(&ioc->encoutput, header_size + size);
+    buffer_append(&ioc->encoutput, header.buf, header_size);
+    for (i = 0; i < niov && size != 0; i++) {
+        size_t want = iov->iov_len;
+        if (want > size) {
+            want = size;
+        }
+        buffer_append(&ioc->encoutput, iov->iov_base, want);
+        size -= want;
+    }
 }
 
 
@@ -622,6 +631,7 @@ static ssize_t qio_channel_websock_write_wire(QIOChannelWebsock *, Error **);
 static void qio_channel_websock_write_close(QIOChannelWebsock *ioc,
                                             uint16_t code, const char *reason)
 {
+    struct iovec iov;
     buffer_reserve(&ioc->rawoutput, 2 + (reason ? strlen(reason) : 0));
     *(uint16_t *)(ioc->rawoutput.buffer + ioc->rawoutput.offset) =
         cpu_to_be16(code);
@@ -629,9 +639,10 @@ static void qio_channel_websock_write_close(QIOChannelWebsock *ioc,
     if (reason) {
         buffer_append(&ioc->rawoutput, reason, strlen(reason));
     }
-    qio_channel_websock_encode_buffer(
-        ioc, &ioc->encoutput, QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE,
-        &ioc->rawoutput);
+    iov.iov_base = ioc->rawoutput.buffer;
+    iov.iov_len = ioc->rawoutput.offset;
+    qio_channel_websock_encode(ioc, QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE,
+                               &iov, 1, iov.iov_len);
     buffer_reset(&ioc->rawoutput);
     qio_channel_websock_write_wire(ioc, NULL);
     qio_channel_shutdown(ioc->master, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
@@ -801,9 +812,10 @@ static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
         error_setg(errp, "websocket closed by peer");
         if (payload_len) {
             /* echo client status */
-            qio_channel_websock_encode_buffer(
-                ioc, &ioc->encoutput, QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE,
-                &ioc->encinput);
+            struct iovec iov = { .iov_base = ioc->encinput.buffer,
+                                 .iov_len = ioc->encinput.offset };
+            qio_channel_websock_encode(ioc, QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE,
+                                       &iov, 1, iov.iov_len);
             qio_channel_websock_write_wire(ioc, NULL);
             qio_channel_shutdown(ioc->master, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
         } else {
@@ -816,9 +828,10 @@ static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
         /* ping frames produce an immediate reply, as long as we've not still
          * got a previous ping queued, in which case we drop the new pong */
         if (ioc->ping_remain == 0) {
-            qio_channel_websock_encode_buffer(
-                ioc, &ioc->encoutput, QIO_CHANNEL_WEBSOCK_OPCODE_PONG,
-                &ioc->encinput);
+            struct iovec iov = { .iov_base = ioc->encinput.buffer,
+                                 .iov_len = ioc->encinput.offset };
+            qio_channel_websock_encode(ioc, QIO_CHANNEL_WEBSOCK_OPCODE_PONG,
+                                       &iov, 1, iov.iov_len);
             ioc->ping_remain = ioc->encoutput.offset;
         }
     }   /* pong frames are ignored */
@@ -1120,11 +1133,13 @@ static ssize_t qio_channel_websock_writev(QIOChannel *ioc,
     }
 
  done:
-    if (ioc->rawoutput.offset) {
-        qio_channel_websock_encode_buffer(
-            ioc, &ioc->encoutput, QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME,
-            &ioc->rawoutput);
-        buffer_reset(&ioc->rawoutput);
+    if (wioc->rawoutput.offset) {
+        struct iovec iov = { .iov_base = wioc->rawoutput.buffer,
+                             .iov_len = wioc->rawoutput.offset };
+        qio_channel_websock_encode(wioc,
+                                   QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME,
+                                   &iov, 1, iov.iov_len);
+        buffer_reset(&wioc->rawoutput);
     }
     ret = qio_channel_websock_write_wire(wioc, errp);
     if (ret < 0 &&
-- 
2.13.5

[Qemu-devel] [PATCH v1 5/7] io: get rid of bounce buffering in websock write path

[Daniel P. Berrange]

Currently most outbound I/O on the websock channel gets copied into the
rawoutput buffer, and then immediately copied again into the encoutput
buffer, with a header prepended. Now that qio_channel_websock_encode
accepts a struct iovec, we can trivially remove this bounce buffering
and write directly to encoutput.

In doing so, we also now correctly validate the encoutput size against
the QIO_CHANNEL_WEBSOCK_MAX_BUFFER limit.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/io/channel-websock.h |  1 -
 io/channel-websock.c         | 64 +++++++++++++++++++-------------------------
 2 files changed, 28 insertions(+), 37 deletions(-)

diff --git a/include/io/channel-websock.h b/include/io/channel-websock.h
index 3f92535cae..f2ac0fdad1 100644
--- a/include/io/channel-websock.h
+++ b/include/io/channel-websock.h
@@ -59,7 +59,6 @@ struct QIOChannelWebsock {
     Buffer encinput;
     Buffer encoutput;
     Buffer rawinput;
-    Buffer rawoutput;
     size_t payload_remain;
     size_t ping_remain;
     QIOChannelWebsockMask mask;
diff --git a/io/channel-websock.c b/io/channel-websock.c
index ad62dda479..455f5e322c 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -24,6 +24,7 @@
 #include "io/channel-websock.h"
 #include "crypto/hash.h"
 #include "trace.h"
+#include "qemu/iov.h"
 
 #include <time.h>
 
@@ -631,19 +632,22 @@ static ssize_t qio_channel_websock_write_wire(QIOChannelWebsock *, Error **);
 static void qio_channel_websock_write_close(QIOChannelWebsock *ioc,
                                             uint16_t code, const char *reason)
 {
-    struct iovec iov;
-    buffer_reserve(&ioc->rawoutput, 2 + (reason ? strlen(reason) : 0));
-    *(uint16_t *)(ioc->rawoutput.buffer + ioc->rawoutput.offset) =
-        cpu_to_be16(code);
-    ioc->rawoutput.offset += 2;
+    struct iovec iov[2] = {
+        { .iov_base = &code, .iov_len = sizeof(code) },
+    };
+    size_t niov = 1;
+    size_t size = iov[0].iov_len;
+
+    cpu_to_be16s(&code);
+
     if (reason) {
-        buffer_append(&ioc->rawoutput, reason, strlen(reason));
+        iov[1].iov_base = (void *)reason;
+        iov[1].iov_len = strlen(reason);
+        size += iov[1].iov_len;
+        niov++;
     }
-    iov.iov_base = ioc->rawoutput.buffer;
-    iov.iov_len = ioc->rawoutput.offset;
     qio_channel_websock_encode(ioc, QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE,
-                               &iov, 1, iov.iov_len);
-    buffer_reset(&ioc->rawoutput);
+                               iov, niov, size);
     qio_channel_websock_write_wire(ioc, NULL);
     qio_channel_shutdown(ioc->master, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
 }
@@ -891,7 +895,6 @@ static void qio_channel_websock_finalize(Object *obj)
     buffer_free(&ioc->encinput);
     buffer_free(&ioc->encoutput);
     buffer_free(&ioc->rawinput);
-    buffer_free(&ioc->rawoutput);
     object_unref(OBJECT(ioc->master));
     if (ioc->io_tag) {
         g_source_remove(ioc->io_tag);
@@ -1101,8 +1104,8 @@ static ssize_t qio_channel_websock_writev(QIOChannel *ioc,
                                           Error **errp)
 {
     QIOChannelWebsock *wioc = QIO_CHANNEL_WEBSOCK(ioc);
-    size_t i;
-    ssize_t done = 0;
+    ssize_t want = iov_size(iov, niov);
+    ssize_t avail;
     ssize_t ret;
 
     if (wioc->io_err) {
@@ -1115,32 +1118,21 @@ static ssize_t qio_channel_websock_writev(QIOChannel *ioc,
         return -1;
     }
 
-    for (i = 0; i < niov; i++) {
-        size_t want = iov[i].iov_len;
-        if ((want + wioc->rawoutput.offset) > QIO_CHANNEL_WEBSOCK_MAX_BUFFER) {
-            want = (QIO_CHANNEL_WEBSOCK_MAX_BUFFER - wioc->rawoutput.offset);
-        }
-        if (want == 0) {
-            goto done;
-        }
-
-        buffer_reserve(&wioc->rawoutput, want);
-        buffer_append(&wioc->rawoutput, iov[i].iov_base, want);
-        done += want;
-        if (want < iov[i].iov_len) {
-            break;
-        }
+    avail = wioc->encoutput.offset >= QIO_CHANNEL_WEBSOCK_MAX_BUFFER ?
+        0 : (QIO_CHANNEL_WEBSOCK_MAX_BUFFER - wioc->encoutput.offset);
+    if (want > avail) {
+        want = avail;
     }
 
- done:
-    if (wioc->rawoutput.offset) {
-        struct iovec iov = { .iov_base = wioc->rawoutput.buffer,
-                             .iov_len = wioc->rawoutput.offset };
+    if (want) {
         qio_channel_websock_encode(wioc,
                                    QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME,
-                                   &iov, 1, iov.iov_len);
-        buffer_reset(&wioc->rawoutput);
+                                   iov, niov, want);
     }
+
+    /* Even if want == 0, we'll try write_wire in case there's
+     * pending data we could usefully flush out
+     */
     ret = qio_channel_websock_write_wire(wioc, errp);
     if (ret < 0 &&
         ret != QIO_CHANNEL_ERR_BLOCK) {
@@ -1150,11 +1142,11 @@ static ssize_t qio_channel_websock_writev(QIOChannel *ioc,
 
     qio_channel_websock_set_watch(wioc);
 
-    if (done == 0) {
+    if (want == 0) {
         return QIO_CHANNEL_ERR_BLOCK;
     }
 
-    return done;
+    return want;
 }
 
 static int qio_channel_websock_set_blocking(QIOChannel *ioc,
-- 
2.13.5

[Qemu-devel] [PATCH v1 6/7] io: cope with websock 'Connection' header having multiple values

[Daniel P. Berrange]

The noVNC server sends a header "Connection: keep-alive, Upgrade" which
fails our simple equality test. Split the header on ',', trim whitespace
and then check for 'upgrade' token.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-websock.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/io/channel-websock.c b/io/channel-websock.c
index 455f5e322c..a21915881d 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -374,6 +374,9 @@ static void qio_channel_websock_handshake_process(QIOChannelWebsock *ioc,
     size_t nhdrs = G_N_ELEMENTS(hdrs);
     const char *protocols = NULL, *version = NULL, *key = NULL,
         *host = NULL, *connection = NULL, *upgrade = NULL;
+    char **connectionv;
+    bool upgraded = false;
+    size_t i;
 
     nhdrs = qio_channel_websock_extract_headers(ioc, buffer, hdrs, nhdrs, errp);
     if (!nhdrs) {
@@ -440,7 +443,16 @@ static void qio_channel_websock_handshake_process(QIOChannelWebsock *ioc,
         goto bad_request;
     }
 
-    if (strcasecmp(connection, QIO_CHANNEL_WEBSOCK_CONNECTION_UPGRADE) != 0) {
+    connectionv = g_strsplit(connection, ",", 0);
+    for (i = 0; connectionv != NULL && connectionv[i] != NULL; i++) {
+        g_strstrip(connectionv[i]);
+        if (strcasecmp(connectionv[i],
+                       QIO_CHANNEL_WEBSOCK_CONNECTION_UPGRADE) == 0) {
+            upgraded = true;
+        }
+    }
+    g_strfreev(connectionv);
+    if (!upgraded) {
         error_setg(errp, "No connection upgrade requested '%s'", connection);
         goto bad_request;
     }
-- 
2.13.5

[Qemu-devel] [PATCH v1 7/7] io: add trace points for websocket HTTP protocol headers

[Daniel P. Berrange]

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-websock.c | 4 ++++
 io/trace-events      | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/io/channel-websock.c b/io/channel-websock.c
index a21915881d..003d467715 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -224,6 +224,7 @@ qio_channel_websock_extract_headers(QIOChannelWebsock *ioc,
         goto bad_request;
     }
     *nl = '\0';
+    trace_qio_channel_websock_http_greeting(ioc, buffer);
 
     tmp = strchr(buffer, ' ');
     if (!tmp) {
@@ -425,6 +426,9 @@ static void qio_channel_websock_handshake_process(QIOChannelWebsock *ioc,
         goto bad_request;
     }
 
+    trace_qio_channel_websock_http_request(ioc, protocols, version,
+                                           host, connection, upgrade, key);
+
     if (!g_strrstr(protocols, QIO_CHANNEL_WEBSOCK_PROTOCOL_BINARY)) {
         error_setg(errp, "No '%s' protocol is supported by client '%s'",
                    QIO_CHANNEL_WEBSOCK_PROTOCOL_BINARY, protocols);
diff --git a/io/trace-events b/io/trace-events
index 801b5dcb61..f70bad7cbe 100644
--- a/io/trace-events
+++ b/io/trace-events
@@ -48,6 +48,8 @@ qio_channel_websock_handshake_pending(void *ioc, int status) "Websock handshake
 qio_channel_websock_handshake_reply(void *ioc) "Websock handshake reply ioc=%p"
 qio_channel_websock_handshake_fail(void *ioc, const char *msg) "Websock handshake fail ioc=%p err=%s"
 qio_channel_websock_handshake_complete(void *ioc) "Websock handshake complete ioc=%p"
+qio_channel_websock_http_greeting(void *ioc, const char *greeting) "Websocket HTTP request ioc=%p greeting='%s'"
+qio_channel_websock_http_request(void *ioc, const char *protocols, const char *version, const char *host, const char *connection, const char *upgrade, const char *key) "Websocket HTTP request ioc=%p protocols='%s' version='%s' host='%s' connection='%s' upgrade='%s' key='%s'"
 qio_channel_websock_header_partial_decode(void *ioc, size_t payloadlen, unsigned char fin, unsigned char opcode, unsigned char has_mask) "Websocket header decoded ioc=%p payload-len=%zu fin=0x%x opcode=0x%x has_mask=0x%x"
 qio_channel_websock_header_full_decode(void *ioc, size_t headerlen, size_t payloadlen, uint32_t mask) "Websocket header decoded ioc=%p header-len=%zu payload-len=%zu mask=0x%x"
 qio_channel_websock_payload_decode(void *ioc, uint8_t opcode, size_t payload_remain) "Websocket header decoded ioc=%p opcode=0x%x payload-remain=%zu"
-- 
2.13.5

Re: [Qemu-devel] [PATCH v1 1/7] io: monitor encoutput buffer size from websocket GSource

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2209 bytes --]

On 10/10/2017 10:43 AM, Daniel P. Berrange wrote:
> The websocket GSource is monitoring the size of the rawoutput
> buffer to determine if the channel can accepts more writes.
> The rawoutput buffer, however, is merely a temporary staging
> buffer before data is copied into the encoutput buffer. This

s/This/Thus/

> its size will always be zero when the GSource runs.
> 
> This flaw causes the encoutput buffer to grow without bound
> if the other end of the underlying data channel doesn't
> read data being sent. This can be seen with VNC if a client
> is on a slow WAN link and the guest OS is sending many screen
> updates. A malicious VNC client can act like it is on a slow
> link by playing a video in the guest and then reading data
> very slowly, causing QEMU host memory to expand arbitrarily.
> 
> This issue is assigned CVE-2017-????, publically reported in

If we get the assignment in time, I'm sure you'll update this before the
PULL request.

> 
>   https://bugs.launchpad.net/qemu/+bug/1718964
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  io/channel-websock.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/io/channel-websock.c b/io/channel-websock.c
> index d1d471f86e..04bcc059cd 100644
> --- a/io/channel-websock.c
> +++ b/io/channel-websock.c
> @@ -28,7 +28,7 @@
>  #include <time.h>
>  
>  
> -/* Max amount to allow in rawinput/rawoutput buffers */
> +/* Max amount to allow in rawinput/encoutput buffers */
>  #define QIO_CHANNEL_WEBSOCK_MAX_BUFFER 8192
>  
>  #define QIO_CHANNEL_WEBSOCK_CLIENT_KEY_LEN 24
> @@ -1208,7 +1208,7 @@ qio_channel_websock_source_check(GSource *source)
>      if (wsource->wioc->rawinput.offset || wsource->wioc->io_eof) {
>          cond |= G_IO_IN;
>      }
> -    if (wsource->wioc->rawoutput.offset < QIO_CHANNEL_WEBSOCK_MAX_BUFFER) {
> +    if (wsource->wioc->encoutput.offset < QIO_CHANNEL_WEBSOCK_MAX_BUFFER) {
>          cond |= G_IO_OUT;
>      }
>  
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v1 2/7] io: simplify websocket ping reply handling

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2348 bytes --]

On 10/10/2017 10:43 AM, Daniel P. Berrange wrote:
> We must ensure we don't get flooded with ping replies if the outbound
> channel is slow. Currently we do this by keeping the ping reply in a
> separate temporary buffer and only writing it if the encoutput buffer
> is completely empty. This is overly pessimistic, as it is reasonable
> to add a ping reply to the encoutput buffer even if it has previous
> data in it, as long as that previous data doesn't include a ping
> reply.
> 
> To track this better, put the ping reply directly into the encoutput
> buffer, and then record the size of encoutput at this time in
> ping_remain. As we write encoutput to the underlying channel, we
> can decrement the ping_remain counter. Once it hits zero, we can
> accept further ping replies for transmission.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/io/channel-websock.h |  2 +-
>  io/channel-websock.c         | 28 +++++++++++++++-------------
>  2 files changed, 16 insertions(+), 14 deletions(-)
> 

> +++ b/io/channel-websock.c
> @@ -825,11 +825,14 @@ static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
>          }
>          return -1;
>      } else if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_PING) {
> -        /* ping frames produce an immediate reply */
> -        buffer_reset(&ioc->ping_reply);
> -        qio_channel_websock_encode_buffer(
> -            ioc, &ioc->ping_reply, QIO_CHANNEL_WEBSOCK_OPCODE_PONG,
> -            &ioc->encinput);
> +        /* ping frames produce an immediate reply, as long as we've not still
> +         * got a previous ping queued, in which case we drop the new pong */

Wouldn't that be a 'previous pong queued'?

> +        if (ioc->ping_remain == 0) {
> +            qio_channel_websock_encode_buffer(
> +                ioc, &ioc->encoutput, QIO_CHANNEL_WEBSOCK_OPCODE_PONG,
> +                &ioc->encinput);
> +            ioc->ping_remain = ioc->encoutput.offset;
> +        }

But if you change the comment, then naming the variable pong_remain may
make more sense.

But naming is a bikeshed issue, so either way,

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v1 3/7] io: get rid of qio_channel_websock_encode helper method

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 720 bytes --]

On 10/10/2017 10:43 AM, Daniel P. Berrange wrote:
> The qio_channel_websock_encode method is only used in one place,
> everything else calls qio_channel_websock_encode_buffer directly.
> It can also be pushed up a level into the qio_channel_websock_writev
> method, since every other caller of qio_channel_websock_write_wire
> has already filled encoutput.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  io/channel-websock.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v1 4/7] io: pass a struct iovec into qio_channel_websock_encode

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2053 bytes --]

On 10/10/2017 10:43 AM, Daniel P. Berrange wrote:
> Instead of requiring use of another Buffer, pass a struct iovec
> into qio_channel_websock_encode, which gives callers more
> flexibility in how they process data.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  io/channel-websock.c | 69 ++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 42 insertions(+), 27 deletions(-)
> 

> +static void qio_channel_websock_encode(QIOChannelWebsock *ioc,
> +                                       uint8_t opcode,
> +                                       const struct iovec *iov,
> +                                       size_t niov,
> +                                       size_t size)
>  {

Is size redundant with iov_size(iov, niov)?  Or is a caller allowed to
pass a smaller size (tail end of iov is not encoded) or larger size
(encoding stops at end of iov, even if size is not exhausted)?  I'd lean
towards the former (one less parameter), especially since all callers in
this patch could have passed iov_size(&iov, 1) for the same effect.

> -    trace_qio_channel_websock_encode(ioc, opcode, header_size, buffer->offset);
> -    buffer_reserve(output, header_size + buffer->offset);
> -    buffer_append(output, header.buf, header_size);
> -    buffer_append(output, buffer->buffer, buffer->offset);
> +    trace_qio_channel_websock_encode(ioc, opcode, header_size, size);
> +    buffer_reserve(&ioc->encoutput, header_size + size);
> +    buffer_append(&ioc->encoutput, header.buf, header_size);
> +    for (i = 0; i < niov && size != 0; i++) {
> +        size_t want = iov->iov_len;
> +        if (want > size) {
> +            want = size;
> +        }
> +        buffer_append(&ioc->encoutput, iov->iov_base, want);
> +        size -= want;
> +    }

Umm, where are you incrementing iov? It appears you only tested with
niov == 1.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v1 5/7] io: get rid of bounce buffering in websock write path

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2192 bytes --]

On 10/10/2017 10:43 AM, Daniel P. Berrange wrote:
> Currently most outbound I/O on the websock channel gets copied into the
> rawoutput buffer, and then immediately copied again into the encoutput
> buffer, with a header prepended. Now that qio_channel_websock_encode
> accepts a struct iovec, we can trivially remove this bounce buffering
> and write directly to encoutput.
> 
> In doing so, we also now correctly validate the encoutput size against
> the QIO_CHANNEL_WEBSOCK_MAX_BUFFER limit.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/io/channel-websock.h |  1 -
>  io/channel-websock.c         | 64 +++++++++++++++++++-------------------------
>  2 files changed, 28 insertions(+), 37 deletions(-)
> 

> -    iov.iov_base = ioc->rawoutput.buffer;
> -    iov.iov_len = ioc->rawoutput.offset;
>      qio_channel_websock_encode(ioc, QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE,
> -                               &iov, 1, iov.iov_len);
> -    buffer_reset(&ioc->rawoutput);
> +                               iov, niov, size);

If 4/7 changes this to compute size from the iov/niov, then this has
knock-on impact.

> @@ -1115,32 +1118,21 @@ static ssize_t qio_channel_websock_writev(QIOChannel *ioc,
>          return -1;

> +    avail = wioc->encoutput.offset >= QIO_CHANNEL_WEBSOCK_MAX_BUFFER ?
> +        0 : (QIO_CHANNEL_WEBSOCK_MAX_BUFFER - wioc->encoutput.offset);
> +    if (want > avail) {
> +        want = avail;
>      }
>  
> - done:
> -    if (wioc->rawoutput.offset) {
> -        struct iovec iov = { .iov_base = wioc->rawoutput.buffer,
> -                             .iov_len = wioc->rawoutput.offset };
> +    if (want) {
>          qio_channel_websock_encode(wioc,
>                                     QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME,
> -                                   &iov, 1, iov.iov_len);
> -        buffer_reset(&wioc->rawoutput);
> +                                   iov, niov, want);

then again, I see that you DO support want < iov_size(iov, niov).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v1 2/7] io: simplify websocket ping reply handling

[Daniel P. Berrange]

On Tue, Oct 10, 2017 at 11:55:25AM -0500, Eric Blake wrote:
> On 10/10/2017 10:43 AM, Daniel P. Berrange wrote:
> > We must ensure we don't get flooded with ping replies if the outbound
> > channel is slow. Currently we do this by keeping the ping reply in a
> > separate temporary buffer and only writing it if the encoutput buffer
> > is completely empty. This is overly pessimistic, as it is reasonable
> > to add a ping reply to the encoutput buffer even if it has previous
> > data in it, as long as that previous data doesn't include a ping
> > reply.
> > 
> > To track this better, put the ping reply directly into the encoutput
> > buffer, and then record the size of encoutput at this time in
> > ping_remain. As we write encoutput to the underlying channel, we
> > can decrement the ping_remain counter. Once it hits zero, we can
> > accept further ping replies for transmission.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  include/io/channel-websock.h |  2 +-
> >  io/channel-websock.c         | 28 +++++++++++++++-------------
> >  2 files changed, 16 insertions(+), 14 deletions(-)
> > 
> 
> > +++ b/io/channel-websock.c
> > @@ -825,11 +825,14 @@ static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
> >          }
> >          return -1;
> >      } else if (ioc->opcode == QIO_CHANNEL_WEBSOCK_OPCODE_PING) {
> > -        /* ping frames produce an immediate reply */
> > -        buffer_reset(&ioc->ping_reply);
> > -        qio_channel_websock_encode_buffer(
> > -            ioc, &ioc->ping_reply, QIO_CHANNEL_WEBSOCK_OPCODE_PONG,
> > -            &ioc->encinput);
> > +        /* ping frames produce an immediate reply, as long as we've not still
> > +         * got a previous ping queued, in which case we drop the new pong */
> 
> Wouldn't that be a 'previous pong queued'?

Indeed

> 
> > +        if (ioc->ping_remain == 0) {
> > +            qio_channel_websock_encode_buffer(
> > +                ioc, &ioc->encoutput, QIO_CHANNEL_WEBSOCK_OPCODE_PONG,
> > +                &ioc->encinput);
> > +            ioc->ping_remain = ioc->encoutput.offset;
> > +        }
> 
> But if you change the comment, then naming the variable pong_remain may
> make more sense.

Yeah, that's sensible.

> 
> But naming is a bikeshed issue, so either way,
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

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 :|

Re: [Qemu-devel] [PATCH v1 1/7] io: monitor encoutput buffer size from websocket GSource

[Daniel P. Berrange]

On Tue, Oct 10, 2017 at 11:51:00AM -0500, Eric Blake wrote:
> On 10/10/2017 10:43 AM, Daniel P. Berrange wrote:
> > The websocket GSource is monitoring the size of the rawoutput
> > buffer to determine if the channel can accepts more writes.
> > The rawoutput buffer, however, is merely a temporary staging
> > buffer before data is copied into the encoutput buffer. This
> 
> s/This/Thus/
> 
> > its size will always be zero when the GSource runs.
> > 
> > This flaw causes the encoutput buffer to grow without bound
> > if the other end of the underlying data channel doesn't
> > read data being sent. This can be seen with VNC if a client
> > is on a slow WAN link and the guest OS is sending many screen
> > updates. A malicious VNC client can act like it is on a slow
> > link by playing a video in the guest and then reading data
> > very slowly, causing QEMU host memory to expand arbitrarily.
> > 
> > This issue is assigned CVE-2017-????, publically reported in
> 
> If we get the assignment in time, I'm sure you'll update this before the
> PULL request.

Yes, exactly the plan...



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 :|

Re: [Qemu-devel] [PATCH v1 4/7] io: pass a struct iovec into qio_channel_websock_encode

[Daniel P. Berrange]

On Tue, Oct 10, 2017 at 12:18:26PM -0500, Eric Blake wrote:
> On 10/10/2017 10:43 AM, Daniel P. Berrange wrote:
> > Instead of requiring use of another Buffer, pass a struct iovec
> > into qio_channel_websock_encode, which gives callers more
> > flexibility in how they process data.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  io/channel-websock.c | 69 ++++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 42 insertions(+), 27 deletions(-)
> > 
> 
> > +static void qio_channel_websock_encode(QIOChannelWebsock *ioc,
> > +                                       uint8_t opcode,
> > +                                       const struct iovec *iov,
> > +                                       size_t niov,
> > +                                       size_t size)
> >  {
> 
> Is size redundant with iov_size(iov, niov)?  Or is a caller allowed to
> pass a smaller size (tail end of iov is not encoded) or larger size
> (encoding stops at end of iov, even if size is not exhausted)?  I'd lean
> towards the former (one less parameter), especially since all callers in
> this patch could have passed iov_size(&iov, 1) for the same effect.

You've already noticed the code in later patch which passes a smaller
size. This lets me avoid having to duplicate the iovec array and
set smaller iov_len

> 
> > -    trace_qio_channel_websock_encode(ioc, opcode, header_size, buffer->offset);
> > -    buffer_reserve(output, header_size + buffer->offset);
> > -    buffer_append(output, header.buf, header_size);
> > -    buffer_append(output, buffer->buffer, buffer->offset);
> > +    trace_qio_channel_websock_encode(ioc, opcode, header_size, size);
> > +    buffer_reserve(&ioc->encoutput, header_size + size);
> > +    buffer_append(&ioc->encoutput, header.buf, header_size);
> > +    for (i = 0; i < niov && size != 0; i++) {
> > +        size_t want = iov->iov_len;
> > +        if (want > size) {
> > +            want = size;
> > +        }
> > +        buffer_append(&ioc->encoutput, iov->iov_base, want);
> > +        size -= want;
> > +    }
> 
> Umm, where are you incrementing iov? It appears you only tested with
> niov == 1.

Opps. Yeah, we need to cope with niov > 0 to satisfy qio_channel_writev
API contract, but the VNC server only ever sends a single iov element
so I didn't hit the bug.

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 :|

Re: [Qemu-devel] [PATCH v1 6/7] io: cope with websock 'Connection' header having multiple values

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]

On 10/10/2017 10:43 AM, Daniel P. Berrange wrote:
> The noVNC server sends a header "Connection: keep-alive, Upgrade" which
> fails our simple equality test. Split the header on ',', trim whitespace
> and then check for 'upgrade' token.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  io/channel-websock.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 

> @@ -440,7 +443,16 @@ static void qio_channel_websock_handshake_process(QIOChannelWebsock *ioc,
>          goto bad_request;
>      }
>  
> -    if (strcasecmp(connection, QIO_CHANNEL_WEBSOCK_CONNECTION_UPGRADE) != 0) {

My first thought was whether you could just use strcasestr() instead of
strcasecmp(), rather than the malloc overhead of g_strsplit().  But that
would treat "noUpgradeGarbage" as success, making your approach a bit
stricter.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v1 7/7] io: add trace points for websocket HTTP protocol headers

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

On 10/10/2017 10:43 AM, Daniel P. Berrange wrote:
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  io/channel-websock.c | 4 ++++
>  io/trace-events      | 2 ++
>  2 files changed, 6 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v1 6/7] io: cope with websock 'Connection' header having multiple values

[Daniel P. Berrange]

On Tue, Oct 10, 2017 at 12:42:55PM -0500, Eric Blake wrote:
> On 10/10/2017 10:43 AM, Daniel P. Berrange wrote:
> > The noVNC server sends a header "Connection: keep-alive, Upgrade" which
> > fails our simple equality test. Split the header on ',', trim whitespace
> > and then check for 'upgrade' token.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  io/channel-websock.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> 
> > @@ -440,7 +443,16 @@ static void qio_channel_websock_handshake_process(QIOChannelWebsock *ioc,
> >          goto bad_request;
> >      }
> >  
> > -    if (strcasecmp(connection, QIO_CHANNEL_WEBSOCK_CONNECTION_UPGRADE) != 0) {
> 
> My first thought was whether you could just use strcasestr() instead of
> strcasecmp(), rather than the malloc overhead of g_strsplit().  But that
> would treat "noUpgradeGarbage" as success, making your approach a bit
> stricter.

Also note that when reading HTTP headers we've already limited max data
size to 4k for the entire HTTP header set. So we're doing g_strsplit
over a pretty short piece of data, so negligible perf implications
of that.

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

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 :|