vici: Increase vici message length header from 16 to 32 bits

While we currently have no need for messages larger than 65KB, we should design
the protocol to be future-proof, as we plan to keep at least to lowest protocol
layer stable.

To avoid any allocation issues, we currently keep the message size limit at
512KB.

diff --git a/src/libcharon/plugins/vici/README.md b/src/libcharon/plugins/vici/README.md
index ff5031e..46fa5b5 100644 (file)
--- a/src/libcharon/plugins/vici/README.md
+++ b/src/libcharon/plugins/vici/README.md
@@ -14,10 +14,10 @@ A client connects to this service to access functionality. It may send an
 arbitrary number of packets over the connection before closing it.
 
 To exchange data, the transport protocol is segmented into byte sequences.
-Each byte sequence is prefixed by a 16-bit length header in network order,
-followed by the data. The maximum segment length is 65535 bytes, and the length
-field contains the length of the data only, not including the length field
-itself.
+Each byte sequence is prefixed by a 32-bit length header in network order,
+followed by the data. The maximum segment length is currently limited to 512KB
+of data, and the length field contains the length of the data only, not
+including the length field itself.
 
 The order of byte sequences must be strict, byte sequences must arrive in the
 same order as sent.

diff --git a/src/libcharon/plugins/vici/libvici.c b/src/libcharon/plugins/vici/libvici.c
index becd886..211fefd 100644 (file)
--- a/src/libcharon/plugins/vici/libvici.c
+++ b/src/libcharon/plugins/vici/libvici.c
@@ -16,6 +16,7 @@
 #include "libvici.h"
 #include "vici_builder.h"
 #include "vici_dispatcher.h"
+#include "vici_socket.h"
 
 #include <library.h>
 #include <threading/mutex.h>
@@ -122,7 +123,7 @@ static bool read_error(vici_conn_t *conn, int err)
 /**
  * Handle a command response message
  */
-static bool handle_response(vici_conn_t *conn, u_int16_t len)
+static bool handle_response(vici_conn_t *conn, u_int32_t len)
 {
        chunk_t buf;
 
@@ -139,7 +140,7 @@ static bool handle_response(vici_conn_t *conn, u_int16_t len)
 /**
  * Dispatch received event message
  */
-static bool handle_event(vici_conn_t *conn, u_int16_t len)
+static bool handle_event(vici_conn_t *conn, u_int32_t len)
 {
        vici_message_t *message;
        event_t *event;
@@ -197,7 +198,7 @@ static bool handle_event(vici_conn_t *conn, u_int16_t len)
 CALLBACK(on_read, bool,
        vici_conn_t *conn, stream_t *stream)
 {
-       u_int16_t len;
+       u_int32_t len;
        u_int8_t op;
        ssize_t hlen;
 
@@ -218,7 +219,11 @@ CALLBACK(on_read, bool,
                }
        }
 
-       len = ntohs(len);
+       len = ntohl(len);
+       if (len > VICI_MESSAGE_SIZE_MAX)
+       {
+               return read_error(conn, EBADMSG);
+       }
        if (len-- < sizeof(op))
        {
                return read_error(conn, EBADMSG);
@@ -353,7 +358,7 @@ vici_res_t* vici_submit(vici_req_t *req, vici_conn_t *conn)
        vici_message_t *message;
        vici_res_t *res;
        chunk_t data;
-       u_int16_t len;
+       u_int32_t len;
        u_int8_t namelen, op;
 
        message = req->b->finalize(req->b);
@@ -366,7 +371,7 @@ vici_res_t* vici_submit(vici_req_t *req, vici_conn_t *conn)
        op = VICI_CMD_REQUEST;
        namelen = strlen(req->name);
        data = message->get_encoding(message);
-       len = htons(sizeof(op) + sizeof(namelen) + namelen + data.len);
+       len = htonl(sizeof(op) + sizeof(namelen) + namelen + data.len);
 
        if (!conn->stream->write_all(conn->stream, &len, sizeof(len)) ||
                !conn->stream->write_all(conn->stream, &op, sizeof(op)) ||
@@ -679,13 +684,13 @@ void vici_free_res(vici_res_t *res)
 int vici_register(vici_conn_t *conn, char *name, vici_event_cb_t cb, void *user)
 {
        event_t *event;
-       u_int16_t len;
+       u_int32_t len;
        u_int8_t namelen, op;
        int ret = 1;
 
        op = cb ? VICI_EVENT_REGISTER : VICI_EVENT_UNREGISTER;
        namelen = strlen(name);
-       len = htons(sizeof(op) + sizeof(namelen) + namelen);
+       len = htonl(sizeof(op) + sizeof(namelen) + namelen);
        if (!conn->stream->write_all(conn->stream, &len, sizeof(len)) ||
                !conn->stream->write_all(conn->stream, &op, sizeof(op)) ||
                !conn->stream->write_all(conn->stream, &namelen, sizeof(namelen)) ||

diff --git a/src/libcharon/plugins/vici/suites/test_socket.c b/src/libcharon/plugins/vici/suites/test_socket.c
index a9ebdbf..032445b 100644 (file)
--- a/src/libcharon/plugins/vici/suites/test_socket.c
+++ b/src/libcharon/plugins/vici/suites/test_socket.c
@@ -32,7 +32,7 @@ static void echo_inbound(void *user, u_int id, chunk_t buf)
 
        ck_assert_int_eq(data->id, id);
        /* count number of bytes, including the header */
-       data->bytes += buf.len + sizeof(u_int16_t);
+       data->bytes += buf.len + sizeof(u_int32_t);
        /* echo back data chunk */
        data->s->send(data->s, id, chunk_clone(buf));
 }
@@ -73,13 +73,13 @@ START_TEST(test_echo)
        stream_t *c;
        test_data_t data = {};
        chunk_t x, m = chunk_from_chars(
-               0x00,0x00,
-               0x00,0x01,      0x01,
-               0x00,0x05,      0x11,0x12,0x13,0x14,0x15,
-               0x00,0x0A,      0x21,0x22,0x23,0x24,0x25,0x26,0x27,0x28,0x29,0x02A,
+               0x00,0x00,0x00,0x00,
+               0x00,0x00,0x00,0x01,    0x01,
+               0x00,0x00,0x00,0x05,    0x11,0x12,0x13,0x14,0x15,
+               0x00,0x00,0x00,0x0A,    0x21,0x22,0x23,0x24,0x25,0x26,0x27,0x28,0x29,0x02A,
        );
        char buf[m.len];
-       u_int16_t len;
+       u_int32_t len;
 
        lib->processor->set_threads(lib->processor, 4);
 

diff --git a/src/libcharon/plugins/vici/vici_socket.c b/src/libcharon/plugins/vici/vici_socket.c
index 5a0fc81..05c2374 100644 (file)
--- a/src/libcharon/plugins/vici/vici_socket.c
+++ b/src/libcharon/plugins/vici/vici_socket.c
@@ -95,11 +95,11 @@ typedef struct {
        /** bytes of length header sent/received */
        u_char hdrlen;
        /** bytes of length header */
-       char hdr[sizeof(u_int16_t)];
+       char hdr[sizeof(u_int32_t)];
        /** send/receive buffer on heap */
        chunk_t buf;
        /** bytes sent/received in buffer */
-       u_int16_t done;
+       u_int32_t done;
 } msg_buf_t;
 
 /**
@@ -397,6 +397,7 @@ CALLBACK(on_write, bool,
 static bool do_read(private_vici_socket_t *this, entry_t *entry,
                                        stream_t *stream)
 {
+       u_int32_t msglen;
        ssize_t len;
 
        /* assemble the length header first */
@@ -420,8 +421,15 @@ static bool do_read(private_vici_socket_t *this, entry_t *entry,
                entry->in.hdrlen += len;
                if (entry->in.hdrlen == sizeof(entry->in.hdr))
                {
+                       msglen = untoh32(entry->in.hdr);
+                       if (msglen > VICI_MESSAGE_SIZE_MAX)
+                       {
+                               DBG1(DBG_CFG, "vici message length %u exceeds %u bytes limit, "
+                                        "ignored", msglen, VICI_MESSAGE_SIZE_MAX);
+                               return FALSE;
+                       }
                        /* header complete, continue with data */
-                       entry->in.buf = chunk_alloc(untoh16(entry->in.hdr));
+                       entry->in.buf = chunk_alloc(msglen);
                }
        }
 
@@ -584,7 +592,7 @@ CALLBACK(enable_writer, job_requeue_t,
 METHOD(vici_socket_t, send_, void,
        private_vici_socket_t *this, u_int id, chunk_t msg)
 {
-       if (msg.len <= (u_int16_t)~0)
+       if (msg.len <= VICI_MESSAGE_SIZE_MAX)
        {
                entry_selector_t *sel;
                msg_buf_t *out;
@@ -596,7 +604,7 @@ METHOD(vici_socket_t, send_, void,
                        INIT(out,
                                .buf = msg,
                        );
-                       htoun16(out->hdr, msg.len);
+                       htoun32(out->hdr, msg.len);
 
                        array_insert(entry->out, ARRAY_TAIL, out);
                        if (array_count(entry->out) == 1)
@@ -619,7 +627,8 @@ METHOD(vici_socket_t, send_, void,
        }
        else
        {
-               DBG1(DBG_CFG, "vici message exceeds maximum size, discarded");
+               DBG1(DBG_CFG, "vici message size %zu exceeds maximum size of %u, "
+                        "discarded", msg.len, VICI_MESSAGE_SIZE_MAX);
                chunk_clear(&msg);
        }
 }

diff --git a/src/libcharon/plugins/vici/vici_socket.h b/src/libcharon/plugins/vici/vici_socket.h
index f591998..8727836 100644 (file)
--- a/src/libcharon/plugins/vici/vici_socket.h
+++ b/src/libcharon/plugins/vici/vici_socket.h
@@ -23,6 +23,11 @@
 
 #include <library.h>
 
+/**
+ * Maximum size of a single message exchanged.
+ */
+#define VICI_MESSAGE_SIZE_MAX (512 * 1024)
+
 typedef struct vici_socket_t vici_socket_t;
 
 /**